[ovs-dev] [PATCH V2 2/3] bfd: Send FINAL immediately after receiving POLL.

Jarno Rajahalme jrajahalme at nicira.com
Fri Dec 20 19:29:35 UTC 2013


Alex,

I’m not familiar with the code, so I’ll ask you to explain some things below,

  Jarno

On Dec 16, 2013, at 9:30 AM, Alex Wang <alexw at nicira.com> wrote:

> Commit 307464a11 (ofproto-dpif-monitor: Use heap to order the mport
> wakeup time.) makes bfd only send packet at specified periodic instant.
> This fails to meet the RFC5880 requirement, which requires bfd send
> FINAL immediately after receiving POLL.
> 
> This commit fixes the above issue by scheduling bfd to send FINAL
> within 100 ms after receiving POLL.
> 
> Signed-off-by: Alex Wang <alexw at nicira.com>
> ---
> 
> v1 -> v2:
> - rebase master.
> 
> ---
> ofproto/ofproto-dpif-monitor.c |   29 +++++++++++++++++++++++++++--
> ofproto/ofproto-dpif-monitor.h |    3 +++
> ofproto/ofproto-dpif-xlate.c   |    9 +++++++++
> 3 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-monitor.c b/ofproto/ofproto-dpif-monitor.c
> index af66387..f67f036 100644
> --- a/ofproto/ofproto-dpif-monitor.c
> +++ b/ofproto/ofproto-dpif-monitor.c
> @@ -225,8 +225,9 @@ monitor_run(void)
>             bfd_wait(mport->bfd);
>         }
>         /* Computes the next wakeup time for this mport. */
> -        next_wake_time = MIN(bfd_wake_time(mport->bfd), cfm_wake_time(mport->cfm));
> -        heap_change(&monitor_heap, heap_max(&monitor_heap),
> +        next_wake_time = MIN(bfd_wake_time(mport->bfd),
> +                             cfm_wake_time(mport->cfm));
> +        heap_change(&monitor_heap, &mport->heap_node,
>                     MSEC_TO_PRIO(next_wake_time));

So instead of changing the existing max node in the heap (the topmost), now we change the current mport’s node, as the comment says. I guess the old behavior was a bug, as it could have changed some other mport’s priority?

>     }
> 
> @@ -275,3 +276,27 @@ ofproto_dpif_monitor_port_update(const struct ofport_dpif *ofport,
>         monitor_running = false;
>     }
> }
> +
> +/* Moves the mport on top of the heap.  This is necessary when
> + * for example, bfd POLL is received and the mport should
> + * immediately send FINAL back. */
> +void
> +ofproto_dpif_monitor_port_send_soon_safe(const struct ofport_dpif *ofport)
> +{
> +    ovs_rwlock_wrlock(&monitor_rwlock);
> +    ofproto_dpif_monitor_port_send_soon(ofport);
> +    ovs_rwlock_unlock(&monitor_rwlock);
> +}
> +
> +void
> +ofproto_dpif_monitor_port_send_soon(const struct ofport_dpif *ofport)
> +    OVS_REQ_WRLOCK(monitor_rwlock)
> +{
> +    struct mport *mport;
> +
> +    monitor_init();

This is in no way critical for this patch, but a remark for later: I guess we could change to use the OVS_CONSTRUCTOR to make sure monitor_init is called?

> +    mport = mport_find(ofport);
> +    if (mport) {
> +        heap_change(&monitor_heap, &mport->heap_node, LLONG_MAX);
> +    }
> +}
> diff --git a/ofproto/ofproto-dpif-monitor.h b/ofproto/ofproto-dpif-monitor.h
> index f914fbe..1f6be5c 100644
> --- a/ofproto/ofproto-dpif-monitor.h
> +++ b/ofproto/ofproto-dpif-monitor.h
> @@ -23,6 +23,9 @@ struct bfd;
> struct cfm;
> struct ofport_dpif;
> 
> +void ofproto_dpif_monitor_port_send_soon(const struct ofport_dpif *);
> +void ofproto_dpif_monitor_port_send_soon_safe(const struct ofport_dpif *);
> +
> void ofproto_dpif_monitor_port_update(const struct ofport_dpif *,
>                                       struct bfd *, struct cfm *,
>                                       uint8_t[OFP_ETH_ALEN]);
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 56d007c..15edd5e 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -42,6 +42,7 @@
> #include "ofp-actions.h"
> #include "ofproto/ofproto-dpif-ipfix.h"
> #include "ofproto/ofproto-dpif-mirror.h"
> +#include "ofproto/ofproto-dpif-monitor.h"
> #include "ofproto/ofproto-dpif-sflow.h"
> #include "ofproto/ofproto-dpif.h"
> #include "ofproto/ofproto-provider.h"
> @@ -1645,6 +1646,14 @@ process_special(struct xlate_ctx *ctx, const struct flow *flow,
>     } else if (xport->bfd && bfd_should_process_flow(xport->bfd, flow, wc)) {
>         if (packet) {
>             bfd_process_packet(xport->bfd, flow, packet);
> +            /* If POLL received, immediately sends FINAL back. */
> +            if (bfd_should_send_packet(xport->bfd)) {
> +                if (xport->peer) {
> +                    ofproto_dpif_monitor_port_send_soon(xport->ofport);
> +                } else {
> +                    ofproto_dpif_monitor_port_send_soon_safe(xport->ofport);
> +                }
> +            }

Can you briefly explain the rationale here? Do we already have the lock if (sport->peer), and not otherwise?

>         }
>         return SLOW_BFD;
>     } else if (xport->xbundle && xport->xbundle->lacp
> -- 
> 1.7.9.5
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list