cli: add configuration option to enable/disable vhost_net by caoruidong · Pull Request #706 · kata-containers/runtime

caoruidong · 2018-09-07T03:56:43Z

Add disable_vhost_net option to enable or disable the use of
vhost_net. Vhost_net can improve network performance.

Fixes: #169

Signed-off-by: Ruidong Cao caoruidong@huawei.com

opendev-zuul · 2018-09-07T04:36:47Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : FAILURE in 29m 02s

katacontainersbot · 2018-09-07T05:07:19Z

PSS Measurement:
Qemu: 169432 KB
Proxy: 4175 KB
Shim: 8789 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2003596 KB

bergwolf · 2018-09-07T07:08:01Z

@caoruidong I believe you need to change the govmm code as well since it also has hardcoded vhost dependency there. Something like:

diff --git a/vendor/github.com/intel/govmm/qemu/qmp.go b/vendor/github.com/intel/govmm/qemu/qmp.go
index 58b8924..1e7ed25 100644
--- a/vendor/github.com/intel/govmm/qemu/qmp.go
+++ b/vendor/github.com/intel/govmm/qemu/qmp.go
@@ -794,13 +794,16 @@ func (q *QMP) ExecuteNetdevChardevAdd(ctx context.Context, netdevType, netdevID,
 // Must be valid QMP identifier.
 func (q *QMP) ExecuteNetdevAddByFds(ctx context.Context, netdevType, netdevID string, fdNames, vhostFdNames []string) error {
        fdNameStr := strings.Join(fdNames, ":")
-       vhostFdNameStr := strings.Join(vhostFdNames, ":")
        args := map[string]interface{}{
-               "type":     netdevType,
-               "id":       netdevID,
-               "fds":      fdNameStr,
-               "vhost":    "on",
-               "vhostfds": vhostFdNameStr,
+               "type": netdevType,
+               "id":   netdevID,
+               "fds":  fdNameStr,
+       }
+
+       if len(vhostFdNames) != 0 {
+               vhostFdNameStr := strings.Join(vhostFdNames, ":")
+               args["vhost"] = "on"
+               args["vhostfds"] = vhostFdNameStr
        }

        return q.executeCommand(ctx, "netdev_add", args, nil)
@@ -832,7 +835,7 @@ func (q *QMP) ExecuteNetPCIDeviceAdd(ctx context.Context, netdevID, devID, macAd
                args["bus"] = bus
        }

-       if queues > 0 {
+       if queues > 1 {
                // (2N+2 vectors, N for tx queues, N for rx queues, 1 for config, and one for possible control vq)
                // -device virtio-net-pci,mq=on,vectors=2N+2...
                // enable mq in guest by 'ethtool -L eth0 combined $queue_num'

bergwolf

Please change govmm and update vendor here.

caoruidong · 2018-09-07T07:28:43Z

See #169 (comment). So we assume kata can still run without vhost_net?

katacontainersbot · 2018-09-07T07:32:13Z

PSS Measurement:
Qemu: 165962 KB
Proxy: 4098 KB
Shim: 8751 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2003572 KB

katacontainersbot · 2018-09-07T08:02:10Z

PSS Measurement:
Qemu: 169679 KB
Proxy: 4066 KB
Shim: 8841 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2003720 KB

opendev-zuul · 2018-09-07T08:56:22Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : TIMED_OUT in 1h 01m 00s

bergwolf · 2018-09-07T09:55:30Z

@caoruidong yes, I think so and I did succeed running kata w/o vhost locally.

sboeuf · 2018-09-10T18:21:17Z

@caoruidong please add @jiulongzaitian to the Signed-off-by section, since he's also part of this PR.

caoruidong · 2018-09-11T06:02:59Z

OK. The original commit has Soby. I'll squash them into one for clarity.

codecov · 2018-09-11T12:15:56Z

Codecov Report

Merging #706 into master will increase coverage by 0.07%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master     #706      +/-   ##
==========================================
+ Coverage   65.25%   65.33%   +0.07%     
==========================================
  Files          85       85              
  Lines        9956     9951       -5     
==========================================
+ Hits         6497     6501       +4     
+ Misses       2807     2798       -9     
  Partials      652      652

katacontainersbot · 2018-09-11T12:31:39Z

PSS Measurement:
Qemu: 173671 KB
Proxy: 4035 KB
Shim: 8889 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2003548 KB

opendev-zuul · 2018-09-11T13:33:48Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : TIMED_OUT in 1h 01m 10s

sboeuf · 2018-09-11T14:47:44Z

@caoruidong looks fine to me! Please rebase.

jodh-intel · 2018-09-11T14:50:01Z

lgtm

sboeuf · 2018-09-11T16:58:11Z

@bergwolf PTAL and update your review!

amshinde · 2018-09-11T19:05:03Z

Sorry, I am just seeing this PR.
I am not in favor of detecting if vhost is available during runtime.
This means we perform an exec, anytime we need to attach a network device.

cmd := exec.Command(modInfoCmd, module)

I think this should really be an explicit runtime configuration, set to true by default and documented.
What do others think about this?
cc @egernst

sboeuf · 2018-09-11T19:22:32Z

@amshinde actually I think you're totally right. A lot of exec are going to be performed eventually...
Having this, part of the hypervisor configuration would be the best place, but would require a bit of rework regarding network.go to make sure it can access the hypervisor config.

sboeuf

Agreed with @amshinde this patch will result in a lot of exec at runtime, which will impact performance.

bergwolf · 2018-09-12T04:11:13Z

I agree with @amshinde that this should be a config option and if vhost is enabled by configuration, try to modprobe it if it doesn't exist yet. And maybe we want another option to say if we want to fallback when vhost-net module doesn't exist.

caoruidong · 2018-09-12T08:06:15Z

I agree this patch doesn't work now. We create vhost fds without judgement.

runtime/virtcontainers/network.go

Lines 939 to 943 in e02695b

    
           vhostFds, err := createVhostFds(defaultQueues) 
        
           if err != nil { 
        
           	return fmt.Errorf("Could not setup vhost fds %s : %s", netPair.VirtIface.Name, err) 
        
           } 
        
           netPair.VhostFds = vhostFds

Our codes strongly depend on vhost_net. @bergwolf How do you run kata without vhost_net? Do you change the code?

jodh-intel · 2018-09-12T08:49:31Z

Good catch @amshinde - agreed!

katacontainersbot · 2018-09-13T07:29:48Z

PSS Measurement:
Qemu: 169613 KB
Proxy: 4029 KB
Shim: 9154 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2003432 KB

opendev-zuul · 2018-09-13T08:19:46Z

Build succeeded (third-party-check pipeline).

kata-runsh : SUCCESS in 58m 31s

bergwolf

LGTM. Just one comment on the new option explanation.

bergwolf · 2018-09-13T08:46:53Z

cli/config/configuration.toml.in


+# If host doesn't support vhost_net, set to true. Thus we won't create vhost fds for nics.
+# Default false
+#disable_vhost_net = true


So we no longger modprobe vhost-net, neither we fallback to non-vhost case, right? Please document it here that vhost-net kernel module needs to be builtin or inserted if vhost_net is enabled. Otherwise we'll fail to create the network endpoint for containers.

If vhost_net.ko exists but is not inserted, when someone accesses /dev/vhost-net, vhost_net.ko will be auto loaded. So we don't need to modprobe it by hand.
If vhost is enabled but actually ko doesn't exist, we won't know until we try to create vhost fds. So I wonder whether that is a good time to fallback to non-vhost when we meet error.

runtime/virtcontainers/network.go

Lines 917 to 921 in a5f05bf

vhostFds, err := createVhostFds(defaultQueues)

if err != nil {

return fmt.Errorf("Could not setup vhost fds %s : %s", netPair.VirtIface.Name, err)

}

netPair.VhostFds = vhostFds

Both just error out and fallback are OK.

It's OK to just fail in such case. Thanks for the info that vhost_net.ko is automatically loaded.

bergwolf · 2018-09-13T14:45:52Z

LGTM, thanks! @caoruidong Please rebase though since it conflicts with new changes merged in #719.

bergwolf · 2018-09-13T14:47:10Z

@sboeuf @amshinde ptal

…-net If the length of vhostfds is zero, it means host doesn't support vhost. So do not pass vhost="on" in QMP. Full list: 1a1fee7 qemu/qmp: nic can works without vhost Fixes kata-containers#169 Signed-off-by: Ruidong Cao <caoruidong@huawei.com>

Add `disable_vhost_net` option to enable or disable the use of vhost_net. Vhost_net can improve network performance. Signed-off-by: Ruidong Cao <caoruidong@huawei.com>

katacontainersbot · 2018-09-13T16:44:55Z

PSS Measurement:
Qemu: 169415 KB
Proxy: 3998 KB
Shim: 8743 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2006332 KB

opendev-zuul · 2018-09-13T17:49:45Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : TIMED_OUT in 1h 01m 12s

FC: Add new vsock connection handshake

egernst added the review label Sep 7, 2018

bergwolf suggested changes Sep 7, 2018

View reviewed changes

caoruidong force-pushed the vhost branch from ba050dc to 851c0f7 Compare September 7, 2018 07:15

caoruidong force-pushed the vhost branch from 851c0f7 to b4a0ae7 Compare September 7, 2018 07:45

sboeuf mentioned this pull request Sep 10, 2018

Disable VHost when host does not support vhost-net #422

Closed

caoruidong force-pushed the vhost branch from b4a0ae7 to f8577ff Compare September 11, 2018 12:15

sboeuf approved these changes Sep 11, 2018

View reviewed changes

sboeuf suggested changes Sep 11, 2018

View reviewed changes

sboeuf added the enhancement Improvement to an existing feature label Sep 12, 2018

caoruidong force-pushed the vhost branch from f8577ff to 6973625 Compare September 13, 2018 07:20

caoruidong changed the title ~~virtcontainers: set VHost to false when host doesn't have vhost_net.ko~~ cli: add configuration option to enable/disable vhost_net Sep 13, 2018

bergwolf reviewed Sep 13, 2018

View reviewed changes

bergwolf approved these changes Sep 13, 2018

View reviewed changes

caoruidong added 2 commits September 14, 2018 00:03

cli: add configuration option to enable/disable vhost_net

225e10c

Add `disable_vhost_net` option to enable or disable the use of vhost_net. Vhost_net can improve network performance. Signed-off-by: Ruidong Cao <caoruidong@huawei.com>

caoruidong force-pushed the vhost branch from 6973625 to 225e10c Compare September 13, 2018 16:15

amshinde approved these changes Sep 13, 2018

View reviewed changes

sboeuf approved these changes Sep 13, 2018

View reviewed changes

bergwolf merged commit d6e4a98 into kata-containers:master Sep 14, 2018

egernst removed the review label Sep 14, 2018

caoruidong deleted the vhost branch September 14, 2018 07:20

egernst pushed a commit to egernst/runtime that referenced this pull request Feb 9, 2021

Merge pull request kata-containers#706 from Pennyzct/vsock_handshake

cc95027

FC: Add new vsock connection handshake

	vhostFds, err := createVhostFds(defaultQueues)
	if err != nil {
	return fmt.Errorf("Could not setup vhost fds %s : %s", netPair.VirtIface.Name, err)
	}
	netPair.VhostFds = vhostFds

Conversation

caoruidong commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

opendev-zuul bot commented Sep 7, 2018

Uh oh!

katacontainersbot commented Sep 7, 2018

Uh oh!

bergwolf commented Sep 7, 2018

Uh oh!

bergwolf left a comment

Choose a reason for hiding this comment

Uh oh!

caoruidong commented Sep 7, 2018

Uh oh!

katacontainersbot commented Sep 7, 2018

Uh oh!

katacontainersbot commented Sep 7, 2018

Uh oh!

opendev-zuul bot commented Sep 7, 2018

Uh oh!

bergwolf commented Sep 7, 2018

Uh oh!

sboeuf commented Sep 10, 2018

Uh oh!

caoruidong commented Sep 11, 2018

Uh oh!

codecov bot commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

katacontainersbot commented Sep 11, 2018

Uh oh!

opendev-zuul bot commented Sep 11, 2018

Uh oh!

sboeuf commented Sep 11, 2018

Uh oh!

jodh-intel commented Sep 11, 2018 • edited by amshinde Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sboeuf commented Sep 11, 2018

Uh oh!

amshinde commented Sep 11, 2018

Uh oh!

sboeuf commented Sep 11, 2018

Uh oh!

sboeuf left a comment

Choose a reason for hiding this comment

Uh oh!

bergwolf commented Sep 12, 2018

Uh oh!

caoruidong commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jodh-intel commented Sep 12, 2018

Uh oh!

katacontainersbot commented Sep 13, 2018

Uh oh!

opendev-zuul bot commented Sep 13, 2018

Uh oh!

bergwolf left a comment

Choose a reason for hiding this comment

Uh oh!

bergwolf Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

caoruidong Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergwolf Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

bergwolf commented Sep 13, 2018

Uh oh!

bergwolf commented Sep 13, 2018

Uh oh!

katacontainersbot commented Sep 13, 2018

Uh oh!

opendev-zuul bot commented Sep 13, 2018

caoruidong commented Sep 7, 2018 •

edited

Loading

codecov bot commented Sep 11, 2018 •

edited

Loading

jodh-intel commented Sep 11, 2018 •

edited by amshinde

Loading

caoruidong commented Sep 12, 2018 •

edited

Loading

caoruidong Sep 13, 2018 •

edited

Loading