subject:"bgpd, retire F

Re: bgpd, retire F_RTLABEL flag

2022-06-07 Thread Claudio Jeker

On Tue, Jun 07, 2022 at 05:47:52PM +0200, Theo Buehler wrote:
> On Tue, Jun 07, 2022 at 05:09:30PM +0200, Claudio Jeker wrote:
> > The F_RTLABEL flag does nothing. So just remove it.
> > Checking the rtlabelid != 0 is equivalent.
> 
> Doesn't twiddling the F_RTLABEL potentially change the outcome of the two
> 
>   if (flags != oflags)
>   changed = 1;
> 
> bits in dispatch_rtmsg_addr()? If that's not a concern or if it's desired,
> then this is ok.

Nice find. "changes" is only used once:

if (kr->r.flags & F_NEXTHOP && changed)
knexthop_track(kt, kr);

So this triggers a check for nexthops that depend on this route.
rtlabels have no influence on this check so the side-effect so this
actually fixes some inefficency.

> Two minor nits below.

Ah, yes those checks. I have those fixed in a different tree. I should
probably remove them right now as well.
 
> > -- 
> > :wq Claudio
> > 
> > Index: bgpd.h
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> > retrieving revision 1.426
> > diff -u -p -r1.426 bgpd.h
> > --- bgpd.h  5 Jun 2022 12:43:13 -   1.426
> > +++ bgpd.h  7 Jun 2022 15:04:14 -
> > @@ -90,7 +90,6 @@
> >  #defineF_CTL_ADJ_IN0x2000  /* only set on requests */
> >  #defineF_CTL_ADJ_OUT   0x4000  /* only set on requests */
> >  #defineF_CTL_BEST  0x8000
> > -#defineF_RTLABEL   0x1
> >  #defineF_CTL_SSV   0x2 /* only used by bgpctl */
> >  #defineF_CTL_INVALID   0x4 /* only set on requests */
> >  #defineF_CTL_OVS_VALID 0x8
> > Index: kroute.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> > retrieving revision 1.248
> > diff -u -p -r1.248 kroute.c
> > --- kroute.c5 Jun 2022 12:43:13 -   1.248
> > +++ kroute.c7 Jun 2022 15:05:00 -
> > @@ -3268,7 +3268,6 @@ fetchtable(struct ktable *kt)
> > kr->r.labelid = 0;
> > if ((label = (struct sockaddr_rtlabel *)
> > rti_info[RTAX_LABEL]) != NULL) {
> > -   kr->r.flags |= F_RTLABEL;
> > kr->r.labelid =
> > rtlabel_name2id(label->sr_label);
> > }
> > @@ -3309,7 +3308,6 @@ fetchtable(struct ktable *kt)
> > kr6->r.labelid = 0;
> > if ((label = (struct sockaddr_rtlabel *)
> > rti_info[RTAX_LABEL]) != NULL) {
> > -   kr6->r.flags |= F_RTLABEL;
> > kr6->r.labelid =
> > rtlabel_name2id(label->sr_label);
> > }
> > @@ -3702,15 +3700,12 @@ dispatch_rtmsg_addr(struct rt_msghdr *rt
> > rtlabel_name2id(label->sr_label);
> > if (kr->r.labelid != new_labelid) {
> > rtlabel_unref(kr->r.labelid);
> > -   kr->r.labelid = 0;
> > -   flags |= F_RTLABEL;
> > kr->r.labelid = new_labelid;
> > rtlabel_changed = 1;
> > }
> > } else if (kr->r.labelid && label == NULL) {
> 
> The 'else' corresponds to an 'if (label != NULL)' check, so the
> '&& label == NULL' part could go without changing the logic.
> 
> > rtlabel_unref(kr->r.labelid);
> > kr->r.labelid = 0;
> > -   flags &= ~F_RTLABEL;
> > rtlabel_changed = 1;
> > }
> >  
> > @@ -3760,7 +3755,6 @@ add4:
> > kr->r.priority = prio;
> >  
> > if (label) {
> > -   kr->r.flags |= F_RTLABEL;
> > kr->r.labelid =
> > rtlabel_name2id(label->sr_label);
> > }
> > @@ -3809,14 +3803,12 @@ add4:
> > if (kr6->r.labelid != new_labelid) {
> > rtlabel_unref(kr6->r.labelid);
> > kr6->r.labelid = 0;
> > -   flags |= F_RTLABEL;
> > kr6->r.labelid = new_labelid;
> > rtlabel_changed = 1;
> > }
> > } else if (kr6->r.labelid && label == NULL) {
> 
> ditto re label == NULL.
> 
> >

Re: bgpd, retire F_RTLABEL flag

2022-06-07 Thread Theo Buehler

On Tue, Jun 07, 2022 at 05:09:30PM +0200, Claudio Jeker wrote:
> The F_RTLABEL flag does nothing. So just remove it.
> Checking the rtlabelid != 0 is equivalent.

Doesn't twiddling the F_RTLABEL potentially change the outcome of the two

if (flags != oflags)
changed = 1;

bits in dispatch_rtmsg_addr()? If that's not a concern or if it's desired,
then this is ok.

Two minor nits below.

> -- 
> :wq Claudio
> 
> Index: bgpd.h
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> retrieving revision 1.426
> diff -u -p -r1.426 bgpd.h
> --- bgpd.h5 Jun 2022 12:43:13 -   1.426
> +++ bgpd.h7 Jun 2022 15:04:14 -
> @@ -90,7 +90,6 @@
>  #define  F_CTL_ADJ_IN0x2000  /* only set on requests */
>  #define  F_CTL_ADJ_OUT   0x4000  /* only set on requests */
>  #define  F_CTL_BEST  0x8000
> -#define  F_RTLABEL   0x1
>  #define  F_CTL_SSV   0x2 /* only used by bgpctl */
>  #define  F_CTL_INVALID   0x4 /* only set on requests */
>  #define  F_CTL_OVS_VALID 0x8
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.248
> diff -u -p -r1.248 kroute.c
> --- kroute.c  5 Jun 2022 12:43:13 -   1.248
> +++ kroute.c  7 Jun 2022 15:05:00 -
> @@ -3268,7 +3268,6 @@ fetchtable(struct ktable *kt)
>   kr->r.labelid = 0;
>   if ((label = (struct sockaddr_rtlabel *)
>   rti_info[RTAX_LABEL]) != NULL) {
> - kr->r.flags |= F_RTLABEL;
>   kr->r.labelid =
>   rtlabel_name2id(label->sr_label);
>   }
> @@ -3309,7 +3308,6 @@ fetchtable(struct ktable *kt)
>   kr6->r.labelid = 0;
>   if ((label = (struct sockaddr_rtlabel *)
>   rti_info[RTAX_LABEL]) != NULL) {
> - kr6->r.flags |= F_RTLABEL;
>   kr6->r.labelid =
>   rtlabel_name2id(label->sr_label);
>   }
> @@ -3702,15 +3700,12 @@ dispatch_rtmsg_addr(struct rt_msghdr *rt
>   rtlabel_name2id(label->sr_label);
>   if (kr->r.labelid != new_labelid) {
>   rtlabel_unref(kr->r.labelid);
> - kr->r.labelid = 0;
> - flags |= F_RTLABEL;
>   kr->r.labelid = new_labelid;
>   rtlabel_changed = 1;
>   }
>   } else if (kr->r.labelid && label == NULL) {

The 'else' corresponds to an 'if (label != NULL)' check, so the
'&& label == NULL' part could go without changing the logic.

>   rtlabel_unref(kr->r.labelid);
>   kr->r.labelid = 0;
> - flags &= ~F_RTLABEL;
>   rtlabel_changed = 1;
>   }
>  
> @@ -3760,7 +3755,6 @@ add4:
>   kr->r.priority = prio;
>  
>   if (label) {
> - kr->r.flags |= F_RTLABEL;
>   kr->r.labelid =
>   rtlabel_name2id(label->sr_label);
>   }
> @@ -3809,14 +3803,12 @@ add4:
>   if (kr6->r.labelid != new_labelid) {
>   rtlabel_unref(kr6->r.labelid);
>   kr6->r.labelid = 0;
> - flags |= F_RTLABEL;
>   kr6->r.labelid = new_labelid;
>   rtlabel_changed = 1;
>   }
>   } else if (kr6->r.labelid && label == NULL) {

ditto re label == NULL.

>   rtlabel_unref(kr6->r.labelid);
>   kr6->r.labelid = 0;
> - flags &= ~F_RTLABEL;
>   rtlabel_changed = 1;
>   }
>  
> @@ -3870,7 +3862,6 @@ add6:
>   kr6->r.priority = prio;
>  
>   if (label) {
> - kr6->r.flags |= F_RTLABEL;
>   kr6->r.labelid =
>   rtlabel_name2id(label->sr_label);
>   }
>

bgpd, retire F_RTLABEL flag

2022-06-07 Thread Claudio Jeker

The F_RTLABEL flag does nothing. So just remove it.
Checking the rtlabelid != 0 is equivalent.

-- 
:wq Claudio

Index: bgpd.h
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
retrieving revision 1.426
diff -u -p -r1.426 bgpd.h
--- bgpd.h  5 Jun 2022 12:43:13 -   1.426
+++ bgpd.h  7 Jun 2022 15:04:14 -
@@ -90,7 +90,6 @@
 #defineF_CTL_ADJ_IN0x2000  /* only set on requests */
 #defineF_CTL_ADJ_OUT   0x4000  /* only set on requests */
 #defineF_CTL_BEST  0x8000
-#defineF_RTLABEL   0x1
 #defineF_CTL_SSV   0x2 /* only used by bgpctl */
 #defineF_CTL_INVALID   0x4 /* only set on requests */
 #defineF_CTL_OVS_VALID 0x8
Index: kroute.c
===
RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
retrieving revision 1.248
diff -u -p -r1.248 kroute.c
--- kroute.c5 Jun 2022 12:43:13 -   1.248
+++ kroute.c7 Jun 2022 15:05:00 -
@@ -3268,7 +3268,6 @@ fetchtable(struct ktable *kt)
kr->r.labelid = 0;
if ((label = (struct sockaddr_rtlabel *)
rti_info[RTAX_LABEL]) != NULL) {
-   kr->r.flags |= F_RTLABEL;
kr->r.labelid =
rtlabel_name2id(label->sr_label);
}
@@ -3309,7 +3308,6 @@ fetchtable(struct ktable *kt)
kr6->r.labelid = 0;
if ((label = (struct sockaddr_rtlabel *)
rti_info[RTAX_LABEL]) != NULL) {
-   kr6->r.flags |= F_RTLABEL;
kr6->r.labelid =
rtlabel_name2id(label->sr_label);
}
@@ -3702,15 +3700,12 @@ dispatch_rtmsg_addr(struct rt_msghdr *rt
rtlabel_name2id(label->sr_label);
if (kr->r.labelid != new_labelid) {
rtlabel_unref(kr->r.labelid);
-   kr->r.labelid = 0;
-   flags |= F_RTLABEL;
kr->r.labelid = new_labelid;
rtlabel_changed = 1;
}
} else if (kr->r.labelid && label == NULL) {
rtlabel_unref(kr->r.labelid);
kr->r.labelid = 0;
-   flags &= ~F_RTLABEL;
rtlabel_changed = 1;
}
 
@@ -3760,7 +3755,6 @@ add4:
kr->r.priority = prio;
 
if (label) {
-   kr->r.flags |= F_RTLABEL;
kr->r.labelid =
rtlabel_name2id(label->sr_label);
}
@@ -3809,14 +3803,12 @@ add4:
if (kr6->r.labelid != new_labelid) {
rtlabel_unref(kr6->r.labelid);
kr6->r.labelid = 0;
-   flags |= F_RTLABEL;
kr6->r.labelid = new_labelid;
rtlabel_changed = 1;
}
} else if (kr6->r.labelid && label == NULL) {
rtlabel_unref(kr6->r.labelid);
kr6->r.labelid = 0;
-   flags &= ~F_RTLABEL;
rtlabel_changed = 1;
}
 
@@ -3870,7 +3862,6 @@ add6:
kr6->r.priority = prio;
 
if (label) {
-   kr6->r.flags |= F_RTLABEL;
kr6->r.labelid =
rtlabel_name2id(label->sr_label);
}

Re: bgpd, retire F_RTLABEL flag

Re: bgpd, retire F_RTLABEL flag

bgpd, retire F_RTLABEL flag

3 matches

Site Navigation

Mail list logo

Footer information