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 <ho...@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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
