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 <b...@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 <b...@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);
> > > > }
> >
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
