[ovs-dev] [PATCH 5/5 v2] dpif-netdev: Avoid races on queue and port changes using seq objects.

[Ben Pfaff]

On Sat, Aug 10, 2013 at 04:39:09PM -0700, Andy Zhou wrote:
> On Wed, Aug 7, 2013 at 1:31 PM, Ben Pfaff <blp at nicira.com> wrote:
> > dpif_upcall *upcall,
> >  static void
> >  dpif_netdev_recv_wait(struct dpif *dpif)
> >  {
> > -    /* XXX In a multithreaded process, there is a race window between this
> > -     * function and the poll_block() in one thread and a packet being
> > queued in
> > -     * another thread. */
> > +    struct dp_netdev *dp = get_dp_netdev(dpif);
> > +    uint64_t seq;
> >
> >      ovs_mutex_lock(&dp_netdev_mutex);
> > +    seq = seq_read(dp->queue_seq);
> >
> 
> Is there a risk of lossing events in case seq_change() is executed in
> another thread while this thread is waiting
> on the dp_netdev_mutex lock?

I guess by "losing events" you mean sleeping too long, beyond the time
at which an upcall is queued?  I don't think we can lose events here,
then.  There are two cases.  The first case, where we call
poll_immediate_wake(), is obviously OK.  In the other case, we know that
the queues were empty at a sequence number of 'seq' or later, so calling
seq_wait() passing 'seq' will ensure that we wake up when the sequence
number becomes greater than 'seq', which will definitely happen whenever
an upcall gets queued.

Do you see a hole in the logic?

Thanks,

Ben.

> >      if (find_nonempty_queue(dpif)) {
> >          poll_immediate_wake();
> > +    } else {
> > +        seq_wait(dp->queue_seq, seq);
> >      }
> >      ovs_mutex_unlock(&dp_netdev_mutex);
> >  }