The reasoning and the updated patch seem right to me, so: Acked-by: Jarno Rajahalme <[email protected]>
> On Mar 6, 2017, at 12:16 PM, Joe Stringer <[email protected]> wrote: > > On 20 February 2017 at 04:27, Mika Väisänen <[email protected]> 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 <[email protected]> >> >> --- > > 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) { _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
