Message ID | CAPWQB7Ga4e7vUHiOqJ-PeZ_owdm9=QZRe9721mYxw35W1515EA@mail.gmail.com |
---|---|
State | Not Applicable |
Headers | show |
The reasoning and the updated patch seem right to me, so: Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Mar 6, 2017, at 12:16 PM, Joe Stringer <joe@ovn.org> wrote: > > On 20 February 2017 at 04:27, Mika Väisänen <mika.vaisanen@gmail.com> wrote: >> Interworking of BFD and RSTP does not work, as currently BFD messages are >> dropped if RSTP port is not in forwarding mode. To correct this problem, >> an extra check is added to allow BFD messages to be sent even when >> rstp_forward_state is false. >> >> Note: This patch is made against branch-2.5. >> >> Signed-off-by: Mika Vaisanen <mika.vaisanen@gmail.com> >> >> --- > > There's something a bit off about the formatting of the patch, but > it's simple enough that I can just make the equivalent change locally. > The test seems to show the expected behaviour well. > > I see now, looking at the codepaths, the bfd packet generation link > through to the compose_output_action() goes like this: > > monitor_mport_run() > ofproto_dpif_send_packet() > xlate_send_packet() > ofproto_dpif_execute_actions() > ofproto_dpif_execute_actions__() > xlate_actions() > do_xlate_actions() > xlate_output_action() > compose_output_action() > > I didn't realise that when these various protocols (STP, RSTP, CFM, > BFD, etc.) send packets from OVS, it goes through the standard > translation like this. I guess it makes sense :-) > > The only concern I had about this patch is if there was a way to end > up broadcasting BFD in a loop because BFD is skipping past the > STP/RSTP forwarding checks. However, I believe that on receive, > xlate_actions() will handle BFD via process_special(), so typically it > should not end up in the path where it's attempting to forward the > packet. If it somehow does get past there, the check that is being > added by this patch will ensure that the BFD packet matches the > settings for this link, or otherwise it will respect the STP/RSTP > forwarding state. So I think it's fine. I wouldn't mind bouncing it > off someone like Jarno just to double-check my reasoning. > > As a matter of style, I think this diff in ofproto-dpif-xlate.c is a > little easier to follow---and would cover the CFM case as well: > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 89fc3a44a0d1..578fef168b30 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3127,6 +3127,10 @@ compose_output_action__(struct xlate_ctx *ctx, > ofp_port_t ofp_port, > } > return; > } > + } else if ((xport->cfm && cfm_should_process_flow(xport->cfm, > flow, wc)) > + || (xport->bfd && bfd_should_process_flow(xport->bfd, flow, > + wc))) { > + /* Pass; STP should not block link health detection. */ > } else if (!xport_stp_forward_state(xport) || > !xport_rstp_forward_state(xport)) { > if (ctx->xbridge->stp != NULL) {
On 7 March 2017 at 10:22, Jarno Rajahalme <jarno@ovn.org> wrote: > The reasoning and the updated patch seem right to me, so: > > Acked-by: Jarno Rajahalme <jarno@ovn.org> Thanks for taking a look. I plan to come up with a CFM testcase as well and fold that in before I push this.
On 7 March 2017 at 10:23, Joe Stringer <joe@ovn.org> wrote: > On 7 March 2017 at 10:22, Jarno Rajahalme <jarno@ovn.org> wrote: >> The reasoning and the updated patch seem right to me, so: >> >> Acked-by: Jarno Rajahalme <jarno@ovn.org> > > Thanks for taking a look. I plan to come up with a CFM testcase as > well and fold that in before I push this. I applied it to master and branches 2.5-2.7.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 89fc3a44a0d1..578fef168b30 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3127,6 +3127,10 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, } return; } + } else if ((xport->cfm && cfm_should_process_flow(xport->cfm, flow, wc)) + || (xport->bfd && bfd_should_process_flow(xport->bfd, flow, + wc))) { + /* Pass; STP should not block link health detection. */ } else if (!xport_stp_forward_state(xport) ||