[RFC,v2,2/4] net: resolve race of tap backend and its peer

Submitted by pingfan liu on March 28, 2013, 7:55 a.m.

Details

Message ID 1364457355-4119-3-git-send-email-qemulist@gmail.com
State New
Headers show

Commit Message

pingfan liu March 28, 2013, 7:55 a.m.
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 net/tap.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

Comments

Stefan Hajnoczi March 28, 2013, 2:34 p.m.
On Thu, Mar 28, 2013 at 03:55:53PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  net/tap.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/net/tap.c b/net/tap.c
> index 0b663d1..401161c 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -321,9 +321,14 @@ static void tap_cleanup(NetClientState *nc)
>  static void tap_poll(NetClientState *nc, bool enable)
>  {
>      TAPState *s = DO_UPCAST(TAPState, nc, nc);
> -    /* fixme, when tap backend on another thread, the disable should be sync */
> +
>      tap_read_poll(s, enable);
>      tap_write_poll(s, enable);
> +
> +    if (!enable) {
> +        /* need sync so vhost can take over polling */
> +        g_source_remove_poll(&nc->nsrc->source, &nc->nsrc->gfd);
> +    }

Is this necessary?

Since we're in tap_poll() we are currently not in poll(2) waiting on the
fd.  Therefore it's safe for the caller to use the fd, the prepare
function will remove it from the fd set before glib calls poll(2) again.

Stefan
pingfan liu March 29, 2013, 7:21 a.m.
On Thu, Mar 28, 2013 at 10:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Mar 28, 2013 at 03:55:53PM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  net/tap.c |    7 ++++++-
>>  1 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 0b663d1..401161c 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -321,9 +321,14 @@ static void tap_cleanup(NetClientState *nc)
>>  static void tap_poll(NetClientState *nc, bool enable)
>>  {
>>      TAPState *s = DO_UPCAST(TAPState, nc, nc);
>> -    /* fixme, when tap backend on another thread, the disable should be sync */
>> +
>>      tap_read_poll(s, enable);
>>      tap_write_poll(s, enable);
>> +
>> +    if (!enable) {
>> +        /* need sync so vhost can take over polling */
>> +        g_source_remove_poll(&nc->nsrc->source, &nc->nsrc->gfd);
>> +    }
>
> Is this necessary?
>
> Since we're in tap_poll() we are currently not in poll(2) waiting on the
> fd.  Therefore it's safe for the caller to use the fd, the prepare
> function will remove it from the fd set before glib calls poll(2) again.
>
The main purpose is to make sure that the GSource->dispatch is not
in-flight, in which case, both vhost handler and ->dispatch() will
work on it.

Regards
Pingfan

> Stefan
Stefan Hajnoczi March 29, 2013, 2:38 p.m.
On Fri, Mar 29, 2013 at 8:21 AM, liu ping fan <qemulist@gmail.com> wrote:
> On Thu, Mar 28, 2013 at 10:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Thu, Mar 28, 2013 at 03:55:53PM +0800, Liu Ping Fan wrote:
>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>
>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>> ---
>>>  net/tap.c |    7 ++++++-
>>>  1 files changed, 6 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/net/tap.c b/net/tap.c
>>> index 0b663d1..401161c 100644
>>> --- a/net/tap.c
>>> +++ b/net/tap.c
>>> @@ -321,9 +321,14 @@ static void tap_cleanup(NetClientState *nc)
>>>  static void tap_poll(NetClientState *nc, bool enable)
>>>  {
>>>      TAPState *s = DO_UPCAST(TAPState, nc, nc);
>>> -    /* fixme, when tap backend on another thread, the disable should be sync */
>>> +
>>>      tap_read_poll(s, enable);
>>>      tap_write_poll(s, enable);
>>> +
>>> +    if (!enable) {
>>> +        /* need sync so vhost can take over polling */
>>> +        g_source_remove_poll(&nc->nsrc->source, &nc->nsrc->gfd);
>>> +    }
>>
>> Is this necessary?
>>
>> Since we're in tap_poll() we are currently not in poll(2) waiting on the
>> fd.  Therefore it's safe for the caller to use the fd, the prepare
>> function will remove it from the fd set before glib calls poll(2) again.
>>
> The main purpose is to make sure that the GSource->dispatch is not
> in-flight, in which case, both vhost handler and ->dispatch() will
> work on it.

Okay, I see.  hw/vhost_net.c will give the fd to the vhost kernel
module.  We need to be sure the fd is no longer in use - and we don't
want tap readable/writable handlers to be called.

Stefan

Patch hide | download patch | download mbox

diff --git a/net/tap.c b/net/tap.c
index 0b663d1..401161c 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -321,9 +321,14 @@  static void tap_cleanup(NetClientState *nc)
 static void tap_poll(NetClientState *nc, bool enable)
 {
     TAPState *s = DO_UPCAST(TAPState, nc, nc);
-    /* fixme, when tap backend on another thread, the disable should be sync */
+
     tap_read_poll(s, enable);
     tap_write_poll(s, enable);
+
+    if (!enable) {
+        /* need sync so vhost can take over polling */
+        g_source_remove_poll(&nc->nsrc->source, &nc->nsrc->gfd);
+    }
 }
 
 int tap_get_fd(NetClientState *nc)