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

[liu ping fan]

On Tue, Jun 18, 2013 at 8:57 PM, Stefan Hajnoczi <address@hidden> wrote:
> On Thu, Jun 13, 2013 at 05:03:05PM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <address@hidden>
>>
>> Nested call caused by ->receive() will raise issue like deadlock,
>> so postphone it to BH.
>>
>> Signed-off-by: Liu Ping Fan <address@hidden>
>> ---
>>  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.

Thx & regards,
Pingfan