Thanks. I'll push this series soon, let me know quickly if you want another look beforehand.
On Tue, May 08, 2012 at 05:07:35PM -0700, Ethan Jackson wrote: > 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
