Thanks Jarno for the review, please see my explanation below,
On Fri, Dec 20, 2013 at 11:29 AM, Jarno Rajahalme <jrajaha...@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".
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
