[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