Sorry for such a delayed review of this patch. I have bit more time now that multithreading is in, so let's try to get this merged this week.
Is this v4 the most recent version? Since I was so delayed reviewing it, it no longer applies to master. There's one major problem with this patch which will require some re-architecture. So let's get that cleared up before I dig to deeply into the review. In this version of the patch, there are several places where you set bfd->min_rx directly. This is very much not allowed. If you set bfd->min_rx without giving the remote BFD session a change to learn of the new configuration, you'll cause tunnel flapping. Imagine if min_rx is 1 second and you suddenly set it to 100ms. There's going to be a lot of 100ms intervals with no received BFD packets before the remote session updates. I think as a high level architecture we should do the following. 1) Get rid of the second half of bfd_decay that attempts to set the min_rx. Instead if we need to either enter or exit decay mode, simply do a bfd_poll(). 2) In bfd_poll change this line: + bfd->poll_min_rx = bfd->min_rx == bfd->decay_min_rx + ? bfd->decay_min_rx : bfd->cfg_min_rx; To something like: bfd->poll_min_rx = in_decay_mode ? bfd->decay_min-rx : bfd->cfg_min_rx 3) change the configuration code to simply call a bfd_poll() if the decay_min_rx changes instead of handling it directly. Does that all make sense? I'm 12 hours off so if you could send out a new version I'll have a look at it in the morning. Thanks, Ethan X-CudaMail-Whitelist-To: dev@openvswitch.org _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
