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

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

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

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

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