[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openvswitch-dev
Subject:    [ovs-dev] [PATCH] Implement partial stats update
From:       blp () nicira ! com (Ben Pfaff)
Date:       2012-01-30 19:00:30
Message-ID: 20120130190030.GB6219 () nicira ! com
[Download RAW message or body]

On Wed, Jan 25, 2012 at 02:51:34PM +0900, Simon Horman wrote:
> As the number of flows grows so does the length of time taken to update the
> statistics of all facets. It has been obvserved that this becomes a performance
> problem. This patch mitiages this effect by only dumping a limited number of
> facets at a time.
> 
> Signed-off-by: Simon Horman <horms at verge.net.au>

Thank you for doing this work.

This is a much smaller change than I expected.  (I'm happy about
that.)

update_stats() uses local static variables to track the flow dump and
to determine whether to begin a new flow dump.  This can only work
properly when there is just one bridge.  Presumably these need to be
members of struct ofproto_dpif.

Something needs to destroy the flow dump, if one is active, in
destruct().

I think that the list management is slightly wrong.  In
subfacet_destroy__(), the call to list_remove() is going to corrupt
memory if the subfacet is not currently on a list.  The list_remove()
in expire_subfacets() definitely looks like a use-after-free error.

I'd consider changing the list management so that, instead of keeping
a list inside struct ofproto_dpif, instead the list is a variable
local to expire().  After all, the list is only important during a
given expire() call.  Furthermore, it seems pointless to ever remove
anything from the list, since we build up a completely new list on the
next call to expire() anyway.

List usage is simple enough, actually, that we could just use a
fixed-size local array of pointers to subfacets within expire(), say
"struct subfacet *expirable_subfacets[20000];", which wouldn't require
adding a new field to struct subfacet, either.

I was surprised to see that subfacet_max_idle() considers not just the
subfacets that we are expiring in this round but in fact all the
subfacets total.  But a comment in subfacet_max_idle() reminded me
that I think this patch re-introduces a bug that we had some time
back: with this patch, I think that uninstallable subfacets can never
expire, because they never show up in the flow dump and therefore
never get onto the expirable_subfacets list.  We will need to fix that
somehow before this change makes sense.  Do you have an idea?

Thanks,

Ben.


[prev in list] [next in list] [prev in thread] [next in thread]