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 <stefa...@gmail.com> wrote:
> > On Thu, Jun 13, 2013 at 05:03:05PM +0800, Liu Ping Fan wrote:
> >> From: Liu Ping Fan <pingf...@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 <pingf...@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
