[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