Page MenuHomeFreeBSD
Differential D25595 Diff 74902 sys/net/route/nhop_ctl.c

Changeset View

Standalone View

View Options

sys/net/route/nhop_ctl.c

Show First 20 Lines • Show All 99 Lines • ▼ Show 20 Lines
static uma_zone_t nhops_zone; /* Global zone for each and every nexthop */ static uma_zone_t nhops_zone; /* Global zone for each and every nexthop */
#define NHOP_OBJECT_ALIGNED_SIZE roundup2(sizeof(struct nhop_object), \ #define NHOP_OBJECT_ALIGNED_SIZE roundup2(sizeof(struct nhop_object), \
2 * CACHE_LINE_SIZE) 2 * CACHE_LINE_SIZE)
#define NHOP_PRIV_ALIGNED_SIZE roundup2(sizeof(struct nhop_priv), \ #define NHOP_PRIV_ALIGNED_SIZE roundup2(sizeof(struct nhop_priv), \
2 * CACHE_LINE_SIZE) 2 * CACHE_LINE_SIZE)
#define MULTI_BIT_SET(x) (x && (x & (x - 1))) /* Inverse of IS_POW2() */
donnerUnsubmitted
Not Done
Inline Actions

Let's take for example "x = 4"

ONE_BIT_SET(4) = 4 && (0b100 & 0b011) = 4 && 0 = false

or "x = 5"

ONE_BIT_SET(5) = 5 && (0b101 & 0b100)  = 5 && 4 = true

Is this the intended behavior?
The name "ONE_BIT_SET" suggests otherwise.

donner: Let's take for example "x = 4" ONE_BIT_SET(4) = 4 && (0b100 & 0b011) = 4 && 0 = false or "x…
ncAuthorUnsubmitted
Done
Inline Actions

Yes.

However, "ONE_BIT_SET" should be "MULTI_BIT_SET". Patch upcoming.

nc: Yes. However, "ONE_BIT_SET" should be "MULTI_BIT_SET". Patch upcoming.
void void
nhops_init(void) nhops_init(void)
{ {
nhops_zone = uma_zcreate("routing nhops", nhops_zone = uma_zcreate("routing nhops",
NHOP_OBJECT_ALIGNED_SIZE + NHOP_PRIV_ALIGNED_SIZE, NHOP_OBJECT_ALIGNED_SIZE + NHOP_PRIV_ALIGNED_SIZE,
NULL, NULL, NULL, NULL, UMA_ALIGN_PTR, 0); NULL, NULL, NULL, NULL, UMA_ALIGN_PTR, 0);
} }
▲ Show 20 Lines • Show All 255 Lines • ▼ Show 20 Lines
* *
* Returns: * Returns:
* 0 on success * 0 on success
*/ */
static int static int
alter_nhop_from_info(struct nhop_object *nh, struct rt_addrinfo *info) alter_nhop_from_info(struct nhop_object *nh, struct rt_addrinfo *info)
{ {
struct sockaddr *info_gw; struct sockaddr *info_gw;
int flags;
melifaroUnsubmitted
Done
Inline Actions

Could you please rename it to something more meaningful? flags or something like that?

melifaro: Could you please rename it to something more meaningful? `flags` or something like that?
int error; int error;
/* Update MTU if set in the request*/ /* Update MTU if set in the request*/
set_nhop_mtu_from_info(nh, info); set_nhop_mtu_from_info(nh, info);
/* XXX: allow only one of BLACKHOLE,REJECT,GATEWAY */ /* allow only one of BLACKHOLE,REJECT,GATEWAY */
flags = nh->nh_priv->rt_flags & (RTF_BLACKHOLE | RTF_REJECT | RTF_GATEWAY);
if (MULTI_BIT_SET(flags))
melifaroUnsubmitted
Done
Inline Actions

Can you make it a macro, like in other parts of the kernel?

#define POWER_OF_2(n)   (!((n) & ((n)-1)))
#define IS_POW2(x) (x && ((x & (x - 1)) == 0))
melifaro: Can you make it a macro, like in other parts of the kernel? ``` #define POWER_OF_2(n) (!((n)…
return (EINVAL);
donnerUnsubmitted
Done
Inline Actions

I'd summarize the cases:

if ( (nh->nh_priv->rt_flags &
     (RTF_BLACKHOLE | RTF_REJECT | RTF_GATEWAY)) != 0 )
donner: I'd summarize the cases: if ( (nh->nh_priv->rt_flags & (RTF_BLACKHOLE | RTF_REJECT…
rgrimesUnsubmitted
Done
Inline Actions

AH, that looks like it only allows "None OF"

rgrimes: AH, that looks like it only allows "None OF"
donnerUnsubmitted
Done
Inline Actions

There is no guarantee in the language, that the boolean result is converted to "1" for true. The only guarantee is, that the numerical value of false is "0".

How about:

switch (nh->nh_priv->rt_flags & (RTF_BLACKHOLE | RTF_REJECT | RTF_GATEWAY)) {
   case RTF_BLACKHOLE | RTF_REJECT | RTF_GATEWAY:
     ...
   default:
     break;
}
donner: There is no guarantee in the language, that the boolean result is converted to "1" for true.
rgrimesUnsubmitted
Done
Inline Actions

Your first attempt would allow any combination of bits to be set, including only 1 of them and would lead to a return EINVAL. Do we agree on that?
It looks like this new one works, but i I suspect the generated code is not much better. It should be possible to do a function of the form I use a to simplify, the long form should all end up in register, this needs double checked I could have the if backwards.

a = nh->nh_priv->rt_flags & (RTF_BLACKHOLE | RTF_REJECT | RTF_GATEWAY)
if (a && !(a & (a-1)))
    return(EINVAL)

This is the power of 2 method to see if more than 1 bit is set, i believe it reduces this to 1 branch.

rgrimes: Your first attempt would allow any combination of bits to be set, including only 1 of them and…
ncAuthorUnsubmitted
Done
Inline Actions

Sorry for the delay.

I'm no expert in bitwise, but I think your logic is correct, just that you need:

if (a && (a & a - 1))

Will upload an updated patch.

Source: https://stackoverflow.com/questions/51094594/how-to-check-if-exactly-one-bit-is-set-in-an-int/51094793

nc: Sorry for the delay. I'm no expert in bitwise, but I think your logic is correct, just that…
donnerUnsubmitted
Done
Inline Actions

The bit operation is very hard to read.
Such optimization should be done by the compiler only, unless there is a strong reason.

So I'd like to urge you to reconsider the "switch/case" approach instead

donner: The bit operation is very hard to read. Such optimization should be done by the compiler only…
/* Allow some flags (FLAG1,STATIC,BLACKHOLE,REJECT) to be toggled on change. */ /* Allow some flags (FLAG1,STATIC,BLACKHOLE,REJECT) to be toggled on change. */
nh->nh_priv->rt_flags &= ~RTF_FMASK; nh->nh_priv->rt_flags &= ~RTF_FMASK;
nh->nh_priv->rt_flags |= info->rti_flags & RTF_FMASK; nh->nh_priv->rt_flags |= info->rti_flags & RTF_FMASK;
/* Consider gateway change */ /* Consider gateway change */
info_gw = info->rti_info[RTAX_GATEWAY]; info_gw = info->rti_info[RTAX_GATEWAY];
if (info_gw != NULL) { if (info_gw != NULL) {
▲ Show 20 Lines • Show All 455 Lines • Show Last 20 Lines