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 <al...@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 <al...@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
