DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Lazaros Koromilas <l@nofutznetworks.com>
Cc: Olivier Matz <olivier.matz@6wind.com>,
	dev@dpdk.org, Thomas Monjalon <thomas.monjalon@6wind.com>
Subject: Re: [dpdk-dev] [PATCH v2] ring: check for zero objects mc dequeue / mp enqueue
Date: Tue, 29 Mar 2016 09:54:43 +0100	[thread overview]
Message-ID: <20160329085443.GA17800@bricha3-MOBL3> (raw)
In-Reply-To: <CAHPNE8jmWLQAjZV71ryhH01SnJbGA=ZuZD+z3vR-hWUjEbpd+Q@mail.gmail.com>

On Mon, Mar 28, 2016 at 06:48:07PM +0300, Lazaros Koromilas wrote:
> Hi Olivier,
> 
> We could have two threads (running on different cores in the general
> case) that both succeed the cmpset operation. In the dequeue path,
> when n == 0, then cons_next == cons_head, and cmpset will always
> succeed. Now, if they both see an old r->cons.tail value from a
> previous dequeue, they can get stuck in the while

Hi,

I don't see how threads reading an "old r->cons.tail" value is even possible.
The head and tail pointers on the ring are marked in the code as volatile, so
all reads and writes to those values are always done from memory and not cached
in registers. No deadlock should be possible on that while loop, unless a 
process crashes in the middle of a ring operation. Each thread which updates
the head pointer from x to y, is responsible for updating the tail pointer in
a similar manner. The loop ensures the tail updates are in the same order as the
head updates.

If you believe deadlock is possible, can you outline the sequence of operations
which would lead to such a state, because I cannot see how it could occur without
a crash inside one of the threads.

/Bruce

> (unlikely(r->cons.tail != cons_head)) loop. I tried, however, to
> reproduce (without the patch) and it seems that there is still a
> window for deadlock.
> 
> I'm pasting some debug output below that shows two processes' state.
> It's two receivers doing interleaved mc_dequeue(32)/mc_dequeue(0), and
> one sender doing mp_enqueue(32) on the same ring.
> 
> gdb --args ./ring-test -l0 --proc-type=primary
> gdb --args ./ring-test -l1 --proc-type=secondary
> gdb --args ./ring-test -l2 --proc-type=secondary -- tx
> 
> This is what I would usually see, process 0 and 1 both stuck at the same state:
> 
> 663             while (unlikely(r->cons.tail != cons_head)) {
> (gdb) p n
> $1 = 0
> (gdb) p r->cons.tail
> $2 = 576416
> (gdb) p cons_head
> $3 = 576448
> (gdb) p cons_next
> $4 = 576448
> 
> But now I managed to get the two processes stuck at this state too.
> 
> process 0:
> 663             while (unlikely(r->cons.tail != cons_head)) {
> (gdb) p n
> $1 = 32
> (gdb) p r->cons.tail
> $2 = 254348832
> (gdb) p cons_head
> $3 = 254348864
> (gdb) p cons_next
> $4 = 254348896
> 
> proccess 1:
> 663             while (unlikely(r->cons.tail != cons_head)) {
> (gdb) p n
> $1 = 32
> (gdb) p r->cons.tail
> $2 = 254348832
> (gdb) p cons_head
> $3 = 254348896
> (gdb) p cons_next
> $4 = 254348928
> 

Where is the thread which updated the head pointer from 832 to 864? That thread
was the one which would update the tail pointer to 864 to allow your thread 0
to continue.

/Bruce

> I haven't been able to trigger this with the patch so far, but it
> should be possible.
> 
> Lazaros.
> 
> On Fri, Mar 25, 2016 at 1:15 PM, Olivier Matz <olivier.matz@6wind.com> wrote:
> > Hi Lazaros,
> >
> > On 03/17/2016 04:49 PM, Lazaros Koromilas wrote:
> >> Issuing a zero objects dequeue with a single consumer has no effect.
> >> Doing so with multiple consumers, can get more than one thread to succeed
> >> the compare-and-set operation and observe starvation or even deadlock in
> >> the while loop that checks for preceding dequeues.  The problematic piece
> >> of code when n = 0:
> >>
> >>     cons_next = cons_head + n;
> >>     success = rte_atomic32_cmpset(&r->cons.head, cons_head, cons_next);
> >>
> >> The same is possible on the enqueue path.
> >
> > Just a question about this patch (that has been applied). Thomas
> > retitled the commit from your log message:
> >
> >   ring: fix deadlock in zero object multi enqueue or dequeue
> >   http://dpdk.org/browse/dpdk/commit/?id=d0979646166e
> >
> > I think this patch does not fix a deadlock, or did I miss something?
> >
> > As explained in the following links, the ring may not perform well
> > if several threads running on the same cpu use it:
> >
> >   http://dpdk.org/ml/archives/dev/2013-November/000714.html
> >   http://www.dpdk.org/ml/archives/dev/2014-January/001070.html
> >   http://www.dpdk.org/ml/archives/dev/2014-January/001162.html
> >   http://dpdk.org/ml/archives/dev/2015-July/020659.html
> >
> > A deadlock could occur if the threads running on the same core
> > have different priority.
> >
> > Regards,
> > Olivier

  reply	other threads:[~2016-03-29  8:54 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-17 15:49 Lazaros Koromilas
2016-03-17 16:09 ` Mauricio Vásquez
2016-03-18 10:18   ` Bruce Richardson
2016-03-18 10:27     ` Olivier Matz
2016-03-18 10:35       ` Bruce Richardson
2016-03-18 10:35       ` Thomas Monjalon
2016-03-18 12:47         ` Mauricio Vásquez
2016-03-18 14:16           ` Bruce Richardson
2016-03-21 17:47             ` Xie, Huawei
2016-03-22 10:13               ` Bruce Richardson
2016-03-22 14:38                 ` Xie, Huawei
2016-03-21 12:23 ` Olivier Matz
2016-03-22 16:49   ` Thomas Monjalon
2016-03-25 11:15 ` Olivier Matz
2016-03-28 15:48   ` Lazaros Koromilas
2016-03-29  8:54     ` Bruce Richardson [this message]
2016-03-29 15:29       ` Olivier MATZ
2016-03-29 16:04         ` Bruce Richardson
2016-03-29 17:35           ` Lazaros Koromilas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160329085443.GA17800@bricha3-MOBL3 \
    --to=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=l@nofutznetworks.com \
    --cc=olivier.matz@6wind.com \
    --cc=thomas.monjalon@6wind.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).