[ovs-dev] [PATCH] ofproto: Optimise OpenFlow flow expiry

[Ben Pfaff]

On Fri, Jan 25, 2013 at 01:46:15PM +0900, Simon Horman wrote:
> On Wed, Jan 23, 2013 at 09:48:17AM -0800, Ben Pfaff wrote:
> > On Thu, Jan 17, 2013 at 09:42:29AM +0900, Simon Horman wrote:
> > > On Wed, Jan 16, 2013 at 01:59:21PM -0800, Ben Pfaff wrote:
> > > > On Tue, Jan 15, 2013 at 01:20:57PM +0900, Simon Horman wrote:
> > > > > Optimise OpenFlow flow expiry by placing expirable flows on a list.
> > > > > This optimises scanning of flows for expiry in two ways:
> > > > > 
> > > > > * Empirically list traversal appears faster than the code it replaces.
> > > > > 
> > > > >   With 1,000,000 flows present an otherwise idle system I observed CPU
> > > > >   utilisation of around 20% with the existing code but around 10% with
> > > > >   this new code.
> > > > > 
> > > > > * Flows that will never expire are not traversed.
> > > > > 
> > > > >   This addresses a case seen in the field.
> > > > 
> > > > This version looks better.  I still have a few comments, but before
> > > > that, may I ask a little bit about the situation in which the
> > > > performance improvement was observed?  In this situation, about how
> > > > many of the 1,000,000 flows were actually expirable, that is, had
> > > > either a hard timeout or an idle timeout?  That is, is the performance
> > > > improvement due more to the first or the second bullet you list above?
> > > > If none of the flows were expirable, then I guess it was the second;
> > > > if all of them were, then I guess it was the first; and otherwise it
> > > > is something in between.
> > > > 
> > > > Basically I'm wondering if we should do something to make flow table
> > > > traversal faster, independent of expiration.
> > > 
> > > Hi Ben,
> > > 
> > > the primary aim of this patch was to address a performance issue that
> > > was noticed when inserting 100,000 flows none of which were expirable.
> > > I have been told this is representative of an expected use-case.
> > > 
> > > During my testing I used 1,000,000 flows instead of 100,000 in order to
> > > make the CPU utilisation more pronounced and easier to observe.
> > > 
> > > In the course of my testing I tested 1,000,000 flows none of which were
> > > expirable and in that case CPU utilisation was dramatically reduced to
> > > approximately 0. This seems to be a good outcome for the use-case
> > > originally reported.
> > > 
> > > In the course of testing I also tested 1,000,000 flows all of which
> > > were expirable. This was primarily to see if there were any regressions.
> > > In the course of this test I noticed that there seemed to be some
> > > reduction in CPU utilisation. However this was just a side effect and
> > > not an aim of my work. I should have placed it as the second bullet
> > > in my commit message and noted that it was a side effect.
> > 
> > OK, I made a few stylistic and comment changes to the patch, and I've
> > updated the commit message to reflect what you said above.  I'm happy
> > with it now, but I don't want to apply it without your approval since I
> > changed the meaning of your commit message.  Please review?
> 
> Looks good. Please feel free to apply.

Done.