I'm happy with it as long as we fix the race.
Ethan
On Tue, Aug 27, 2013 at 11:02 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Tue, Aug 27, 2013 at 03:50:06PM -0700, Jarno Rajahalme wrote:
>>
>> On Aug 27, 2013, at 3:13 PM, Ben Pfaff <b...@nicira.com> wrote:
>>
>> > On Tue, Aug 27, 2013 at 03:04:54PM -0700, Jarno Rajahalme wrote:
>> >> Batching reduces overheads and enables upto 4 times the upcall processing
>> >> performance in a specialized test case.
>> >>
>> >> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>> >
>> > Nice!
>> >
>> >> + 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);
>> >> + }
>> >
>> > Usually there's a race if one signals a condition variable without
>> > holding the corresponding mutex. Did you check that there is no race
>> > here?
>>
>> I did not notice anything hanging or the like, if that's what you
>> mean. Documentation says:
>
> 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.
>
> Ethan, unless you see something, I'm happy with this. (I'll be at
> VMworld tomorrow and probably not continuing the conversation.)
>
> Acked-by: Ben Pfaff <b...@nicira.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
