On 21 May 2014 10:33, Joe Stringer <joestrin...@nicira.com> wrote: > On 21 May 2014 10:05, Ryan Wilson 76511 <wr...@vmware.com> wrote: > > Hey Andy, >> >> >At a high level, ofgroup struct current has rwlock that essentially >> >solving >> >the same problem as the ref count proposed in this patch. It would be >> >better >> >it seems confusing if we use both method together. >> > >> >Looking at the code, I'd think extending rwlock to cover xlate >> >fastpath is the most >> >straight forward approach. >> >> The reason I use the reference count is because of the following >> situation: >> - The main thread and xlate cache both have pointers to the group. >> - The main thread deletes the group and frees memory. >> - However, the xlate cache still has a pointer to the group and if a >> handler is run prior to the revalidator clearing the xlate cache, a >> handler could write to freed memory. >> > > Strictly speaking, the xlate_cache is only used inside revalidators at the > moment. However, this doesn't contradict your point. We need to guarantee > that the xlate_cache won't try to write to the group stats after the main > thread frees it. And this is not guaranteed by the group rwlock. As you > say, for other types of stats, this is guaranteed with refcounting. >
Andy, I looked again at struct ofgroup and I see the comment now :-) I'll describe my understanding of the possible problem: * ofproto lock ensures that the adding/deletion/lookup of ofgroups is consistent across multiple threads. * ofgroup->rwlock can ensure that reading/writing of ofgroup contents is consistent across multiple threads. * We can currently guarantee existence of ofgroup because we always lookup (grabbing both locks), read/write, release. As with current group access, when building up the xlate_cache, we grab the ofproto lock to lookup the ofgroup. We grab the ofgroup lock to ensure it isn't freed. However, for xlate_cache we want to store a pointer to the ofgroup for a longer period. As soon as we release the lock, there is no guarantee that the ofgroup still exists. The refcount proposed in this patch allows the pointer to be held longer-term and guaranteed to still be available. The alternative would be to hold a readlock on the rwlock for the lifetime of the xlate_cache, which would prevent group deletion. Does that make sense?
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
