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

Jarno Rajahalme jrajahalme at nicira.com
Fri Dec 20 23:02:38 UTC 2013


Alex,

Thanks, I just pushed this,

  Jarno

On Dec 20, 2013, at 12:47 PM, Alex Wang <alexw at nicira.com> wrote:

> 
> Thanks Jarno for the review, please see my explanation below,
> 
> 
> On Fri, Dec 20, 2013 at 11:29 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> 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?
> 
> Yes, you are right.
> 
> Without the change, this logic works fine, since we do not re-heapify anywhere else.
> 
> Under the current change, the old behavior was a bug.
> 
>  
> > +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?
> 
> 
> Yeah, thanks for mentioning this.  I actually haven't noticed this.
> 
> Yes, that is a very cool!  I'll send separate patch for it.
> 
>  
> > +    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?
> 
> 
> Exactly, this is for the case using patch port.  And in that case, the monitor thread (already acquired the wrlock) will call ofproto_dpif_send_packet() to send pkt while holding the "monitor->wrlock".
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20131220/e11154a7/attachment-0003.html>


More information about the dev mailing list