On Aug 28, 2013 11:54 AM, "Jarno Rajahalme" <jrajaha...@nicira.com> wrote:
>
>
> On Aug 28, 2013, at 9:39 AM, Jarno Rajahalme <jrajaha...@nicira.com>
wrote:
> >> One specific race that I was concerned about is:
> >>
> >> 1. This code in udpif_miss_handler() checks n_upcalls and sees that it
> >>  is zero.
> >>
> >>       if (!handler->n_upcalls) {
> >>
> >> 2. This code in recv_upcalls() signals wake_cond:
> >>
> >>   for (n = 0; n < udpif->n_handlers; ++n) {
> >>       handler = &udpif->handlers[n];
> >>       if (handler->n_new_upcalls) {
> >>           handler->n_new_upcalls = 0;
> >>           xpthread_cond_signal(&handler->wake_cond);
> >>       }
> >>   }
> >>
> >> 3. This code in udpif_miss_handler() starts waiting on wake_cond:
> >>
> >>           ovs_mutex_cond_wait(&handler->wake_cond, &handler->mutex);
> >>
> >> Maybe this race cannot happen, because n_upcalls only changes with the
> >> mutex taken.  I guess that's the case.
> >
>
> On a second thought, I think it is better to be conservative here and
lock while cond_signaling. That way there will be no nasty surprises if
some of the other code gets reorganized.

That way requires less careful thought so I support it in the absence of
evidence that there is a performance benefit to avoiding the lock.

Will you post a version that always takes the lock then?

Thanks,

Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to