On Thu, Feb 27, 2014 at 04:55:49PM -0800, Joe Stringer wrote: > On 27 February 2014 15:23, Ben Pfaff <b...@nicira.com> wrote: > > Datapath flow dumps can sometimes repeat or omit flows, but > > revalidate() appears to create a new ukey if it doesn't find one > > without making sure that a duplicate wasn't added in the meantime. > > > > That said, I guess that the common case is that a given ukey is being > > accessed by only a single thread at a time. Is that true? If so, did > > you consider locking it once in revalidate() as soon as we find it, > > and then releasing it when we go on to the next flow? (I guess that > > we only need to deal with a single ukey once per dump, so we could > > possibly even use trylock at the beginning and skip on to the next > > flow if some other thread is already working on that one?) > > I would agree that this race is unusual, but quite possible. > > The simplest case to consider here is if two threads attempt to perform the > same ukey lookup at the same time: > * Thread A locks the revalidator, searches for (and fails to find) the > ukey. Unlocks and continues revalidate(). > * Thread B does the same > * Thread A begins creating the ukey, locks revalidator, inserts ukey, > unlocks > * Thread B will then attempt to do the same. > > I've got two possible fixes in mind: > 1) Lock revalidator->mutex before the lookup. Only release after insertion > (or the equivalent codepath if we are not creating a ukey). Thread B is > unable to process the duplicate flow until Thread A has set up the ukey and > inserted it. However, Thread B has no idea that it is handling a duplicate. > 2) Perform an additional lookup immediately before the hmap insertion > (while holding revalidator->mutex) to make sure we don't double-insert. If > it exists, then we're in this race condition and can skip processing the > current flow. > > I'm leaning towards #2, although it is still possible to have the same flow > processed twice (for example, if the ukey exists, or if the ukey exists but > the flow is ready to expire). Thoughts?
I also think #2 is fine. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
