<div dir="ltr">Hi,<br><div class="gmail_extra"><br><div class="gmail_quote">2016-02-16 18:48 GMT+01:00 Ondrej Zajicek <span dir="ltr"><<a href="mailto:santiago@crfreenet.org" target="_blank">santiago@crfreenet.org</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>On Wed, Feb 10, 2016 at 04:38:03PM +0100, Mikhail Sennikovskii wrote:<br>
> Hello Fellows,<br>
><br>
> as a follow-up for the recent discussion about BIRD support of IPv6 ECMP on Linux<br>
> ( <a href="http://trubka.network.cz/pipermail/bird-users/2016-January/010163.html" rel="noreferrer" target="_blank">http://trubka.network.cz/pipermail/bird-users/2016-January/010163.html</a> ),<br>
> I've created a patch, which addresses this problem.<br>
<br>
</span>Hi<br>
<br>
Thanks for the patch, i have several comments:<br>
<br>
1) In the patch, the incoming direction (collection from kernel) is<br>
handled in unix/krt.c, while outgoing direction (krt_replace_rte()) is<br>
handled in linux/netlink.c . It would make more sense to handle both<br>
directions in the same place. As BIRD uses internally representation<br>
with one route and mpnh structure, i would prefer to move collection<br>
also to linux/netlink.c, so generic code in unix/krt.c would not<br>
care about OS-specific interface (and does not need any changes).<br>
Linux code would just collect next hops and then call krt_got_route()<br>
for the whole multipath route.<br></blockquote><div>Agreed, will fix. <br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
2) Function krt_replace_rte() should be smarter than just flushing the<br>
whole old multipath rte. It should compare old and new next hops (we<br>
could suppose they are sorted) and either add missing</blockquote><div>I'm not sure for now whether it's possible to make this work,<br></div><div>as netlink returns EEXIST whenever one tries to add a route for the network,<br>already present in the routing table.<br></div><div>This is why removing and re-installing seemed the best solution for me.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> or remove<br>
surplussing next hops by calling nl_send_route().<br></blockquote><div>Just removing should work, yes.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
3) When collecting received routes, you should avoid creating list<br>
of routes for the same net, and then merging such list by rt_merge_list(),<br>
you could simply collect next hops (struct mpnh *) for the current route.<br></blockquote><div>I can do it this way.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
4) Also rt_merge_list() uses mpnh_merge_rta() which uses rte_update_pool.<br>
That is probably not safe outside of nest update code. The linux netlink<br>
code should have its own linpool (static and shared, because netlink socket<br>
is also shared), so you could allocate mpnh structures from it. The linpool<br>
should be flushed when the route is collected and propagated to krt_got_route().<br>
So you don't need to do rta_lookup() for temporary next hops.<br></blockquote><div>Right, will do.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Note that such linpool could be also used for IPv4 multipath next hops in<br>
nl_parse_multipath() instead of nh_buffer.<br></blockquote><div>I can add this fix as well, probably as a separate patch.<br><span></span><br><span></span><span></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>> Note that the patch still does not support the learn mode for IPv6 ECMP.<br>
> The support for it can be added later.<br>
<br>
</span>Well, the learn mode is not really a problem. With the modifications<br>
mentioned above, learning would work automatically (it is handled by<br>
unix/krt.c, regardless of how route is generated).<br>
<br>
What is problematic is handling asynchronous notifications (because you<br>
don't know inside netlink code what are other next hops and whether you<br>
will get another one related to the same net).<br></blockquote><div>Sorry, I guess I just mixed up with bird terminology.<br></div><div>I thought the learn mode is directly related to handling these asynchronous notifications<br></div><div>and adjusting the bird routes set accordingly.<br><br></div><div>Regards,<br></div><div>Mikhail<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span><font color="#888888"><br>
<br>
--<br>
Elen sila lumenn' omentielvo<br>
<br>
Ondrej 'Santiago' Zajicek (email: <a href="mailto:santiago@crfreenet.org" target="_blank">santiago@crfreenet.org</a>)<br>
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, <a href="http://wwwkeys.pgp.net" rel="noreferrer" target="_blank">wwwkeys.pgp.net</a>)<br>
"To err is human -- to blame it on a computer is even more so."<br>
</font></span></blockquote></div><br></div></div>