[ovs-dev] [PATCH] ofproto-dpif-upcall: Batch upcalls.

[Ethan Jackson]

I'm happy with it as long as we fix the race.

Ethan

On Tue, Aug 27, 2013 at 11:02 PM, Ben Pfaff <blp at 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 <blp at 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 <jrajahalme at 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 <blp at nicira.com>