[Qemu-devel] [PATCH v2] net:Enable vhost with vhostforce, vhost options for guests without MSI-X support

[Qemu-devel] [PATCH v2] net:Enable vhost with vhostforce, vhost options for guests without MSI-X support

[Pankaj Gupta]

    We use vhostforce to enable vhost even if Guests don't have MSI-X support
and we fall back to QEMU virtio-net. This patch will enable vhost unconditionally 
whenever we have vhostforce='ON' or vhost='ON'. 

Initially, I wanted to remove vhostforce completely as an additional argument. 
But after discussing this in mailing list found that some programs are using vhostforce
and some vhost. So, we want to keep semantics of both the options.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---
 net/tap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index d1ca314..4618359 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -649,13 +649,13 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
         }
     }
 
-    if (tap->has_vhost ? tap->vhost :
-        vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
+    if ((tap->has_vhost ? tap->vhost :
+        vhostfdname) || tap->vhostforce) {
         VhostNetOptions options;
 
         options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
         options.net_backend = &s->nc;
-        options.force = tap->has_vhostforce && tap->vhostforce;
+        options.force = true;
 
         if (tap->has_vhostfd || tap->has_vhostfds) {
             vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2] net:Enable vhost with vhostforce, vhost options for guests without MSI-X support

[Jason Wang]

On 06/05/2015 10:32 PM, Pankaj Gupta wrote:
>     We use vhostforce to enable vhost even if Guests don't have MSI-X support
> and we fall back to QEMU virtio-net. This patch will enable vhost unconditionally 
> whenever we have vhostforce='ON' or vhost='ON'. 
>
> Initially, I wanted to remove vhostforce completely as an additional argument. 
> But after discussing this in mailing list found that some programs are using vhostforce
> and some vhost. So, we want to keep semantics of both the options.
>
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  net/tap.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index d1ca314..4618359 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -649,13 +649,13 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>          }
>      }
>  
> -    if (tap->has_vhost ? tap->vhost :
> -        vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
> +    if ((tap->has_vhost ? tap->vhost :
> +        vhostfdname) || tap->vhostforce) {
>          VhostNetOptions options;
>  
>          options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
>          options.net_backend = &s->nc;
> -        options.force = tap->has_vhostforce && tap->vhostforce;
> +        options.force = true;
>  
>          if (tap->has_vhostfd || tap->has_vhostfds) {
>              vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);

In this case, I believe there's no need to have vhost_net_query() and
query_guest_notifiers() callbacks (and maybe more others).

Re: [Qemu-devel] [PATCH v2] net:Enable vhost with vhostforce, vhost options for guests without MSI-X support

[Pankaj Gupta]

> 
> On 06/05/2015 10:32 PM, Pankaj Gupta wrote:
> >     We use vhostforce to enable vhost even if Guests don't have MSI-X
> >     support
> > and we fall back to QEMU virtio-net. This patch will enable vhost
> > unconditionally
> > whenever we have vhostforce='ON' or vhost='ON'.
> >
> > Initially, I wanted to remove vhostforce completely as an additional
> > argument.
> > But after discussing this in mailing list found that some programs are
> > using vhostforce
> > and some vhost. So, we want to keep semantics of both the options.
> >
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> >  net/tap.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/tap.c b/net/tap.c
> > index d1ca314..4618359 100644
> > --- a/net/tap.c
> > +++ b/net/tap.c
> > @@ -649,13 +649,13 @@ static void net_init_tap_one(const NetdevTapOptions
> > *tap, NetClientState *peer,
> >          }
> >      }
> >  
> > -    if (tap->has_vhost ? tap->vhost :
> > -        vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
> > +    if ((tap->has_vhost ? tap->vhost :
> > +        vhostfdname) || tap->vhostforce) {
> >          VhostNetOptions options;
> >  
> >          options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
> >          options.net_backend = &s->nc;
> > -        options.force = tap->has_vhostforce && tap->vhostforce;
> > +        options.force = true;
> >  
> >          if (tap->has_vhostfd || tap->has_vhostfds) {
> >              vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
> 
> In this case, I believe there's no need to have vhost_net_query() and
> query_guest_notifiers() callbacks (and maybe more others).

I also thought on this. If same functions can be used by some other module in future?
If not, I was thinking to remove those in another patch.

Does the main functionality looks OK? 

> 

Re: [Qemu-devel] [PATCH v2] net:Enable vhost with vhostforce, vhost options for guests without MSI-X support

[Jason Wang]

On 06/11/2015 07:49 PM, Pankaj Gupta wrote:
>> On 06/05/2015 10:32 PM, Pankaj Gupta wrote:
>>>     We use vhostforce to enable vhost even if Guests don't have MSI-X
>>>     support
>>> and we fall back to QEMU virtio-net. This patch will enable vhost
>>> unconditionally
>>> whenever we have vhostforce='ON' or vhost='ON'.
>>>
>>> Initially, I wanted to remove vhostforce completely as an additional
>>> argument.
>>> But after discussing this in mailing list found that some programs are
>>> using vhostforce
>>> and some vhost. So, we want to keep semantics of both the options.
>>>
>>> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
>>> ---
>>>  net/tap.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/tap.c b/net/tap.c
>>> index d1ca314..4618359 100644
>>> --- a/net/tap.c
>>> +++ b/net/tap.c
>>> @@ -649,13 +649,13 @@ static void net_init_tap_one(const NetdevTapOptions
>>> *tap, NetClientState *peer,
>>>          }
>>>      }
>>>  
>>> -    if (tap->has_vhost ? tap->vhost :
>>> -        vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
>>> +    if ((tap->has_vhost ? tap->vhost :
>>> +        vhostfdname) || tap->vhostforce) {

The change here seems useless.

>>>          VhostNetOptions options;
>>>  
>>>          options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
>>>          options.net_backend = &s->nc;
>>> -        options.force = tap->has_vhostforce && tap->vhostforce;
>>> +        options.force = true;
>>>  
>>>          if (tap->has_vhostfd || tap->has_vhostfds) {
>>>              vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
>> In this case, I believe there's no need to have vhost_net_query() and
>> query_guest_notifiers() callbacks (and maybe more others).
> I also thought on this. If same functions can be used by some other module in future?
> If not, I was thinking to remove those in another patch.

I could not think a usage of this in the future.
>
> Does the main functionality looks OK? 

See comment above and I prefer to remove all unnecessary functions.

Thanks

Re: [Qemu-devel] [PATCH v2] net:Enable vhost with vhostforce, vhost options for guests without MSI-X support

[Pankaj Gupta]

> 
> On 06/11/2015 07:49 PM, Pankaj Gupta wrote:
> >> On 06/05/2015 10:32 PM, Pankaj Gupta wrote:
> >>>     We use vhostforce to enable vhost even if Guests don't have MSI-X
> >>>     support
> >>> and we fall back to QEMU virtio-net. This patch will enable vhost
> >>> unconditionally
> >>> whenever we have vhostforce='ON' or vhost='ON'.
> >>>
> >>> Initially, I wanted to remove vhostforce completely as an additional
> >>> argument.
> >>> But after discussing this in mailing list found that some programs are
> >>> using vhostforce
> >>> and some vhost. So, we want to keep semantics of both the options.
> >>>
> >>> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> >>> ---
> >>>  net/tap.c | 6 +++---
> >>>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/net/tap.c b/net/tap.c
> >>> index d1ca314..4618359 100644
> >>> --- a/net/tap.c
> >>> +++ b/net/tap.c
> >>> @@ -649,13 +649,13 @@ static void net_init_tap_one(const NetdevTapOptions
> >>> *tap, NetClientState *peer,
> >>>          }
> >>>      }
> >>>  
> >>> -    if (tap->has_vhost ? tap->vhost :
> >>> -        vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
> >>> +    if ((tap->has_vhost ? tap->vhost :
> >>> +        vhostfdname) || tap->vhostforce) {
> 
> The change here seems useless.
> 
> >>>          VhostNetOptions options;
> >>>  
> >>>          options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
> >>>          options.net_backend = &s->nc;
> >>> -        options.force = tap->has_vhostforce && tap->vhostforce;
> >>> +        options.force = true;
> >>>  
> >>>          if (tap->has_vhostfd || tap->has_vhostfds) {
> >>>              vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
> >> In this case, I believe there's no need to have vhost_net_query() and
> >> query_guest_notifiers() callbacks (and maybe more others).
> > I also thought on this. If same functions can be used by some other module
> > in future?
> > If not, I was thinking to remove those in another patch.
> 
> I could not think a usage of this in the future.
> >
> > Does the main functionality looks OK?
> 
> See comment above and I prefer to remove all unnecessary functions.

o.k, will do the changes and post a new version.
 
> 
> Thanks
> 
> 
>