Re: [PATCH V2 11/20] tap: support enabling or disabling a queue

[Jason Wang <jasowang@redhat.com>]

On 01/30/2013 07:03 AM, Michael S. Tsirkin wrote:
> On Tue, Jan 29, 2013 at 04:55:25PM -0600, Anthony Liguori wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>
>>> On Tue, Jan 29, 2013 at 08:10:26PM +0000, Blue Swirl wrote:
>>>> On Tue, Jan 29, 2013 at 1:50 PM, Jason Wang <jasowang@redhat.com> wrote:
>>>>> On 01/26/2013 03:13 AM, Blue Swirl wrote:
>>>>>> On Fri, Jan 25, 2013 at 10:35 AM, Jason Wang <jasowang@redhat.com> wrote:
>>>>>>> This patch introduce a new bit - enabled in TAPState which tracks whether a
>>>>>>> specific queue/fd is enabled. The tap/fd is enabled during initialization and
>>>>>>> could be enabled/disabled by tap_enalbe() and tap_disable() which calls platform
>>>>>>> specific helpers to do the real work. Polling of a tap fd can only done when
>>>>>>> the tap was enabled.
>>>>>>>
>>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>>> ---
>>>>>>>  include/net/tap.h |    2 ++
>>>>>>>  net/tap-win32.c   |   10 ++++++++++
>>>>>>>  net/tap.c         |   43 ++++++++++++++++++++++++++++++++++++++++---
>>>>>>>  3 files changed, 52 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/net/tap.h b/include/net/tap.h
>>>>>>> index bb7efb5..0caf8c4 100644
>>>>>>> --- a/include/net/tap.h
>>>>>>> +++ b/include/net/tap.h
>>>>>>> @@ -35,6 +35,8 @@ int tap_has_vnet_hdr_len(NetClientState *nc, int len);
>>>>>>>  void tap_using_vnet_hdr(NetClientState *nc, int using_vnet_hdr);
>>>>>>>  void tap_set_offload(NetClientState *nc, int csum, int tso4, int tso6, int ecn, int ufo);
>>>>>>>  void tap_set_vnet_hdr_len(NetClientState *nc, int len);
>>>>>>> +int tap_enable(NetClientState *nc);
>>>>>>> +int tap_disable(NetClientState *nc);
>>>>>>>
>>>>>>>  int tap_get_fd(NetClientState *nc);
>>>>>>>
>>>>>>> diff --git a/net/tap-win32.c b/net/tap-win32.c
>>>>>>> index 265369c..a2cd94b 100644
>>>>>>> --- a/net/tap-win32.c
>>>>>>> +++ b/net/tap-win32.c
>>>>>>> @@ -764,3 +764,13 @@ void tap_set_vnet_hdr_len(NetClientState *nc, int len)
>>>>>>>  {
>>>>>>>      assert(0);
>>>>>>>  }
>>>>>>> +
>>>>>>> +int tap_enable(NetClientState *nc)
>>>>>>> +{
>>>>>>> +    assert(0);
>>>>>> abort()
>>>>> This is just to be consistent with the reset of the helpers in this file.
>>>>>>> +}
>>>>>>> +
>>>>>>> +int tap_disable(NetClientState *nc)
>>>>>>> +{
>>>>>>> +    assert(0);
>>>>>>> +}
>>>>>>> diff --git a/net/tap.c b/net/tap.c
>>>>>>> index 67080f1..95e557b 100644
>>>>>>> --- a/net/tap.c
>>>>>>> +++ b/net/tap.c
>>>>>>> @@ -59,6 +59,7 @@ typedef struct TAPState {
>>>>>>>      unsigned int write_poll : 1;
>>>>>>>      unsigned int using_vnet_hdr : 1;
>>>>>>>      unsigned int has_ufo: 1;
>>>>>>> +    unsigned int enabled : 1;
>>>>>> bool without bit field?
>>>>> Also to be consistent with other field. If you wish I can send patches
>>>>> to convert all those bit field to bool on top of this series.
>>>> That would be nice, likewise for the assert(0).
>>> OK so let's go ahead with this patchset as is,
>>> and a cleanup patch will be send after 1.4 then.
>> Why?  I'd prefer that we didn't rush things into 1.4 just because.
>> There's still ample time to respin a corrected series.
>>
>> Regards,
>>
>> Anthony Liguori
> Confused.  Do you want the coding style rework of net/tap.c
> switching it from assert(0)/bitfields to abort()/bool for 1.4?

I will send a new series with the patches that addresses Blue's comments
on assert(0) and bitfields.

Thanks
>>>
>>>>> Thanks
>>>>>>>      VHostNetState *vhost_net;
>>>>>>>      unsigned host_vnet_hdr_len;
>>>>>>>  } TAPState;
>>>>>>> @@ -72,9 +73,9 @@ static void tap_writable(void *opaque);
>>>>>>>  static void tap_update_fd_handler(TAPState *s)
>>>>>>>  {
>>>>>>>      qemu_set_fd_handler2(s->fd,
>>>>>>> -                         s->read_poll  ? tap_can_send : NULL,
>>>>>>> -                         s->read_poll  ? tap_send     : NULL,
>>>>>>> -                         s->write_poll ? tap_writable : NULL,
>>>>>>> +                         s->read_poll && s->enabled ? tap_can_send : NULL,
>>>>>>> +                         s->read_poll && s->enabled ? tap_send     : NULL,
>>>>>>> +                         s->write_poll && s->enabled ? tap_writable : NULL,
>>>>>>>                           s);
>>>>>>>  }
>>>>>>>
>>>>>>> @@ -339,6 +340,7 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
>>>>>>>      s->host_vnet_hdr_len = vnet_hdr ? sizeof(struct virtio_net_hdr) : 0;
>>>>>>>      s->using_vnet_hdr = 0;
>>>>>>>      s->has_ufo = tap_probe_has_ufo(s->fd);
>>>>>>> +    s->enabled = 1;
>>>>>>>      tap_set_offload(&s->nc, 0, 0, 0, 0, 0);
>>>>>>>      /*
>>>>>>>       * Make sure host header length is set correctly in tap:
>>>>>>> @@ -737,3 +739,38 @@ VHostNetState *tap_get_vhost_net(NetClientState *nc)
>>>>>>>      assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_TAP);
>>>>>>>      return s->vhost_net;
>>>>>>>  }
>>>>>>> +
>>>>>>> +int tap_enable(NetClientState *nc)
>>>>>>> +{
>>>>>>> +    TAPState *s = DO_UPCAST(TAPState, nc, nc);
>>>>>>> +    int ret;
>>>>>>> +
>>>>>>> +    if (s->enabled) {
>>>>>>> +        return 0;
>>>>>>> +    } else {
>>>>>>> +        ret = tap_fd_enable(s->fd);
>>>>>>> +        if (ret == 0) {
>>>>>>> +            s->enabled = 1;
>>>>>>> +            tap_update_fd_handler(s);
>>>>>>> +        }
>>>>>>> +        return ret;
>>>>>>> +    }
>>>>>>> +}
>>>>>>> +
>>>>>>> +int tap_disable(NetClientState *nc)
>>>>>>> +{
>>>>>>> +    TAPState *s = DO_UPCAST(TAPState, nc, nc);
>>>>>>> +    int ret;
>>>>>>> +
>>>>>>> +    if (s->enabled == 0) {
>>>>>>> +        return 0;
>>>>>>> +    } else {
>>>>>>> +        ret = tap_fd_disable(s->fd);
>>>>>>> +        if (ret == 0) {
>>>>>>> +            qemu_purge_queued_packets(nc);
>>>>>>> +            s->enabled = 0;
>>>>>>> +            tap_update_fd_handler(s);
>>>>>>> +        }
>>>>>>> +        return ret;
>>>>>>> +    }
>>>>>>> +}
>>>>>>> --
>>>>>>> 1.7.1
>>>>>>>
>>>>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html