Looks good, thanks. Ethan
On Sat, May 5, 2012 at 11:10 AM, Ben Pfaff <[email protected]> wrote: > Until now, packets for these special protocols have been mixed with general > traffic in the kernel-to-userspace queues. This means that a big-enough > storm of new flows in these queues can cause packets for these special > protocols to be dropped at this interface, fooling userspace into believing > that, say, no CFM packets have been received even though they are arriving > at the expected rate. > > This commit moves special protocols to a dedicated kernel-to-userspace > queue to avoid the problem. > > Bug #7550. > Signed-off-by: Ben Pfaff <[email protected]> > --- > lib/dpif-linux.c | 11 ++++++++--- > lib/dpif-provider.h | 4 ++++ > lib/dpif.c | 3 +++ > ofproto/ofproto-dpif.c | 19 +++++++++++++++++-- > 4 files changed, 32 insertions(+), 5 deletions(-) > > diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c > index 3b2fba3..256c9d6 100644 > --- a/lib/dpif-linux.c > +++ b/lib/dpif-linux.c > @@ -61,8 +61,9 @@ > VLOG_DEFINE_THIS_MODULE(dpif_linux); > enum { MAX_PORTS = USHRT_MAX }; > > -enum { N_UPCALL_SOCKS = 16 }; > -BUILD_ASSERT_DECL(IS_POW2(N_UPCALL_SOCKS)); > +enum { N_UPCALL_SOCKS = 17 }; > +BUILD_ASSERT_DECL(IS_POW2(N_UPCALL_SOCKS - 1)); > +BUILD_ASSERT_DECL(N_UPCALL_SOCKS > 1); > BUILD_ASSERT_DECL(N_UPCALL_SOCKS <= 32); /* We use a 32-bit word as a mask. > */ > > /* This ethtool flag was introduced in Linux 2.6.24, so it might be > @@ -460,7 +461,11 @@ dpif_linux_port_get_pid(const struct dpif *dpif_, > uint16_t port_no) > if (dpif->epoll_fd < 0) { > return 0; > } else { > - int idx = port_no & (N_UPCALL_SOCKS - 1); > + int idx; > + > + idx = (port_no != UINT16_MAX > + ? 1 + (port_no & (N_UPCALL_SOCKS - 2)) > + : 0); > return nl_sock_pid(dpif->upcall_socks[idx]); > } > } > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > index 594ce59..0641d30 100644 > --- a/lib/dpif-provider.h > +++ b/lib/dpif-provider.h > @@ -136,6 +136,10 @@ struct dpif_class { > * actions as the OVS_USERSPACE_ATTR_PID attribute's value, for use in > * flows whose packets arrived on port 'port_no'. > * > + * A 'port_no' of UINT16_MAX should be treated as a special case. The > + * implementation should return a reserved PID, not allocated to any > port, > + * that the client may use for special purposes. > + * > * The return value only needs to be meaningful when DPIF_UC_ACTION has > * been enabled in the 'dpif''s listen mask, and it is allowed to change > * when DPIF_UC_ACTION is disabled and then re-enabled. > diff --git a/lib/dpif.c b/lib/dpif.c > index 4d7e8b3..ce7c580 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -541,6 +541,9 @@ dpif_get_max_ports(const struct dpif *dpif) > * as the OVS_USERSPACE_ATTR_PID attribute's value, for use in flows whose > * packets arrived on port 'port_no'. > * > + * A 'port_no' of UINT16_MAX is a special case: it returns a reserved PID, > not > + * allocated to any port, that the client may use for special purposes. > + * > * The return value is only meaningful when DPIF_UC_ACTION has been enabled in > * the 'dpif''s listen mask. It is allowed to change when DPIF_UC_ACTION is > * disabled and then re-enabled, so a client that does that must be prepared > to > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 486630c..97d3426 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -3474,7 +3474,12 @@ expire_batch(struct ofproto_dpif *ofproto, struct > subfacet **subfacets, int n) > static void > expire_subfacets(struct ofproto_dpif *ofproto, int dp_max_idle) > { > - long long int cutoff = time_msec() - dp_max_idle; > + /* Cutoff time for most flows. */ > + long long int normal_cutoff = time_msec() - dp_max_idle; > + > + /* We really want to keep flows for special protocols around, so use a > more > + * conservative cutoff. */ > + long long int special_cutoff = time_msec() - 10000; > > struct subfacet *subfacet, *next_subfacet; > struct subfacet *batch[EXPIRE_MAX_BATCH]; > @@ -3483,6 +3488,11 @@ expire_subfacets(struct ofproto_dpif *ofproto, int > dp_max_idle) > n_batch = 0; > HMAP_FOR_EACH_SAFE (subfacet, next_subfacet, hmap_node, > &ofproto->subfacets) { > + long long int cutoff; > + > + cutoff = (subfacet->slow & (SLOW_CFM | SLOW_LACP | SLOW_STP) > + ? special_cutoff > + : normal_cutoff); > if (subfacet->used < cutoff) { > if (subfacet->path != SF_NOT_INSTALLED) { > batch[n_batch++] = subfacet; > @@ -4719,7 +4729,12 @@ compose_slow_path(const struct ofproto_dpif *ofproto, > const struct flow *flow, > cookie.slow_path.reason = slow; > > ofpbuf_use_stack(&buf, stub, stub_size); > - put_userspace_action(ofproto, &buf, flow, &cookie); > + if (slow & (SLOW_CFM | SLOW_LACP | SLOW_STP)) { > + uint32_t pid = dpif_port_get_pid(ofproto->dpif, UINT16_MAX); > + odp_put_userspace_action(pid, &cookie, &buf); > + } else { > + put_userspace_action(ofproto, &buf, flow, &cookie); > + } > *actionsp = buf.data; > *actions_lenp = buf.size; > } > -- > 1.7.2.5 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
