On 21 May 2014 14:25, Andy Zhou <az...@nicira.com> wrote: > On Tue, May 20, 2014 at 5:27 PM, Joe Stringer <joestrin...@nicira.com> > wrote: > > 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? > Would this lead to writer to be locked out for a long time? >
Yes. I don't think it's a viable choice for this reason.
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
