[ovs-dev] [PATCH] ofproto-dpif: Allow setting of flow eviction threshold
Simon Horman
horms at verge.net.au
Thu Jul 28 01:57:51 UTC 2011
On Wed, Jul 27, 2011 at 04:41:21PM -0700, Ben Pfaff wrote:
> On Wed, Jul 27, 2011 at 05:39:05PM +0900, Simon Horman wrote:
> > Allow setting the number of flows present in the flow hash
> > at which point eviction of entries from the kernel flow hash
> > will begin to occur.
> >
> > The value may be set using a bridge's other-config column.
> >
> > e.g.
> >
> > ovs-vsctl set bridge br3 other-config:flow-eviction-threshold=10000
> >
> > default is 1000, reflecting constant value previously used.
> >
> > Increasing this value can result in reduced CPU usage and
> > packet loss in situations where the number of active flows
> > is significantly larger than 1000.
>
> Thanks for passing this along. It looks pretty good. I have a few
> comments.
>
> The struct ofproto member could use a comment. This affects only the
> ofproto-dpif implementation, so it would be worth mentioning that. (If
> we get any more ofproto-dpif only features then we'll want to move them
> out of the generic structure.)
Sure.
> I don't see anything that initializes the threshold in the absence of a
> call to ofproto_set_flow_eviction_threshold().
Yes, that is true. My assumption was that it would always be called.
> I don't see anything that validates that a threshold that is set makes
> sense. I think that it would be appropriate for facet_max_idle() to
> force a minimum of at least, say, 100.
Sure. At this point the minimum is 0 due to the use
of a signed type. I'll change the code to round up
any value less than 100 to 100, which seems like
a reasonable if arbitrary minimum value to me.
> There's a typo in vswitch.xml: s/fromt he/from the/.
Thanks, will fix.
> The commit log does a good job of documenting this option, but I'd
> recommend also mentioning in vswitch.xml the default, the minimum value,
> the reason why one might want to change the default. It's probably also
> worth mentioning that this affects only the software-based switch.
Sure, will do.
More information about the dev
mailing list