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

login
register
mail settings
Submitter pingfan liu
Date March 28, 2013, 7:55 a.m.
Message ID <1364457355-4119-3-git-send-email-qemulist@gmail.com>
Download mbox | patch
Permalink /patch/231924/
State New
Headers show

Comments

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(-)
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

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)