[ovs-dev] [PATCH] Implement partial stats update
blp at nicira.com
Mon Jan 30 11:00:30 PST 2012
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
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
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;", 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?
More information about the dev