Mailing List Archive: PATCH: static nexthop calculation enhancement

PATCH: static nexthop calculation enhancement

Oct 26, 2003, 11:24 AM

Post #1 of 4 (838 views)

Hi,

The following patch addresses an irritating deficiency with static route
install / uninstall into the kernel: assume your zebra is configured
with some static route, say 192.68.1.0/24 via 192.69.1.1, installed into
the kernel/FIB. Then, add another nexthop to the same destination via
some other gateway, be it active or inactive. What happens in this case
is that the former route is uninstalled from the kernel, then the new
route (re-calculated with the newly added nexthop) is installed back
into it.

However, absurdly enough, this procedure of uninstall/reinstall happens
even if:

#1, the newly added nexthop is inactive, that is, has no matching
connected route to make it resolve into a reachable neighbor: in this
case there's clearly no need to reinstall anything, since nothing has
changed.

#2, the newly added nexthop is active, but the MULTIPATH_NUM limit is
exceeded: in this case, again, nothing needs to be reinstalled, since
the nexthop elements are added by chronological order into the nexthop
list, so the older ones are still the first to become active.

Note, that the same deficiency holds for nexthop removal, namely:

#1, even when an inactive nexthop is removed, the route is reinstalled.

#2, even when an active yet non-FIB nexthop is removed, the route is
reinstalled.

I figured this is worth fixing. Please note that:

- I believe this doesn't harm other behaviors, but I'd appreciate a
decent patch review (Paul?)

- sounds silly, but I didn't compile quagga with the new patch... (I
still don't have my automake tools updated, sorry ;-> ) I'll
appreciate it if someone verifies it passes. Thanks.

- I did test this fix in limited scenario, but it was applied to another
revision of the code that I'm running, so I'll appreciate it if someone
can verify that it does what it's aimed to do and doesn't screw up other
things. Thanks again. ;->

Also, this patch overrides a potential hazard in
zebra/zebra_rib.c/nexthop_active_check(), by which the returned value
may not be 0 or 1 (it's the result of CHECK_FLAG macro), but happens to
be so only because the checked flag NEXTHOP_FLAG_ACTIVE equals (1 << 0).
Otherwise, if the flag values are changed sometime in the future,
rib->nexthop_active_num in nexthop_active_update() may be evaluated to a
meaningless value.

Any comments are welcome!

(PS: same problem is present in zebra, AFAICT)

Thanks,
Gilad

Re: PATCH: static nexthop calculation enhancement [ In reply to ]

paul at clubi

Oct 27, 2003, 6:37 PM

Post #2 of 4 (824 views)

Permalink

On Sun, 26 Oct 2003, Gilad Arnold wrote:

> Hi,
>
> The following patch addresses an irritating deficiency with static
> route install / uninstall into the kernel: assume your zebra is
> configured with some static route, say 192.68.1.0/24 via
> 192.69.1.1, installed into the kernel/FIB. Then, add another
> nexthop to the same destination via some other gateway, be it
> active or inactive. What happens in this case is that the former
> route is uninstalled from the kernel, then the new route
> (re-calculated with the newly added nexthop) is installed back into
> it.

Aha.. Is this why static routes often 'interfere' with dynamic (eg
OSPF) routes, ie prevent the installation of?

> However, absurdly enough, this procedure of uninstall/reinstall happens
> even if:
>
> #1, the newly added nexthop is inactive, that is, has no matching
> connected route to make it resolve into a reachable neighbor: in
> this case there's clearly no need to reinstall anything, since
> nothing has changed.
>
> #2, the newly added nexthop is active, but the MULTIPATH_NUM limit is
> exceeded: in this case, again, nothing needs to be reinstalled, since
> the nexthop elements are added by chronological order into the nexthop
> list, so the older ones are still the first to become active.
>
> Note, that the same deficiency holds for nexthop removal, namely:
>
> #1, even when an inactive nexthop is removed, the route is reinstalled.
>
> #2, even when an active yet non-FIB nexthop is removed, the route is
> reinstalled.
>
> I figured this is worth fixing. Please note that:

absolutely.

> - I believe this doesn't harm other behaviors, but I'd appreciate a
> decent patch review (Paul?)

Seems to address the issues you describe.

> - sounds silly, but I didn't compile quagga with the new patch...
> (I still don't have my automake tools updated, sorry ;-> ) I'll
> appreciate it if someone verifies it passes. Thanks.

Compiles. NB: the CVS snapshots do not require auto* tools to be
installed, the configure script and makefiles are already built. If
you track CVS you should be able to take the configure script and
makefiles from the snapshot and use them with your CVS working copy.
(until one day you see 'M configure.ac' :) )

> - I did test this fix in limited scenario, but it was applied to
> another revision of the code that I'm running, so I'll appreciate
> it if someone can verify that it does what it's aimed to do and
> doesn't screw up other things. Thanks again. ;->

:)

> Also, this patch overrides a potential hazard in
> zebra/zebra_rib.c/nexthop_active_check(), by which the returned
> value may not be 0 or 1 (it's the result of CHECK_FLAG macro), but
> happens to be so only because the checked flag NEXTHOP_FLAG_ACTIVE
> equals (1 << 0). Otherwise, if the flag values are changed
> sometime in the future, rib->nexthop_active_num in
> nexthop_active_update() may be evaluated to a meaningless value.

Hmmm....

#define CHECK_FLAG(V,F) ((V) & (F))

Surely though, the intent of the macro is that it will evaluate to
either 0 or !0? (ie not '0 or 1'). Further, AFAICT, the only caller
of nexthop_active_check() doesnt even bother to check the return
value, never mind checking that it returns 1 or NEXTHOP_FLAG_ACTIVE -
at least this is what you've changed it to and hence no longer
presumes /anything/ of the return value. :)

The original was a bit borked though, doing a += based on return
value of nexthop_active_check() - which seems very strange, bit of a
mismatch between the caller expecting, presumably, a return value ==
number of active next hops (?) but the implementation actually
returning a boolean value to indicate whether any were active or not
(0 / !0). Weird.

> Any comments are welcome!
>
> (PS: same problem is present in zebra, AFAICT)
>
> Thanks,
> Gilad

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
Your mode of life will be changed to ASCII.

Re: PATCH: static nexthop calculation enhancement [ In reply to ]

gilad.arnold at terayon

Oct 28, 2003, 9:52 AM

Post #3 of 4 (825 views)

Permalink

Hi again,

I must say that I was really bothered thinking over this patch after I
issued it, for one main reason: it uses MULTIPATH_NUM value explicitly
in zebra/zebra_rib.c, while (a) so far this value has only been used in
kernel layers, so using it explicitly seems like an abstraction
violation; and (b) I'm not even sure it is defined for cases of kernel
layers other than netlink.

Hence, I'll first try and verify that this usage is proper and valid
under the context I've applied it, otherwise I'll try to provide some
nicer mean to get the max-nexthop value, using a decent extension to the
kernel API layer (something like kernel_multipath_num() call, which will
return MULTIPATH_NUM for rt_netlink.c and 1 (?) for other layers).

(the above is something more of a general warning, I didn't yet get to
take a proper look, I'll do that tomorrow hopefully and send an update
to the patch -- meanwhile I wouldn't apply it to the repository)

Now, to your comments...

Paul Jakma wrote:

> Aha.. Is this why static routes often 'interfere' with dynamic (eg
> OSPF) routes, ie prevent the installation of?

No, I don't think so. It only relates to the re-evaluation of a changed
static route.

...
> Compiles. NB: the CVS snapshots do not require auto* tools to be
> installed, the configure script and makefiles are already built. If
> you track CVS you should be able to take the configure script and
> makefiles from the snapshot and use them with your CVS working copy.
> (until one day you see 'M configure.ac' :) )

I know, I know, didn't get to do that one either... ;->

Expect further updates.
Gilad

Re: PATCH: static nexthop calculation enhancement [ In reply to ]

paul at clubi

Oct 28, 2003, 10:53 AM

Post #4 of 4 (822 views)

Permalink

On Tue, 28 Oct 2003, Gilad Arnold wrote:

> Hi again,
>
> I must say that I was really bothered thinking over this patch
> after I issued it, for one main reason: it uses MULTIPATH_NUM value
> explicitly in zebra/zebra_rib.c, while (a) so far this value has
> only been used in kernel layers, so using it explicitly seems like
> an abstraction violation; and (b) I'm not even sure it is defined
> for cases of kernel layers other than netlink.

ah, good point.

> Hence, I'll first try and verify that this usage is proper and
> valid under the context I've applied it, otherwise I'll try to
> provide some nicer mean to get the max-nexthop value, using a
> decent extension to the kernel API layer (something like
> kernel_multipath_num() call, which will return MULTIPATH_NUM for
> rt_netlink.c and 1 (?) for other layers).

thats an idea.

> (the above is something more of a general warning, I didn't yet get
> to take a proper look, I'll do that tomorrow hopefully and send an
> update to the patch -- meanwhile I wouldn't apply it to the
> repository)

Well, 'tis applied now, but hey, kernel_multipath_num() shouldnt be
hard to add to the backends.

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
The trouble with doing something right the first time is that nobody
appreciates how difficult it was.

Mailing List Archive

Mailing List Archive

Attached Files: