| Message ID | 1433514749-19584-1-git-send-email-pagupta@redhat.com |
|---|---|
| State | New |
| Headers | show |
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).
> > 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? >
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
> > 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 > > >
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);
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(-)