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

[Andy Zhou]

I was thinking of the first case. You are right, it is covered by
find_nonempty_queue().

Then this patch looks good to me.


On Mon, Aug 12, 2013 at 1:06 PM, Ben Pfaff <blp at nicira.com> wrote:

> To be specific, I guess we are talking about
> dp_netdev_output_userspace() calling seq_change() and
> dpif_netdev_recv_wait() calling seq_wait().  As you say, they are
> serialized by dp_netdev_mutex, so there are two possible orders:
>
>     - If dp_netdev_output_userspace() runs first, then queue_seq does
>       not matter at all, because find_nonempty_queue() will return
>       true and poll_immediate_wake() will be called.
>
>     - If dpif_netdev_recv_wait() runs first, then it will read some
>       sequence number N and pass that to seq_wait().
>       dp_netdev_output_userspace() will then increment the sequence
>       number, which (barring a bug inside the seq module) will prevent
>       the subsequent poll_block() from sleeping.
>
> On Mon, Aug 12, 2013 at 09:31:36AM -0700, Andy Zhou wrote:
> > Here is an example I can think of:
> >
> > Thread A wants to do a seq_change(), Thread B wants to do seq_wait(),
> they
> > both compete for dp_netdev_mutex at the same time.
> >
> > Assume Thread A wins, seq_change changed the seq->value to +1 and
> releases
> > the dp_netdev_mutex. Thread B now waits for the +1 value to change, thus
> > missing Thread A's +1 event.
> >
> > Is this possible?
> >
> >
> >
> > On Sat, Aug 10, 2013 at 8:57 PM, Ben Pfaff <blp at nicira.com> wrote:
> >
> > > 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);
> > > > >  }
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130812/5d9080ef/attachment-0003.html>