Re: [Qemu-devel] [PATCH v2 5/6] net: defer nested call to BH

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, Jun 20, 2013 at 02:30:56PM +0800, liu ping fan wrote:
> On Tue, Jun 18, 2013 at 8:57 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Thu, Jun 13, 2013 at 05:03:05PM +0800, Liu Ping Fan wrote:
> >> From: Liu Ping Fan <pingfanl@linux.vnet.ibm.com>
> >>
> >> Nested call caused by ->receive() will raise issue like deadlock,
> >> so postphone it to BH.
> >>
> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >> ---
> >>  net/queue.c | 40 ++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 38 insertions(+), 2 deletions(-)
> >
> > Does this patch belong before the netqueue lock patch?  The commit
> > history should be bisectable without temporary failures/deadlocks.
> >
> Ok.
> >> diff --git a/net/queue.c b/net/queue.c
> >> index 58222b0..9c343ab 100644
> >> --- a/net/queue.c
> >> +++ b/net/queue.c
> >> @@ -24,6 +24,8 @@
> >>  #include "net/queue.h"
> >>  #include "qemu/queue.h"
> >>  #include "net/net.h"
> >> +#include "block/aio.h"
> >> +#include "qemu/main-loop.h"
> >>
> >>  /* The delivery handler may only return zero if it will call
> >>   * qemu_net_queue_flush() when it determines that it is once again able
> >> @@ -183,6 +185,22 @@ static ssize_t qemu_net_queue_deliver_iov(NetQueue *queue,
> >>      return ret;
> >>  }
> >>
> >> +typedef struct NetQueBH {
> >
> > This file uses "Queue" consistently, please don't add "Que" here.
> >
> >> @@ -192,8 +210,17 @@ ssize_t qemu_net_queue_send(NetQueue *queue,
> >>  {
> >>      ssize_t ret;
> >>
> >> -    if (queue->delivering || !qemu_can_send_packet_nolock(sender)) {
> >> +    if (queue->delivering || !qemu_can_send_packet_nolock(sender)
> >> +        || sender->send_queue->delivering) {
> >
> > Not sure this is safe, we're only holding one NetClientState->peer_lock
> > and one NetQueue->lock.  How can we access both queue->delivering and
> > sender->send_queue->delivering safely?
> 
> Yes, you are right, it is not safely. The queue->delivering is
> protected by peer_lock and we do not take the verse direction lock .
> So finally the above code can not tell out the nested calling
> "A-->B-->A"  from  "A-->B,  B-->A" (where A, B stands for a
> NetClientState).
> What about using TLS to trace the nested calling?  With it, we can
> avoid AB-BA lock problem.

I would take a step back and see if there's a way to avoid reaching into
inspect sender->send_queue->delivering here.

Stefan