[ovs-dev] ofproto-dpif-xlate: Allow sending BFD messages even when RSTP port is not forwarding

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:

                    !xport_rstp_forward_state(xport)) {
             if (ctx->xbridge->stp != NULL) {

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.