[PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Bobby Eshleman]

Hey everybody,

This series introduces datagrams, packet scheduling, and sk_buff usage
to virtio vsock.

The usage of struct sk_buff benefits users by a) preparing vsock to use
other related systems that require sk_buff, such as sockmap and qdisc,
b) supporting basic congestion control via sock_alloc_send_skb, and c)
reducing copying when delivering packets to TAP.

The socket layer no longer forces errors to be -ENOMEM, as typically
userspace expects -EAGAIN when the sk_sndbuf threshold is reached and
messages are being sent with option MSG_DONTWAIT.

The datagram work is based off previous patches by Jiang Wang[1].

The introduction of datagrams creates a transport layer fairness issue
where datagrams may freely starve streams of queue access. This happens
because, unlike streams, datagrams lack the transactions necessary for
calculating credits and throttling.

Previous proposals introduce changes to the spec to add an additional
virtqueue pair for datagrams[1]. Although this solution works, using
Linux's qdisc for packet scheduling leverages already existing systems,
avoids the need to change the virtio specification, and gives additional
capabilities. The usage of SFQ or fq_codel, for example, may solve the
transport layer starvation problem. It is easy to imagine other use
cases as well. For example, services of varying importance may be
assigned different priorities, and qdisc will apply appropriate
priority-based scheduling. By default, the system default pfifo qdisc is
used. The qdisc may be bypassed and legacy queuing is resumed by simply
setting the virtio-vsock%d network device to state DOWN. This technique
still allows vsock to work with zero-configuration.

In summary, this series introduces these major changes to vsock:

- virtio vsock supports datagrams
- virtio vsock uses struct sk_buff instead of virtio_vsock_pkt
  - Because virtio vsock uses sk_buff, it also uses sock_alloc_send_skb,
    which applies the throttling threshold sk_sndbuf.
- The vsock socket layer supports returning errors other than -ENOMEM.
  - This is used to return -EAGAIN when the sk_sndbuf threshold is
    reached.
- virtio vsock uses a net_device, through which qdisc may be used.
 - qdisc allows scheduling policies to be applied to vsock flows.
  - Some qdiscs, like SFQ, may allow vsock to avoid transport layer congestion. That is,
    it may avoid datagrams from flooding out stream flows. The benefit
    to this is that additional virtqueues are not needed for datagrams.
  - The net_device and qdisc is bypassed by simply setting the
    net_device state to DOWN.

[1]: https://lore.kernel.org/all/20210914055440.3121004-1-jiang.wang@bytedance.com/

Bobby Eshleman (5):
  vsock: replace virtio_vsock_pkt with sk_buff
  vsock: return errors other than -ENOMEM to socket
  vsock: add netdev to vhost/virtio vsock
  virtio/vsock: add VIRTIO_VSOCK_F_DGRAM feature bit
  virtio/vsock: add support for dgram

Jiang Wang (1):
  vsock_test: add tests for vsock dgram

 drivers/vhost/vsock.c                   | 238 ++++----
 include/linux/virtio_vsock.h            |  73 ++-
 include/net/af_vsock.h                  |   2 +
 include/uapi/linux/virtio_vsock.h       |   2 +
 net/vmw_vsock/af_vsock.c                |  30 +-
 net/vmw_vsock/hyperv_transport.c        |   2 +-
 net/vmw_vsock/virtio_transport.c        | 237 +++++---
 net/vmw_vsock/virtio_transport_common.c | 771 ++++++++++++++++--------
 net/vmw_vsock/vmci_transport.c          |   9 +-
 net/vmw_vsock/vsock_loopback.c          |  51 +-
 tools/testing/vsock/util.c              | 105 ++++
 tools/testing/vsock/util.h              |   4 +
 tools/testing/vsock/vsock_test.c        | 195 ++++++
 13 files changed, 1176 insertions(+), 543 deletions(-)

-- 
2.35.1

[PATCH 2/6] vsock: return errors other than -ENOMEM to socket

[Bobby Eshleman]

This commit allows vsock implementations to return errors
to the socket layer other than -ENOMEM. One immediate effect
of this is that upon the sk_sndbuf threshold being reached -EAGAIN
will be returned and userspace may throttle appropriately.

Resultingly, a known issue with uperf is resolved[1].

Additionally, to preserve legacy behavior for non-virtio
implementations, hyperv/vmci force errors to be -ENOMEM so that behavior
is unchanged.

[1]: https://gitlab.com/vsock/vsock/-/issues/1

Signed-off-by: Bobby Eshleman <bobby.eshleman@bytedance.com>
---
 include/linux/virtio_vsock.h            | 3 +++
 net/vmw_vsock/af_vsock.c                | 3 ++-
 net/vmw_vsock/hyperv_transport.c        | 2 +-
 net/vmw_vsock/virtio_transport_common.c | 3 ---
 net/vmw_vsock/vmci_transport.c          | 9 ++++++++-
 5 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
index 17ed01466875..9a37eddbb87a 100644
--- a/include/linux/virtio_vsock.h
+++ b/include/linux/virtio_vsock.h
@@ -8,6 +8,9 @@
 #include <net/sock.h>
 #include <net/af_vsock.h>
 
+/* Threshold for detecting small packets to copy */
+#define GOOD_COPY_LEN  128
+
 enum virtio_vsock_metadata_flags {
 	VIRTIO_VSOCK_METADATA_FLAGS_REPLY		= BIT(0),
 	VIRTIO_VSOCK_METADATA_FLAGS_TAP_DELIVERED	= BIT(1),
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index e348b2d09eac..1893f8aafa48 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1844,8 +1844,9 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg,
 			written = transport->stream_enqueue(vsk,
 					msg, len - total_written);
 		}
+
 		if (written < 0) {
-			err = -ENOMEM;
+			err = written;
 			goto out_err;
 		}
 
diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index fd98229e3db3..e99aea571f6f 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -687,7 +687,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk, struct msghdr *msg,
 	if (bytes_written)
 		ret = bytes_written;
 	kfree(send_buf);
-	return ret;
+	return ret < 0 ? -ENOMEM : ret;
 }
 
 static s64 hvs_stream_has_data(struct vsock_sock *vsk)
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 920578597bb9..d5780599fe93 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -23,9 +23,6 @@
 /* How long to wait for graceful shutdown of a connection */
 #define VSOCK_CLOSE_TIMEOUT (8 * HZ)
 
-/* Threshold for detecting small packets to copy */
-#define GOOD_COPY_LEN  128
-
 static const struct virtio_transport *
 virtio_transport_get_ops(struct vsock_sock *vsk)
 {
diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index b14f0ed7427b..c927a90dc859 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -1838,7 +1838,14 @@ static ssize_t vmci_transport_stream_enqueue(
 	struct msghdr *msg,
 	size_t len)
 {
-	return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
+	int err;
+
+	err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
+
+	if (err < 0)
+		err = -ENOMEM;
+
+	return err;
 }
 
 static s64 vmci_transport_stream_has_data(struct vsock_sock *vsk)
-- 
2.35.1

Re: [PATCH 2/6] vsock: return errors other than -ENOMEM to socket

[kernel test robot]

Hi Bobby,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mst-vhost/linux-next]
[also build test WARNING on linus/master v6.0-rc1 next-20220815]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bobby-Eshleman/virtio-vsock-introduce-dgrams-sk_buff-and-qdisc/20220816-015812
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220816/202208160300.HYFUSTbF-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/68c9c8216a573cdfe2170cad677854e2f4a34634
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Bobby-Eshleman/virtio-vsock-introduce-dgrams-sk_buff-and-qdisc/20220816-015812
        git checkout 68c9c8216a573cdfe2170cad677854e2f4a34634
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash net/vmw_vsock/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/vmw_vsock/virtio_transport.c:178: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    * Merge the two most recent skbs together if possible.


vim +178 net/vmw_vsock/virtio_transport.c

93afaf2cdefaa9 Bobby Eshleman 2022-08-15  176  
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  177  /**
93afaf2cdefaa9 Bobby Eshleman 2022-08-15 @178   * Merge the two most recent skbs together if possible.
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  179   *
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  180   * Caller must hold the queue lock.
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  181   */
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  182  static void
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  183  virtio_transport_add_to_queue(struct sk_buff_head *queue, struct sk_buff *new)
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  184  {
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  185  	struct sk_buff *old;
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  186  
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  187  	spin_lock_bh(&queue->lock);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  188  	/* In order to reduce skb memory overhead, we merge new packets with
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  189  	 * older packets if they pass virtio_transport_skbs_can_merge().
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  190  	 */
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  191  	if (skb_queue_empty_lockless(queue)) {
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  192  		__skb_queue_tail(queue, new);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  193  		goto out;
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  194  	}
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  195  
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  196  	old = skb_peek_tail(queue);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  197  
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  198  	if (!virtio_transport_skbs_can_merge(old, new)) {
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  199  		__skb_queue_tail(queue, new);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  200  		goto out;
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  201  	}
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  202  
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  203  	memcpy(skb_put(old, new->len), new->data, new->len);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  204  	vsock_hdr(old)->len = cpu_to_le32(old->len);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  205  	vsock_hdr(old)->buf_alloc = vsock_hdr(new)->buf_alloc;
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  206  	vsock_hdr(old)->fwd_cnt = vsock_hdr(new)->fwd_cnt;
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  207  	dev_kfree_skb_any(new);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  208  
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  209  out:
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  210  	spin_unlock_bh(&queue->lock);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  211  }
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  212  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Michael S. Tsirkin]

On Mon, Aug 15, 2022 at 10:56:03AM -0700, Bobby Eshleman wrote:
> Hey everybody,
> 
> This series introduces datagrams, packet scheduling, and sk_buff usage
> to virtio vsock.
> 
> The usage of struct sk_buff benefits users by a) preparing vsock to use
> other related systems that require sk_buff, such as sockmap and qdisc,
> b) supporting basic congestion control via sock_alloc_send_skb, and c)
> reducing copying when delivering packets to TAP.
> 
> The socket layer no longer forces errors to be -ENOMEM, as typically
> userspace expects -EAGAIN when the sk_sndbuf threshold is reached and
> messages are being sent with option MSG_DONTWAIT.
> 
> The datagram work is based off previous patches by Jiang Wang[1].
> 
> The introduction of datagrams creates a transport layer fairness issue
> where datagrams may freely starve streams of queue access. This happens
> because, unlike streams, datagrams lack the transactions necessary for
> calculating credits and throttling.
> 
> Previous proposals introduce changes to the spec to add an additional
> virtqueue pair for datagrams[1]. Although this solution works, using
> Linux's qdisc for packet scheduling leverages already existing systems,
> avoids the need to change the virtio specification, and gives additional
> capabilities. The usage of SFQ or fq_codel, for example, may solve the
> transport layer starvation problem. It is easy to imagine other use
> cases as well. For example, services of varying importance may be
> assigned different priorities, and qdisc will apply appropriate
> priority-based scheduling. By default, the system default pfifo qdisc is
> used. The qdisc may be bypassed and legacy queuing is resumed by simply
> setting the virtio-vsock%d network device to state DOWN. This technique
> still allows vsock to work with zero-configuration.
> 
> In summary, this series introduces these major changes to vsock:
> 
> - virtio vsock supports datagrams
> - virtio vsock uses struct sk_buff instead of virtio_vsock_pkt
>   - Because virtio vsock uses sk_buff, it also uses sock_alloc_send_skb,
>     which applies the throttling threshold sk_sndbuf.
> - The vsock socket layer supports returning errors other than -ENOMEM.
>   - This is used to return -EAGAIN when the sk_sndbuf threshold is
>     reached.
> - virtio vsock uses a net_device, through which qdisc may be used.
>  - qdisc allows scheduling policies to be applied to vsock flows.
>   - Some qdiscs, like SFQ, may allow vsock to avoid transport layer congestion. That is,
>     it may avoid datagrams from flooding out stream flows. The benefit
>     to this is that additional virtqueues are not needed for datagrams.
>   - The net_device and qdisc is bypassed by simply setting the
>     net_device state to DOWN.
> 
> [1]: https://lore.kernel.org/all/20210914055440.3121004-1-jiang.wang@bytedance.com/

Given this affects the driver/device interface I'd like to
ask you to please copy virtio-dev mailing list on these patches.
Subscriber only I'm afraid you will need to subscribe :(

> Bobby Eshleman (5):
>   vsock: replace virtio_vsock_pkt with sk_buff
>   vsock: return errors other than -ENOMEM to socket
>   vsock: add netdev to vhost/virtio vsock
>   virtio/vsock: add VIRTIO_VSOCK_F_DGRAM feature bit
>   virtio/vsock: add support for dgram
> 
> Jiang Wang (1):
>   vsock_test: add tests for vsock dgram
> 
>  drivers/vhost/vsock.c                   | 238 ++++----
>  include/linux/virtio_vsock.h            |  73 ++-
>  include/net/af_vsock.h                  |   2 +
>  include/uapi/linux/virtio_vsock.h       |   2 +
>  net/vmw_vsock/af_vsock.c                |  30 +-
>  net/vmw_vsock/hyperv_transport.c        |   2 +-
>  net/vmw_vsock/virtio_transport.c        | 237 +++++---
>  net/vmw_vsock/virtio_transport_common.c | 771 ++++++++++++++++--------
>  net/vmw_vsock/vmci_transport.c          |   9 +-
>  net/vmw_vsock/vsock_loopback.c          |  51 +-
>  tools/testing/vsock/util.c              | 105 ++++
>  tools/testing/vsock/util.h              |   4 +
>  tools/testing/vsock/vsock_test.c        | 195 ++++++
>  13 files changed, 1176 insertions(+), 543 deletions(-)
> 
> -- 
> 2.35.1

Re: [PATCH 2/6] vsock: return errors other than -ENOMEM to socket

[kernel test robot]

Hi Bobby,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mst-vhost/linux-next]
[also build test WARNING on linus/master v6.0-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bobby-Eshleman/virtio-vsock-introduce-dgrams-sk_buff-and-qdisc/20220816-015812
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: i386-randconfig-a014-20220815 (https://download.01.org/0day-ci/archive/20220816/202208160737.gXXFmPbY-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 6afcc4a459ead8809a0d6d9b4bf7b64bcc13582b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/68c9c8216a573cdfe2170cad677854e2f4a34634
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Bobby-Eshleman/virtio-vsock-introduce-dgrams-sk_buff-and-qdisc/20220816-015812
        git checkout 68c9c8216a573cdfe2170cad677854e2f4a34634
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/vmw_vsock/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/vmw_vsock/virtio_transport.c:178: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    * Merge the two most recent skbs together if possible.


vim +178 net/vmw_vsock/virtio_transport.c

93afaf2cdefaa9 Bobby Eshleman 2022-08-15  176  
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  177  /**
93afaf2cdefaa9 Bobby Eshleman 2022-08-15 @178   * Merge the two most recent skbs together if possible.
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  179   *
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  180   * Caller must hold the queue lock.
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  181   */
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  182  static void
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  183  virtio_transport_add_to_queue(struct sk_buff_head *queue, struct sk_buff *new)
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  184  {
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  185  	struct sk_buff *old;
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  186  
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  187  	spin_lock_bh(&queue->lock);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  188  	/* In order to reduce skb memory overhead, we merge new packets with
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  189  	 * older packets if they pass virtio_transport_skbs_can_merge().
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  190  	 */
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  191  	if (skb_queue_empty_lockless(queue)) {
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  192  		__skb_queue_tail(queue, new);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  193  		goto out;
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  194  	}
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  195  
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  196  	old = skb_peek_tail(queue);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  197  
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  198  	if (!virtio_transport_skbs_can_merge(old, new)) {
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  199  		__skb_queue_tail(queue, new);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  200  		goto out;
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  201  	}
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  202  
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  203  	memcpy(skb_put(old, new->len), new->data, new->len);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  204  	vsock_hdr(old)->len = cpu_to_le32(old->len);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  205  	vsock_hdr(old)->buf_alloc = vsock_hdr(new)->buf_alloc;
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  206  	vsock_hdr(old)->fwd_cnt = vsock_hdr(new)->fwd_cnt;
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  207  	dev_kfree_skb_any(new);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  208  
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  209  out:
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  210  	spin_unlock_bh(&queue->lock);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  211  }
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  212  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Bobby Eshleman]

On Mon, Aug 15, 2022 at 04:39:08PM -0400, Michael S. Tsirkin wrote:
> 
> Given this affects the driver/device interface I'd like to
> ask you to please copy virtio-dev mailing list on these patches.
> Subscriber only I'm afraid you will need to subscribe :(
> 

Ah makes sense, will do!

Best,
Bobby

Re: [PATCH 2/6] vsock: return errors other than -ENOMEM to socket

[kernel test robot]

Hi Bobby,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mst-vhost/linux-next]
[also build test WARNING on linus/master v6.0-rc1 next-20220815]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bobby-Eshleman/virtio-vsock-introduce-dgrams-sk_buff-and-qdisc/20220816-015812
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: i386-randconfig-s001-20220815 (https://download.01.org/0day-ci/archive/20220816/202208161059.GPIlPpvd-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/68c9c8216a573cdfe2170cad677854e2f4a34634
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Bobby-Eshleman/virtio-vsock-introduce-dgrams-sk_buff-and-qdisc/20220816-015812
        git checkout 68c9c8216a573cdfe2170cad677854e2f4a34634
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash fs/nilfs2/ net/vmw_vsock/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

sparse warnings: (new ones prefixed by >>)
>> net/vmw_vsock/virtio_transport.c:173:31: sparse: sparse: restricted __le16 degrades to integer
   net/vmw_vsock/virtio_transport.c:174:31: sparse: sparse: restricted __le16 degrades to integer

vim +173 net/vmw_vsock/virtio_transport.c

0ea9e1d3a9e3ef Asias He       2016-07-28  161  
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  162  static inline bool
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  163  virtio_transport_skbs_can_merge(struct sk_buff *old, struct sk_buff *new)
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  164  {
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  165  	return (new->len < GOOD_COPY_LEN &&
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  166  		skb_tailroom(old) >= new->len &&
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  167  		vsock_hdr(new)->src_cid == vsock_hdr(old)->src_cid &&
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  168  		vsock_hdr(new)->dst_cid == vsock_hdr(old)->dst_cid &&
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  169  		vsock_hdr(new)->src_port == vsock_hdr(old)->src_port &&
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  170  		vsock_hdr(new)->dst_port == vsock_hdr(old)->dst_port &&
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  171  		vsock_hdr(new)->type == vsock_hdr(old)->type &&
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  172  		vsock_hdr(new)->flags == vsock_hdr(old)->flags &&
93afaf2cdefaa9 Bobby Eshleman 2022-08-15 @173  		vsock_hdr(old)->op == VIRTIO_VSOCK_OP_RW &&
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  174  		vsock_hdr(new)->op == VIRTIO_VSOCK_OP_RW);
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  175  }
93afaf2cdefaa9 Bobby Eshleman 2022-08-15  176  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Bobby Eshleman]

CC'ing virtio-dev@lists.oasis-open.org

On Mon, Aug 15, 2022 at 10:56:03AM -0700, Bobby Eshleman wrote:
> Hey everybody,
> 
> This series introduces datagrams, packet scheduling, and sk_buff usage
> to virtio vsock.
> 
> The usage of struct sk_buff benefits users by a) preparing vsock to use
> other related systems that require sk_buff, such as sockmap and qdisc,
> b) supporting basic congestion control via sock_alloc_send_skb, and c)
> reducing copying when delivering packets to TAP.
> 
> The socket layer no longer forces errors to be -ENOMEM, as typically
> userspace expects -EAGAIN when the sk_sndbuf threshold is reached and
> messages are being sent with option MSG_DONTWAIT.
> 
> The datagram work is based off previous patches by Jiang Wang[1].
> 
> The introduction of datagrams creates a transport layer fairness issue
> where datagrams may freely starve streams of queue access. This happens
> because, unlike streams, datagrams lack the transactions necessary for
> calculating credits and throttling.
> 
> Previous proposals introduce changes to the spec to add an additional
> virtqueue pair for datagrams[1]. Although this solution works, using
> Linux's qdisc for packet scheduling leverages already existing systems,
> avoids the need to change the virtio specification, and gives additional
> capabilities. The usage of SFQ or fq_codel, for example, may solve the
> transport layer starvation problem. It is easy to imagine other use
> cases as well. For example, services of varying importance may be
> assigned different priorities, and qdisc will apply appropriate
> priority-based scheduling. By default, the system default pfifo qdisc is
> used. The qdisc may be bypassed and legacy queuing is resumed by simply
> setting the virtio-vsock%d network device to state DOWN. This technique
> still allows vsock to work with zero-configuration.
> 
> In summary, this series introduces these major changes to vsock:
> 
> - virtio vsock supports datagrams
> - virtio vsock uses struct sk_buff instead of virtio_vsock_pkt
>   - Because virtio vsock uses sk_buff, it also uses sock_alloc_send_skb,
>     which applies the throttling threshold sk_sndbuf.
> - The vsock socket layer supports returning errors other than -ENOMEM.
>   - This is used to return -EAGAIN when the sk_sndbuf threshold is
>     reached.
> - virtio vsock uses a net_device, through which qdisc may be used.
>  - qdisc allows scheduling policies to be applied to vsock flows.
>   - Some qdiscs, like SFQ, may allow vsock to avoid transport layer congestion. That is,
>     it may avoid datagrams from flooding out stream flows. The benefit
>     to this is that additional virtqueues are not needed for datagrams.
>   - The net_device and qdisc is bypassed by simply setting the
>     net_device state to DOWN.
> 
> [1]: https://lore.kernel.org/all/20210914055440.3121004-1-jiang.wang@bytedance.com/
> 
> Bobby Eshleman (5):
>   vsock: replace virtio_vsock_pkt with sk_buff
>   vsock: return errors other than -ENOMEM to socket
>   vsock: add netdev to vhost/virtio vsock
>   virtio/vsock: add VIRTIO_VSOCK_F_DGRAM feature bit
>   virtio/vsock: add support for dgram
> 
> Jiang Wang (1):
>   vsock_test: add tests for vsock dgram
> 
>  drivers/vhost/vsock.c                   | 238 ++++----
>  include/linux/virtio_vsock.h            |  73 ++-
>  include/net/af_vsock.h                  |   2 +
>  include/uapi/linux/virtio_vsock.h       |   2 +
>  net/vmw_vsock/af_vsock.c                |  30 +-
>  net/vmw_vsock/hyperv_transport.c        |   2 +-
>  net/vmw_vsock/virtio_transport.c        | 237 +++++---
>  net/vmw_vsock/virtio_transport_common.c | 771 ++++++++++++++++--------
>  net/vmw_vsock/vmci_transport.c          |   9 +-
>  net/vmw_vsock/vsock_loopback.c          |  51 +-
>  tools/testing/vsock/util.c              | 105 ++++
>  tools/testing/vsock/util.h              |   4 +
>  tools/testing/vsock/vsock_test.c        | 195 ++++++
>  13 files changed, 1176 insertions(+), 543 deletions(-)
> 
> -- 
> 2.35.1
> 

Re: [PATCH 2/6] vsock: return errors other than -ENOMEM to socket

[Bobby Eshleman]

CC'ing virtio-dev@lists.oasis-open.org

On Mon, Aug 15, 2022 at 10:56:05AM -0700, Bobby Eshleman wrote:
> This commit allows vsock implementations to return errors
> to the socket layer other than -ENOMEM. One immediate effect
> of this is that upon the sk_sndbuf threshold being reached -EAGAIN
> will be returned and userspace may throttle appropriately.
> 
> Resultingly, a known issue with uperf is resolved[1].
> 
> Additionally, to preserve legacy behavior for non-virtio
> implementations, hyperv/vmci force errors to be -ENOMEM so that behavior
> is unchanged.
> 
> [1]: https://gitlab.com/vsock/vsock/-/issues/1
> 
> Signed-off-by: Bobby Eshleman <bobby.eshleman@bytedance.com>
> ---
>  include/linux/virtio_vsock.h            | 3 +++
>  net/vmw_vsock/af_vsock.c                | 3 ++-
>  net/vmw_vsock/hyperv_transport.c        | 2 +-
>  net/vmw_vsock/virtio_transport_common.c | 3 ---
>  net/vmw_vsock/vmci_transport.c          | 9 ++++++++-
>  5 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> index 17ed01466875..9a37eddbb87a 100644
> --- a/include/linux/virtio_vsock.h
> +++ b/include/linux/virtio_vsock.h
> @@ -8,6 +8,9 @@
>  #include <net/sock.h>
>  #include <net/af_vsock.h>
>  
> +/* Threshold for detecting small packets to copy */
> +#define GOOD_COPY_LEN  128
> +
>  enum virtio_vsock_metadata_flags {
>  	VIRTIO_VSOCK_METADATA_FLAGS_REPLY		= BIT(0),
>  	VIRTIO_VSOCK_METADATA_FLAGS_TAP_DELIVERED	= BIT(1),
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index e348b2d09eac..1893f8aafa48 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1844,8 +1844,9 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg,
>  			written = transport->stream_enqueue(vsk,
>  					msg, len - total_written);
>  		}
> +
>  		if (written < 0) {
> -			err = -ENOMEM;
> +			err = written;
>  			goto out_err;
>  		}
>  
> diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> index fd98229e3db3..e99aea571f6f 100644
> --- a/net/vmw_vsock/hyperv_transport.c
> +++ b/net/vmw_vsock/hyperv_transport.c
> @@ -687,7 +687,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk, struct msghdr *msg,
>  	if (bytes_written)
>  		ret = bytes_written;
>  	kfree(send_buf);
> -	return ret;
> +	return ret < 0 ? -ENOMEM : ret;
>  }
>  
>  static s64 hvs_stream_has_data(struct vsock_sock *vsk)
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 920578597bb9..d5780599fe93 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -23,9 +23,6 @@
>  /* How long to wait for graceful shutdown of a connection */
>  #define VSOCK_CLOSE_TIMEOUT (8 * HZ)
>  
> -/* Threshold for detecting small packets to copy */
> -#define GOOD_COPY_LEN  128
> -
>  static const struct virtio_transport *
>  virtio_transport_get_ops(struct vsock_sock *vsk)
>  {
> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> index b14f0ed7427b..c927a90dc859 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -1838,7 +1838,14 @@ static ssize_t vmci_transport_stream_enqueue(
>  	struct msghdr *msg,
>  	size_t len)
>  {
> -	return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
> +	int err;
> +
> +	err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
> +
> +	if (err < 0)
> +		err = -ENOMEM;
> +
> +	return err;
>  }
>  
>  static s64 vmci_transport_stream_has_data(struct vsock_sock *vsk)
> -- 
> 2.35.1
> 

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Bobby Eshleman]

On Tue, Aug 16, 2022 at 09:00:45AM +0200, Stefano Garzarella wrote:
> Hi Bobby,

..

> 
> Please send next versions of this series as RFC until we have at least an
> agreement on the spec changes.
> 
> I think will be better to agree on the spec before merge Linux changes.
> 
> Thanks,
> Stefano
> 

Duly noted, I'll tag it as RFC on the next send.


Best,
Bobby

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Bobby Eshleman]

On Wed, Aug 17, 2022 at 02:54:33AM -0400, Michael S. Tsirkin wrote:
> On Mon, Aug 15, 2022 at 10:56:03AM -0700, Bobby Eshleman wrote:
> > Hey everybody,
> > 
> > This series introduces datagrams, packet scheduling, and sk_buff usage
> > to virtio vsock.
> > 
> > The usage of struct sk_buff benefits users by a) preparing vsock to use
> > other related systems that require sk_buff, such as sockmap and qdisc,
> > b) supporting basic congestion control via sock_alloc_send_skb, and c)
> > reducing copying when delivering packets to TAP.
> > 
> > The socket layer no longer forces errors to be -ENOMEM, as typically
> > userspace expects -EAGAIN when the sk_sndbuf threshold is reached and
> > messages are being sent with option MSG_DONTWAIT.
> > 
> > The datagram work is based off previous patches by Jiang Wang[1].
> > 
> > The introduction of datagrams creates a transport layer fairness issue
> > where datagrams may freely starve streams of queue access. This happens
> > because, unlike streams, datagrams lack the transactions necessary for
> > calculating credits and throttling.
> > 
> > Previous proposals introduce changes to the spec to add an additional
> > virtqueue pair for datagrams[1]. Although this solution works, using
> > Linux's qdisc for packet scheduling leverages already existing systems,
> > avoids the need to change the virtio specification, and gives additional
> > capabilities. The usage of SFQ or fq_codel, for example, may solve the
> > transport layer starvation problem. It is easy to imagine other use
> > cases as well. For example, services of varying importance may be
> > assigned different priorities, and qdisc will apply appropriate
> > priority-based scheduling. By default, the system default pfifo qdisc is
> > used. The qdisc may be bypassed and legacy queuing is resumed by simply
> > setting the virtio-vsock%d network device to state DOWN. This technique
> > still allows vsock to work with zero-configuration.
> 
> The basic question to answer then is this: with a net device qdisc
> etc in the picture, how is this different from virtio net then?
> Why do you still want to use vsock?
> 

When using virtio-net, users looking for inter-VM communication are
required to setup bridges, TAPs, allocate IP addresses or setup DNS,
etc... and then finally when you have a network, you can open a socket
on an IP address and port. This is the configuration that vsock avoids.
For vsock, we just need a CID and a port, but no network configuration.

This benefit still exists after introducing a netdev to vsock. The major
added benefit is that when you have many different vsock flows in
parallel and you are observing issues like starvation and tail latency
that are caused by pure FIFO queuing, now there is a mechanism to fix
those issues. You might recall such an issue discussed here[1].

[1]: https://gitlab.com/vsock/vsock/-/issues/1

> > In summary, this series introduces these major changes to vsock:
> > 
> > - virtio vsock supports datagrams
> > - virtio vsock uses struct sk_buff instead of virtio_vsock_pkt
> >   - Because virtio vsock uses sk_buff, it also uses sock_alloc_send_skb,
> >     which applies the throttling threshold sk_sndbuf.
> > - The vsock socket layer supports returning errors other than -ENOMEM.
> >   - This is used to return -EAGAIN when the sk_sndbuf threshold is
> >     reached.
> > - virtio vsock uses a net_device, through which qdisc may be used.
> >  - qdisc allows scheduling policies to be applied to vsock flows.
> >   - Some qdiscs, like SFQ, may allow vsock to avoid transport layer congestion. That is,
> >     it may avoid datagrams from flooding out stream flows. The benefit
> >     to this is that additional virtqueues are not needed for datagrams.
> >   - The net_device and qdisc is bypassed by simply setting the
> >     net_device state to DOWN.
> > 
> > [1]: https://lore.kernel.org/all/20210914055440.3121004-1-jiang.wang@bytedance.com/
> > 
> > Bobby Eshleman (5):
> >   vsock: replace virtio_vsock_pkt with sk_buff
> >   vsock: return errors other than -ENOMEM to socket
> >   vsock: add netdev to vhost/virtio vsock
> >   virtio/vsock: add VIRTIO_VSOCK_F_DGRAM feature bit
> >   virtio/vsock: add support for dgram
> > 
> > Jiang Wang (1):
> >   vsock_test: add tests for vsock dgram
> > 
> >  drivers/vhost/vsock.c                   | 238 ++++----
> >  include/linux/virtio_vsock.h            |  73 ++-
> >  include/net/af_vsock.h                  |   2 +
> >  include/uapi/linux/virtio_vsock.h       |   2 +
> >  net/vmw_vsock/af_vsock.c                |  30 +-
> >  net/vmw_vsock/hyperv_transport.c        |   2 +-
> >  net/vmw_vsock/virtio_transport.c        | 237 +++++---
> >  net/vmw_vsock/virtio_transport_common.c | 771 ++++++++++++++++--------
> >  net/vmw_vsock/vmci_transport.c          |   9 +-
> >  net/vmw_vsock/vsock_loopback.c          |  51 +-
> >  tools/testing/vsock/util.c              | 105 ++++
> >  tools/testing/vsock/util.h              |   4 +
> >  tools/testing/vsock/vsock_test.c        | 195 ++++++
> >  13 files changed, 1176 insertions(+), 543 deletions(-)
> > 
> > -- 
> > 2.35.1
> 

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Bobby Eshleman]

On Wed, Aug 17, 2022 at 01:02:52PM -0400, Michael S. Tsirkin wrote:
> On Tue, Aug 16, 2022 at 09:42:51AM +0000, Bobby Eshleman wrote:
> > > The basic question to answer then is this: with a net device qdisc
> > > etc in the picture, how is this different from virtio net then?
> > > Why do you still want to use vsock?
> > > 
> > 
> > When using virtio-net, users looking for inter-VM communication are
> > required to setup bridges, TAPs, allocate IP addresses or setup DNS,
> > etc... and then finally when you have a network, you can open a socket
> > on an IP address and port. This is the configuration that vsock avoids.
> > For vsock, we just need a CID and a port, but no network configuration.
> 
> Surely when you mention DNS you are going overboard? vsock doesn't
> remove the need for DNS as much as it does not support it.
> 

Oops, s/DNS/dhcp.

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Bobby Eshleman]

On Wed, Aug 17, 2022 at 01:53:32PM -0400, Michael S. Tsirkin wrote:
> On Tue, Aug 16, 2022 at 11:08:26AM +0000, Bobby Eshleman wrote:
> > On Wed, Aug 17, 2022 at 01:02:52PM -0400, Michael S. Tsirkin wrote:
> > > On Tue, Aug 16, 2022 at 09:42:51AM +0000, Bobby Eshleman wrote:
> > > > > The basic question to answer then is this: with a net device qdisc
> > > > > etc in the picture, how is this different from virtio net then?
> > > > > Why do you still want to use vsock?
> > > > > 
> > > > 
> > > > When using virtio-net, users looking for inter-VM communication are
> > > > required to setup bridges, TAPs, allocate IP addresses or setup DNS,
> > > > etc... and then finally when you have a network, you can open a socket
> > > > on an IP address and port. This is the configuration that vsock avoids.
> > > > For vsock, we just need a CID and a port, but no network configuration.
> > > 
> > > Surely when you mention DNS you are going overboard? vsock doesn't
> > > remove the need for DNS as much as it does not support it.
> > > 
> > 
> > Oops, s/DNS/dhcp.
> 
> That too.
> 

Sure, setting up dhcp would be overboard for just inter-VM comms.

It is fair to mention that vsock CIDs also need to be managed /
allocated somehow.

Re: [virtio-dev] Re: [PATCH 2/6] vsock: return errors other than -ENOMEM to socket

[Arseniy Krasnov]

On 16.08.2022 05:30, Bobby Eshleman wrote:
> CC'ing virtio-dev@lists.oasis-open.org
> 
> On Mon, Aug 15, 2022 at 10:56:05AM -0700, Bobby Eshleman wrote:
>> This commit allows vsock implementations to return errors
>> to the socket layer other than -ENOMEM. One immediate effect
>> of this is that upon the sk_sndbuf threshold being reached -EAGAIN
>> will be returned and userspace may throttle appropriately.
>>
>> Resultingly, a known issue with uperf is resolved[1].
>>
>> Additionally, to preserve legacy behavior for non-virtio
>> implementations, hyperv/vmci force errors to be -ENOMEM so that behavior
>> is unchanged.
>>
>> [1]: https://gitlab.com/vsock/vsock/-/issues/1
>>
>> Signed-off-by: Bobby Eshleman <bobby.eshleman@bytedance.com>
>> ---
>>  include/linux/virtio_vsock.h            | 3 +++
>>  net/vmw_vsock/af_vsock.c                | 3 ++-
>>  net/vmw_vsock/hyperv_transport.c        | 2 +-
>>  net/vmw_vsock/virtio_transport_common.c | 3 ---
>>  net/vmw_vsock/vmci_transport.c          | 9 ++++++++-
>>  5 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>> index 17ed01466875..9a37eddbb87a 100644
>> --- a/include/linux/virtio_vsock.h
>> +++ b/include/linux/virtio_vsock.h
>> @@ -8,6 +8,9 @@
>>  #include <net/sock.h>
>>  #include <net/af_vsock.h>
>>  
>> +/* Threshold for detecting small packets to copy */
>> +#define GOOD_COPY_LEN  128
>> +
>>  enum virtio_vsock_metadata_flags {
>>  	VIRTIO_VSOCK_METADATA_FLAGS_REPLY		= BIT(0),
>>  	VIRTIO_VSOCK_METADATA_FLAGS_TAP_DELIVERED	= BIT(1),
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index e348b2d09eac..1893f8aafa48 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -1844,8 +1844,9 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg,
>>  			written = transport->stream_enqueue(vsk,
>>  					msg, len - total_written);
>>  		}
>> +
>>  		if (written < 0) {
>> -			err = -ENOMEM;
>> +			err = written;
>>  			goto out_err;
>>  		}
IIUC, for stream, this thing will have effect, only one first transport access fails. In this
case 'total_written' will be 0, so 'err' == 'written' will be returned. But when 'total_written > 0',
'err' will be overwritten by 'total_written' below, preserving current behaviour. Is it what You
supposed?

Thanks
>>  
>> diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
>> index fd98229e3db3..e99aea571f6f 100644
>> --- a/net/vmw_vsock/hyperv_transport.c
>> +++ b/net/vmw_vsock/hyperv_transport.c
>> @@ -687,7 +687,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk, struct msghdr *msg,
>>  	if (bytes_written)
>>  		ret = bytes_written;
>>  	kfree(send_buf);
>> -	return ret;
>> +	return ret < 0 ? -ENOMEM : ret;
>>  }
>>  
>>  static s64 hvs_stream_has_data(struct vsock_sock *vsk)
>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> index 920578597bb9..d5780599fe93 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -23,9 +23,6 @@
>>  /* How long to wait for graceful shutdown of a connection */
>>  #define VSOCK_CLOSE_TIMEOUT (8 * HZ)
>>  
>> -/* Threshold for detecting small packets to copy */
>> -#define GOOD_COPY_LEN  128
>> -
>>  static const struct virtio_transport *
>>  virtio_transport_get_ops(struct vsock_sock *vsk)
>>  {
>> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
>> index b14f0ed7427b..c927a90dc859 100644
>> --- a/net/vmw_vsock/vmci_transport.c
>> +++ b/net/vmw_vsock/vmci_transport.c
>> @@ -1838,7 +1838,14 @@ static ssize_t vmci_transport_stream_enqueue(
>>  	struct msghdr *msg,
>>  	size_t len)
>>  {
>> -	return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
>> +	int err;
>> +
>> +	err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
>> +
>> +	if (err < 0)
>> +		err = -ENOMEM;
>> +
>> +	return err;
>>  }
>>  
>>  static s64 vmci_transport_stream_has_data(struct vsock_sock *vsk)
>> -- 
>> 2.35.1
>>
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Michael S. Tsirkin]

On Mon, Aug 15, 2022 at 10:56:03AM -0700, Bobby Eshleman wrote:
> Hey everybody,
> 
> This series introduces datagrams, packet scheduling, and sk_buff usage
> to virtio vsock.
> 
> The usage of struct sk_buff benefits users by a) preparing vsock to use
> other related systems that require sk_buff, such as sockmap and qdisc,
> b) supporting basic congestion control via sock_alloc_send_skb, and c)
> reducing copying when delivering packets to TAP.
> 
> The socket layer no longer forces errors to be -ENOMEM, as typically
> userspace expects -EAGAIN when the sk_sndbuf threshold is reached and
> messages are being sent with option MSG_DONTWAIT.
> 
> The datagram work is based off previous patches by Jiang Wang[1].
> 
> The introduction of datagrams creates a transport layer fairness issue
> where datagrams may freely starve streams of queue access. This happens
> because, unlike streams, datagrams lack the transactions necessary for
> calculating credits and throttling.
> 
> Previous proposals introduce changes to the spec to add an additional
> virtqueue pair for datagrams[1]. Although this solution works, using
> Linux's qdisc for packet scheduling leverages already existing systems,
> avoids the need to change the virtio specification, and gives additional
> capabilities. The usage of SFQ or fq_codel, for example, may solve the
> transport layer starvation problem. It is easy to imagine other use
> cases as well. For example, services of varying importance may be
> assigned different priorities, and qdisc will apply appropriate
> priority-based scheduling. By default, the system default pfifo qdisc is
> used. The qdisc may be bypassed and legacy queuing is resumed by simply
> setting the virtio-vsock%d network device to state DOWN. This technique
> still allows vsock to work with zero-configuration.

The basic question to answer then is this: with a net device qdisc
etc in the picture, how is this different from virtio net then?
Why do you still want to use vsock?

> In summary, this series introduces these major changes to vsock:
> 
> - virtio vsock supports datagrams
> - virtio vsock uses struct sk_buff instead of virtio_vsock_pkt
>   - Because virtio vsock uses sk_buff, it also uses sock_alloc_send_skb,
>     which applies the throttling threshold sk_sndbuf.
> - The vsock socket layer supports returning errors other than -ENOMEM.
>   - This is used to return -EAGAIN when the sk_sndbuf threshold is
>     reached.
> - virtio vsock uses a net_device, through which qdisc may be used.
>  - qdisc allows scheduling policies to be applied to vsock flows.
>   - Some qdiscs, like SFQ, may allow vsock to avoid transport layer congestion. That is,
>     it may avoid datagrams from flooding out stream flows. The benefit
>     to this is that additional virtqueues are not needed for datagrams.
>   - The net_device and qdisc is bypassed by simply setting the
>     net_device state to DOWN.
> 
> [1]: https://lore.kernel.org/all/20210914055440.3121004-1-jiang.wang@bytedance.com/
> 
> Bobby Eshleman (5):
>   vsock: replace virtio_vsock_pkt with sk_buff
>   vsock: return errors other than -ENOMEM to socket
>   vsock: add netdev to vhost/virtio vsock
>   virtio/vsock: add VIRTIO_VSOCK_F_DGRAM feature bit
>   virtio/vsock: add support for dgram
> 
> Jiang Wang (1):
>   vsock_test: add tests for vsock dgram
> 
>  drivers/vhost/vsock.c                   | 238 ++++----
>  include/linux/virtio_vsock.h            |  73 ++-
>  include/net/af_vsock.h                  |   2 +
>  include/uapi/linux/virtio_vsock.h       |   2 +
>  net/vmw_vsock/af_vsock.c                |  30 +-
>  net/vmw_vsock/hyperv_transport.c        |   2 +-
>  net/vmw_vsock/virtio_transport.c        | 237 +++++---
>  net/vmw_vsock/virtio_transport_common.c | 771 ++++++++++++++++--------
>  net/vmw_vsock/vmci_transport.c          |   9 +-
>  net/vmw_vsock/vsock_loopback.c          |  51 +-
>  tools/testing/vsock/util.c              | 105 ++++
>  tools/testing/vsock/util.h              |   4 +
>  tools/testing/vsock/vsock_test.c        | 195 ++++++
>  13 files changed, 1176 insertions(+), 543 deletions(-)
> 
> -- 
> 2.35.1

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Michael S. Tsirkin]

On Tue, Aug 16, 2022 at 09:42:51AM +0000, Bobby Eshleman wrote:
> > The basic question to answer then is this: with a net device qdisc
> > etc in the picture, how is this different from virtio net then?
> > Why do you still want to use vsock?
> > 
> 
> When using virtio-net, users looking for inter-VM communication are
> required to setup bridges, TAPs, allocate IP addresses or setup DNS,
> etc... and then finally when you have a network, you can open a socket
> on an IP address and port. This is the configuration that vsock avoids.
> For vsock, we just need a CID and a port, but no network configuration.

Surely when you mention DNS you are going overboard? vsock doesn't
remove the need for DNS as much as it does not support it.

-- 
MST

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Michael S. Tsirkin]

On Tue, Aug 16, 2022 at 11:08:26AM +0000, Bobby Eshleman wrote:
> On Wed, Aug 17, 2022 at 01:02:52PM -0400, Michael S. Tsirkin wrote:
> > On Tue, Aug 16, 2022 at 09:42:51AM +0000, Bobby Eshleman wrote:
> > > > The basic question to answer then is this: with a net device qdisc
> > > > etc in the picture, how is this different from virtio net then?
> > > > Why do you still want to use vsock?
> > > > 
> > > 
> > > When using virtio-net, users looking for inter-VM communication are
> > > required to setup bridges, TAPs, allocate IP addresses or setup DNS,
> > > etc... and then finally when you have a network, you can open a socket
> > > on an IP address and port. This is the configuration that vsock avoids.
> > > For vsock, we just need a CID and a port, but no network configuration.
> > 
> > Surely when you mention DNS you are going overboard? vsock doesn't
> > remove the need for DNS as much as it does not support it.
> > 
> 
> Oops, s/DNS/dhcp.

That too.

-- 
MST

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Jason Wang]

在 2022/8/17 14:54, Michael S. Tsirkin 写道:
> On Mon, Aug 15, 2022 at 10:56:03AM -0700, Bobby Eshleman wrote:
>> Hey everybody,
>>
>> This series introduces datagrams, packet scheduling, and sk_buff usage
>> to virtio vsock.
>>
>> The usage of struct sk_buff benefits users by a) preparing vsock to use
>> other related systems that require sk_buff, such as sockmap and qdisc,
>> b) supporting basic congestion control via sock_alloc_send_skb, and c)
>> reducing copying when delivering packets to TAP.
>>
>> The socket layer no longer forces errors to be -ENOMEM, as typically
>> userspace expects -EAGAIN when the sk_sndbuf threshold is reached and
>> messages are being sent with option MSG_DONTWAIT.
>>
>> The datagram work is based off previous patches by Jiang Wang[1].
>>
>> The introduction of datagrams creates a transport layer fairness issue
>> where datagrams may freely starve streams of queue access. This happens
>> because, unlike streams, datagrams lack the transactions necessary for
>> calculating credits and throttling.
>>
>> Previous proposals introduce changes to the spec to add an additional
>> virtqueue pair for datagrams[1]. Although this solution works, using
>> Linux's qdisc for packet scheduling leverages already existing systems,
>> avoids the need to change the virtio specification, and gives additional
>> capabilities. The usage of SFQ or fq_codel, for example, may solve the
>> transport layer starvation problem. It is easy to imagine other use
>> cases as well. For example, services of varying importance may be
>> assigned different priorities, and qdisc will apply appropriate
>> priority-based scheduling. By default, the system default pfifo qdisc is
>> used. The qdisc may be bypassed and legacy queuing is resumed by simply
>> setting the virtio-vsock%d network device to state DOWN. This technique
>> still allows vsock to work with zero-configuration.
> The basic question to answer then is this: with a net device qdisc
> etc in the picture, how is this different from virtio net then?
> Why do you still want to use vsock?


Or maybe it's time to revisit an old idea[1] to unify at least the 
driver part (e.g using virtio-net driver for vsock then we can all 
features that vsock is lacking now)?

Thanks

[1] 
https://lists.linuxfoundation.org/pipermail/virtualization/2018-November/039783.html


>
>> In summary, this series introduces these major changes to vsock:
>>
>> - virtio vsock supports datagrams
>> - virtio vsock uses struct sk_buff instead of virtio_vsock_pkt
>>    - Because virtio vsock uses sk_buff, it also uses sock_alloc_send_skb,
>>      which applies the throttling threshold sk_sndbuf.
>> - The vsock socket layer supports returning errors other than -ENOMEM.
>>    - This is used to return -EAGAIN when the sk_sndbuf threshold is
>>      reached.
>> - virtio vsock uses a net_device, through which qdisc may be used.
>>   - qdisc allows scheduling policies to be applied to vsock flows.
>>    - Some qdiscs, like SFQ, may allow vsock to avoid transport layer congestion. That is,
>>      it may avoid datagrams from flooding out stream flows. The benefit
>>      to this is that additional virtqueues are not needed for datagrams.
>>    - The net_device and qdisc is bypassed by simply setting the
>>      net_device state to DOWN.
>>
>> [1]: https://lore.kernel.org/all/20210914055440.3121004-1-jiang.wang@bytedance.com/
>>
>> Bobby Eshleman (5):
>>    vsock: replace virtio_vsock_pkt with sk_buff
>>    vsock: return errors other than -ENOMEM to socket
>>    vsock: add netdev to vhost/virtio vsock
>>    virtio/vsock: add VIRTIO_VSOCK_F_DGRAM feature bit
>>    virtio/vsock: add support for dgram
>>
>> Jiang Wang (1):
>>    vsock_test: add tests for vsock dgram
>>
>>   drivers/vhost/vsock.c                   | 238 ++++----
>>   include/linux/virtio_vsock.h            |  73 ++-
>>   include/net/af_vsock.h                  |   2 +
>>   include/uapi/linux/virtio_vsock.h       |   2 +
>>   net/vmw_vsock/af_vsock.c                |  30 +-
>>   net/vmw_vsock/hyperv_transport.c        |   2 +-
>>   net/vmw_vsock/virtio_transport.c        | 237 +++++---
>>   net/vmw_vsock/virtio_transport_common.c | 771 ++++++++++++++++--------
>>   net/vmw_vsock/vmci_transport.c          |   9 +-
>>   net/vmw_vsock/vsock_loopback.c          |  51 +-
>>   tools/testing/vsock/util.c              | 105 ++++
>>   tools/testing/vsock/util.h              |   4 +
>>   tools/testing/vsock/vsock_test.c        | 195 ++++++
>>   13 files changed, 1176 insertions(+), 543 deletions(-)
>>
>> -- 
>> 2.35.1

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Bobby Eshleman]

On Tue, Sep 06, 2022 at 09:26:33AM -0400, Stefan Hajnoczi wrote:
> Hi Bobby,
> If you are attending Linux Foundation conferences in Dublin, Ireland
> next week (Linux Plumbers Conference, Open Source Summit Europe, KVM
> Forum, ContainerCon Europe, CloudOpen Europe, etc) then you could meet
> Stefano Garzarella and others to discuss this patch series.
> 
> Using netdev and sk_buff is a big change to vsock. Discussing your
> requirements and the future direction of vsock in person could help.
> 
> If you won't be in Dublin, don't worry. You can schedule a video call if
> you feel it would be helpful to discuss these topics.
> 
> Stefan

Hey Stefan,

That sounds like a great idea! I was unable to make the Dublin trip work
so I think a video call would be best, of course if okay with everyone.

Thanks,
Bobby

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Stefano Garzarella]

On Thu, Aug 18, 2022 at 12:28:48PM +0800, Jason Wang wrote:
>
>在 2022/8/17 14:54, Michael S. Tsirkin 写道:
>>On Mon, Aug 15, 2022 at 10:56:03AM -0700, Bobby Eshleman wrote:
>>>Hey everybody,
>>>
>>>This series introduces datagrams, packet scheduling, and sk_buff usage
>>>to virtio vsock.
>>>
>>>The usage of struct sk_buff benefits users by a) preparing vsock to use
>>>other related systems that require sk_buff, such as sockmap and qdisc,
>>>b) supporting basic congestion control via sock_alloc_send_skb, and c)
>>>reducing copying when delivering packets to TAP.
>>>
>>>The socket layer no longer forces errors to be -ENOMEM, as typically
>>>userspace expects -EAGAIN when the sk_sndbuf threshold is reached and
>>>messages are being sent with option MSG_DONTWAIT.
>>>
>>>The datagram work is based off previous patches by Jiang Wang[1].
>>>
>>>The introduction of datagrams creates a transport layer fairness issue
>>>where datagrams may freely starve streams of queue access. This happens
>>>because, unlike streams, datagrams lack the transactions necessary for
>>>calculating credits and throttling.
>>>
>>>Previous proposals introduce changes to the spec to add an additional
>>>virtqueue pair for datagrams[1]. Although this solution works, using
>>>Linux's qdisc for packet scheduling leverages already existing systems,
>>>avoids the need to change the virtio specification, and gives additional
>>>capabilities. The usage of SFQ or fq_codel, for example, may solve the
>>>transport layer starvation problem. It is easy to imagine other use
>>>cases as well. For example, services of varying importance may be
>>>assigned different priorities, and qdisc will apply appropriate
>>>priority-based scheduling. By default, the system default pfifo qdisc is
>>>used. The qdisc may be bypassed and legacy queuing is resumed by simply
>>>setting the virtio-vsock%d network device to state DOWN. This technique
>>>still allows vsock to work with zero-configuration.
>>The basic question to answer then is this: with a net device qdisc
>>etc in the picture, how is this different from virtio net then?
>>Why do you still want to use vsock?
>
>
>Or maybe it's time to revisit an old idea[1] to unify at least the 
>driver part (e.g using virtio-net driver for vsock then we can all 
>features that vsock is lacking now)?

Sorry for coming late to the discussion!

This would be great, though, last time I had looked at it, I had found 
it quite complicated. The main problem is trying to avoid all the 
net-specific stuff (MTU, ethernet header, HW offloading, etc.).

Maybe we could start thinking about this idea by adding a new transport 
to vsock (e.g. virtio-net-vsock) completely separate from what we have 
now.

Thanks,
Stefano

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

Hi Bobby,
If you are attending Linux Foundation conferences in Dublin, Ireland
next week (Linux Plumbers Conference, Open Source Summit Europe, KVM
Forum, ContainerCon Europe, CloudOpen Europe, etc) then you could meet
Stefano Garzarella and others to discuss this patch series.

Using netdev and sk_buff is a big change to vsock. Discussing your
requirements and the future direction of vsock in person could help.

If you won't be in Dublin, don't worry. You can schedule a video call if
you feel it would be helpful to discuss these topics.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Stefano Garzarella]

On Thu, Aug 18, 2022 at 02:39:32PM +0000, Bobby Eshleman wrote:
>On Tue, Sep 06, 2022 at 09:26:33AM -0400, Stefan Hajnoczi wrote:
>> Hi Bobby,
>> If you are attending Linux Foundation conferences in Dublin, Ireland
>> next week (Linux Plumbers Conference, Open Source Summit Europe, KVM
>> Forum, ContainerCon Europe, CloudOpen Europe, etc) then you could meet
>> Stefano Garzarella and others to discuss this patch series.
>>
>> Using netdev and sk_buff is a big change to vsock. Discussing your
>> requirements and the future direction of vsock in person could help.
>>
>> If you won't be in Dublin, don't worry. You can schedule a video call if
>> you feel it would be helpful to discuss these topics.
>>
>> Stefan
>
>Hey Stefan,
>
>That sounds like a great idea!

Yep, I agree!

>I was unable to make the Dublin trip work
>so I think a video call would be best, of course if okay with everyone.

It will work for me, but I'll be a bit busy in the next 2 weeks:

 From Sep 12 to Sep 14 I'll be at KVM Forum, so it may be difficult to 
arrange, but we can try.

Sep 15 I'm not available.
Sep 16 I'm traveling, but early in my morning, so I should be available.

Form Sep 10 to Sep 23 I'll be mostly off, but I can try to find some 
slots if needed.

 From Sep 26 I'm back and fully available.

Let's see if others are available and try to find a slot :-)

Thanks,
Stefano

Call to discuss vsock netdev/sk_buff [was Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc]

[Stefano Garzarella]

On Thu, Aug 18, 2022 at 02:39:32PM +0000, Bobby Eshleman wrote:
>On Tue, Sep 06, 2022 at 09:26:33AM -0400, Stefan Hajnoczi wrote:
>> Hi Bobby,
>> If you are attending Linux Foundation conferences in Dublin, Ireland
>> next week (Linux Plumbers Conference, Open Source Summit Europe, KVM
>> Forum, ContainerCon Europe, CloudOpen Europe, etc) then you could meet
>> Stefano Garzarella and others to discuss this patch series.
>>
>> Using netdev and sk_buff is a big change to vsock. Discussing your
>> requirements and the future direction of vsock in person could help.
>>
>> If you won't be in Dublin, don't worry. You can schedule a video call if
>> you feel it would be helpful to discuss these topics.
>>
>> Stefan
>
>Hey Stefan,
>
>That sounds like a great idea! I was unable to make the Dublin trip work
>so I think a video call would be best, of course if okay with everyone.

Looking better at the KVM forum sched, I found 1h slot for Sep 15 at 
16:30 UTC.

Could this work for you?

It would be nice to also have HyperV and VMCI people in the call and 
anyone else who is interested of course.

@Dexuan @Bryan @Vishnu can you attend?

@MST @Jason @Stefan if you can be there that would be great, we could 
connect together from Dublin.

Thanks,
Stefano

Re: Call to discuss vsock netdev/sk_buff [was Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc]

[Bobby Eshleman]

Hey Stefano, thanks for sending this out.

On Thu, Sep 08, 2022 at 04:36:52PM +0200, Stefano Garzarella wrote:
> 
> Looking better at the KVM forum sched, I found 1h slot for Sep 15 at 16:30
> UTC.
> 
> Could this work for you?

Unfortunately, I can't make this time slot.

My schedule also opens up a lot the week of the 26th, especially between
16:00 and 19:00 UTC, as well as after 22:00 UTC.

Best,
Bobby

Re: Call to discuss vsock netdev/sk_buff [was Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc]

[Bobby Eshleman]

On Mon, Sep 12, 2022 at 08:12:58PM +0200, Stefano Garzarella wrote:
> On Fri, Sep 9, 2022 at 8:13 PM Bobby Eshleman <bobbyeshleman@gmail.com> wrote:
> >
> > Hey Stefano, thanks for sending this out.
> >
> > On Thu, Sep 08, 2022 at 04:36:52PM +0200, Stefano Garzarella wrote:
> > >
> > > Looking better at the KVM forum sched, I found 1h slot for Sep 15 at 16:30
> > > UTC.
> > >
> > > Could this work for you?
> >
> > Unfortunately, I can't make this time slot.
> 
> No problem at all!
> 
> >
> > My schedule also opens up a lot the week of the 26th, especially between
> > 16:00 and 19:00 UTC, as well as after 22:00 UTC.
> 
> Great, that week works for me too.
> What about Sep 27 @ 16:00 UTC?
> 

That time works for me!

Thanks,
Bobby

Re: Call to discuss vsock netdev/sk_buff [was Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc]

[Bobby Eshleman]

On Fri, Sep 16, 2022 at 05:51:22AM +0200, Stefano Garzarella wrote:
> On Mon, Sep 12, 2022 at 8:28 PM Bobby Eshleman <bobbyeshleman@gmail.com> wrote:
> >
> > On Mon, Sep 12, 2022 at 08:12:58PM +0200, Stefano Garzarella wrote:
> > > On Fri, Sep 9, 2022 at 8:13 PM Bobby Eshleman <bobbyeshleman@gmail.com> wrote:
> > > >
> > > > Hey Stefano, thanks for sending this out.
> > > >
> > > > On Thu, Sep 08, 2022 at 04:36:52PM +0200, Stefano Garzarella wrote:
> > > > >
> > > > > Looking better at the KVM forum sched, I found 1h slot for Sep 15 at 16:30
> > > > > UTC.
> > > > >
> > > > > Could this work for you?
> > > >
> > > > Unfortunately, I can't make this time slot.
> > >
> > > No problem at all!
> > >
> > > >
> > > > My schedule also opens up a lot the week of the 26th, especially between
> > > > 16:00 and 19:00 UTC, as well as after 22:00 UTC.
> > >
> > > Great, that week works for me too.
> > > What about Sep 27 @ 16:00 UTC?
> > >
> >
> > That time works for me!
> 
> Great! I sent you an invitation.
> 

Awesome, see you then!

Thanks,
Bobby

Re: Call to discuss vsock netdev/sk_buff [was Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc]

[Stefano Garzarella]

On Fri, Sep 9, 2022 at 8:13 PM Bobby Eshleman <bobbyeshleman@gmail.com> wrote:
>
> Hey Stefano, thanks for sending this out.
>
> On Thu, Sep 08, 2022 at 04:36:52PM +0200, Stefano Garzarella wrote:
> >
> > Looking better at the KVM forum sched, I found 1h slot for Sep 15 at 16:30
> > UTC.
> >
> > Could this work for you?
>
> Unfortunately, I can't make this time slot.

No problem at all!

>
> My schedule also opens up a lot the week of the 26th, especially between
> 16:00 and 19:00 UTC, as well as after 22:00 UTC.

Great, that week works for me too.
What about Sep 27 @ 16:00 UTC?

Thanks,
Stefano

Re: Call to discuss vsock netdev/sk_buff [was Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc]

[Stefano Garzarella]

On Mon, Sep 12, 2022 at 8:28 PM Bobby Eshleman <bobbyeshleman@gmail.com> wrote:
>
> On Mon, Sep 12, 2022 at 08:12:58PM +0200, Stefano Garzarella wrote:
> > On Fri, Sep 9, 2022 at 8:13 PM Bobby Eshleman <bobbyeshleman@gmail.com> wrote:
> > >
> > > Hey Stefano, thanks for sending this out.
> > >
> > > On Thu, Sep 08, 2022 at 04:36:52PM +0200, Stefano Garzarella wrote:
> > > >
> > > > Looking better at the KVM forum sched, I found 1h slot for Sep 15 at 16:30
> > > > UTC.
> > > >
> > > > Could this work for you?
> > >
> > > Unfortunately, I can't make this time slot.
> >
> > No problem at all!
> >
> > >
> > > My schedule also opens up a lot the week of the 26th, especially between
> > > 16:00 and 19:00 UTC, as well as after 22:00 UTC.
> >
> > Great, that week works for me too.
> > What about Sep 27 @ 16:00 UTC?
> >
>
> That time works for me!

Great! I sent you an invitation.

For others that want to join the discussion, we will meet Sep 27 @
16:00 UTC at this room: https://meet.google.com/fxi-vuzr-jjb

Thanks,
Stefano

Re: [PATCH 2/6] vsock: return errors other than -ENOMEM to socket

[Stefano Garzarella]

On Mon, Aug 15, 2022 at 10:56:05AM -0700, Bobby Eshleman wrote:
>This commit allows vsock implementations to return errors
>to the socket layer other than -ENOMEM. One immediate effect
>of this is that upon the sk_sndbuf threshold being reached -EAGAIN
>will be returned and userspace may throttle appropriately.
>
>Resultingly, a known issue with uperf is resolved[1].
>
>Additionally, to preserve legacy behavior for non-virtio
>implementations, hyperv/vmci force errors to be -ENOMEM so that behavior
>is unchanged.
>
>[1]: https://gitlab.com/vsock/vsock/-/issues/1
>
>Signed-off-by: Bobby Eshleman <bobby.eshleman@bytedance.com>
>---
> include/linux/virtio_vsock.h            | 3 +++
> net/vmw_vsock/af_vsock.c                | 3 ++-
> net/vmw_vsock/hyperv_transport.c        | 2 +-
> net/vmw_vsock/virtio_transport_common.c | 3 ---
> net/vmw_vsock/vmci_transport.c          | 9 ++++++++-
> 5 files changed, 14 insertions(+), 6 deletions(-)
>
>diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>index 17ed01466875..9a37eddbb87a 100644
>--- a/include/linux/virtio_vsock.h
>+++ b/include/linux/virtio_vsock.h
>@@ -8,6 +8,9 @@
> #include <net/sock.h>
> #include <net/af_vsock.h>
>
>+/* Threshold for detecting small packets to copy */
>+#define GOOD_COPY_LEN  128
>+

This change seems unrelated.

Please move it in the patch where you need this.
Maybe it's better to add a prefix if we move it in an header file (e.g.  
VIRTIO_VSOCK_...).

Thanks,
Stefano

> enum virtio_vsock_metadata_flags {
> 	VIRTIO_VSOCK_METADATA_FLAGS_REPLY		= BIT(0),
> 	VIRTIO_VSOCK_METADATA_FLAGS_TAP_DELIVERED	= BIT(1),
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index e348b2d09eac..1893f8aafa48 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -1844,8 +1844,9 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg,
> 			written = transport->stream_enqueue(vsk,
> 					msg, len - total_written);
> 		}
>+
> 		if (written < 0) {
>-			err = -ENOMEM;
>+			err = written;
> 			goto out_err;
> 		}
>
>diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
>index fd98229e3db3..e99aea571f6f 100644
>--- a/net/vmw_vsock/hyperv_transport.c
>+++ b/net/vmw_vsock/hyperv_transport.c
>@@ -687,7 +687,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk, struct msghdr *msg,
> 	if (bytes_written)
> 		ret = bytes_written;
> 	kfree(send_buf);
>-	return ret;
>+	return ret < 0 ? -ENOMEM : ret;
> }
>
> static s64 hvs_stream_has_data(struct vsock_sock *vsk)
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 920578597bb9..d5780599fe93 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -23,9 +23,6 @@
> /* How long to wait for graceful shutdown of a connection */
> #define VSOCK_CLOSE_TIMEOUT (8 * HZ)
>
>-/* Threshold for detecting small packets to copy */
>-#define GOOD_COPY_LEN  128
>-
> static const struct virtio_transport *
> virtio_transport_get_ops(struct vsock_sock *vsk)
> {
>diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
>index b14f0ed7427b..c927a90dc859 100644
>--- a/net/vmw_vsock/vmci_transport.c
>+++ b/net/vmw_vsock/vmci_transport.c
>@@ -1838,7 +1838,14 @@ static ssize_t vmci_transport_stream_enqueue(
> 	struct msghdr *msg,
> 	size_t len)
> {
>-	return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
>+	int err;
>+
>+	err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
>+
>+	if (err < 0)
>+		err = -ENOMEM;
>+
>+	return err;
> }
>
> static s64 vmci_transport_stream_has_data(struct vsock_sock *vsk)
>-- 
>2.35.1
>

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Stefano Garzarella]

Hi,

On Mon, Aug 15, 2022 at 10:56:03AM -0700, Bobby Eshleman wrote:
>Hey everybody,
>
>This series introduces datagrams, packet scheduling, and sk_buff usage
>to virtio vsock.

Just a reminder for those who are interested, tomorrow Sep 27 @ 16:00 
UTC we will discuss more about the next steps for this series in this 
room: https://meet.google.com/fxi-vuzr-jjb
(I'll try to record it and take notes that we will share)

Bobby, thank you so much for working on this! It would be great to solve 
the fairness issue and support datagram!

I took a look at the series, left some comments in the individual 
patches, and add some advice here that we could pick up tomorrow:
- it would be nice to run benchmarks (e.g., iperf-vsock, uperf, etc.) to
   see how much the changes cost (e.g. sk_buff use)
- we should take care also of other transports (i.e. vmci, hyperv), the 
   uAPI should be as close as possible regardless of the transport

About the use of netdev, it seems the most controversial point and I 
understand Jakub and Michael's concerns. Tomorrow would be great if you 
can update us if you have found any way to avoid it, just reusing a 
packet scheduler somehow.
It would be great if we could make it available for all transports (I'm 
not asking you to implement it for all, but to have a generic api that 
others can use).

But we can talk about that tomorrow!
Thanks,
Stefano

>
>The usage of struct sk_buff benefits users by a) preparing vsock to use
>other related systems that require sk_buff, such as sockmap and qdisc,
>b) supporting basic congestion control via sock_alloc_send_skb, and c)
>reducing copying when delivering packets to TAP.
>
>The socket layer no longer forces errors to be -ENOMEM, as typically
>userspace expects -EAGAIN when the sk_sndbuf threshold )s reached and
>messages are being sent with option MSG_DONTWAIT.
>
>The datagram work is based off previous patches by Jiang Wang[1].
>
>The introduction of datagrams creates a transport layer fairness issue
>where datagrams may freely starve streams of queue access. This happens
>because, unlike streams, datagrams lack the transactions necessary for
>calculating credits and throttling.
>
>Previous proposals introduce changes to the spec to add an additional
>virtqueue pair for datagrams[1]. Although this solution works, using
>Linux's qdisc for packet scheduling leverages already existing systems,
>avoids the need to change the virtio specification, and gives additional
>capabilities. The usage of SFQ or fq_codel, for example, may solve the
>transport layer starvation problem. It is easy to imagine other use
>cases as well. For example, services of varying importance may be
>assigned different priorities, and qdisc will apply appropriate
>priority-based scheduling. By default, the system default pfifo qdisc is
>used. The qdisc may be bypassed and legacy queuing is resumed by simply
>setting the virtio-vsock%d network device to state DOWN. This technique
>still allows vsock to work with zero-configuration.
>
>In summary, this series introduces these major changes to vsock:
>
>- virtio vsock supports datagrams
>- virtio vsock uses struct sk_buff instead of virtio_vsock_pkt
>  - Because virtio vsock uses sk_buff, it also uses sock_alloc_send_skb,
>    which applies the throttling threshold sk_sndbuf.
>- The vsock socket layer supports returning errors other than -ENOMEM.
>  - This is used to return -EAGAIN when the sk_sndbuf threshold is
>    reached.
>- virtio vsock uses a net_device, through which qdisc may be used.
> - qdisc allows scheduling policies to be applied to vsock flows.
>  - Some qdiscs, like SFQ, may allow vsock to avoid transport layer congestion. That is,
>    it may avoid datagrams from flooding out stream flows. The benefit
>    to this is that additional virtqueues are not needed for datagrams.
>  - The net_device and qdisc is bypassed by simply setting the
>    net_device state to DOWN.
>
>[1]: https://lore.kernel.org/all/20210914055440.3121004-1-jiang.wang@bytedance.com/
>
>Bobby Eshleman (5):
>  vsock: replace virtio_vsock_pkt with sk_buff
>  vsock: return errors other than -ENOMEM to socket
>  vsock: add netdev to vhost/virtio vsock
>  virtio/vsock: add VIRTIO_VSOCK_F_DGRAM feature bit
>  virtio/vsock: add support for dgram
>
>Jiang Wang (1):
>  vsock_test: add tests for vsock dgram
>
> drivers/vhost/vsock.c                   | 238 ++++----
> include/linux/virtio_vsock.h            |  73 ++-
> include/net/af_vsock.h                  |   2 +
> include/uapi/linux/virtio_vsock.h       |   2 +
> net/vmw_vsock/af_vsock.c                |  30 +-
> net/vmw_vsock/hyperv_transport.c        |   2 +-
> net/vmw_vsock/virtio_transport.c        | 237 +++++---
> net/vmw_vsock/virtio_transport_common.c | 771 ++++++++++++++++--------
> net/vmw_vsock/vmci_transport.c          |   9 +-
> net/vmw_vsock/vsock_loopback.c          |  51 +-
> tools/testing/vsock/util.c              | 105 ++++
> tools/testing/vsock/util.h              |   4 +
> tools/testing/vsock/vsock_test.c        | 195 ++++++
> 13 files changed, 1176 insertions(+), 543 deletions(-)
>
>-- 
>2.35.1
>

Re: [PATCH 2/6] vsock: return errors other than -ENOMEM to socket

[Bobby Eshleman]

On Mon, Sep 26, 2022 at 03:21:45PM +0200, Stefano Garzarella wrote:
> On Mon, Aug 15, 2022 at 10:56:05AM -0700, Bobby Eshleman wrote:
> > This commit allows vsock implementations to return errors
> > to the socket layer other than -ENOMEM. One immediate effect
> > of this is that upon the sk_sndbuf threshold being reached -EAGAIN
> > will be returned and userspace may throttle appropriately.
> > 
> > Resultingly, a known issue with uperf is resolved[1].
> > 
> > Additionally, to preserve legacy behavior for non-virtio
> > implementations, hyperv/vmci force errors to be -ENOMEM so that behavior
> > is unchanged.
> > 
> > [1]: https://gitlab.com/vsock/vsock/-/issues/1
> > 
> > Signed-off-by: Bobby Eshleman <bobby.eshleman@bytedance.com>
> > ---
> > include/linux/virtio_vsock.h            | 3 +++
> > net/vmw_vsock/af_vsock.c                | 3 ++-
> > net/vmw_vsock/hyperv_transport.c        | 2 +-
> > net/vmw_vsock/virtio_transport_common.c | 3 ---
> > net/vmw_vsock/vmci_transport.c          | 9 ++++++++-
> > 5 files changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> > index 17ed01466875..9a37eddbb87a 100644
> > --- a/include/linux/virtio_vsock.h
> > +++ b/include/linux/virtio_vsock.h
> > @@ -8,6 +8,9 @@
> > #include <net/sock.h>
> > #include <net/af_vsock.h>
> > 
> > +/* Threshold for detecting small packets to copy */
> > +#define GOOD_COPY_LEN  128
> > +
> 
> This change seems unrelated.
> 
> Please move it in the patch where you need this.
> Maybe it's better to add a prefix if we move it in an header file (e.g.
> VIRTIO_VSOCK_...).
> 
> Thanks,
> Stefano
> 

Oh yes, definitely.

Thanks,
Bobby

> > enum virtio_vsock_metadata_flags {
> > 	VIRTIO_VSOCK_METADATA_FLAGS_REPLY		= BIT(0),
> > 	VIRTIO_VSOCK_METADATA_FLAGS_TAP_DELIVERED	= BIT(1),
> > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > index e348b2d09eac..1893f8aafa48 100644
> > --- a/net/vmw_vsock/af_vsock.c
> > +++ b/net/vmw_vsock/af_vsock.c
> > @@ -1844,8 +1844,9 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg,
> > 			written = transport->stream_enqueue(vsk,
> > 					msg, len - total_written);
> > 		}
> > +
> > 		if (written < 0) {
> > -			err = -ENOMEM;
> > +			err = written;
> > 			goto out_err;
> > 		}
> > 
> > diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> > index fd98229e3db3..e99aea571f6f 100644
> > --- a/net/vmw_vsock/hyperv_transport.c
> > +++ b/net/vmw_vsock/hyperv_transport.c
> > @@ -687,7 +687,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk, struct msghdr *msg,
> > 	if (bytes_written)
> > 		ret = bytes_written;
> > 	kfree(send_buf);
> > -	return ret;
> > +	return ret < 0 ? -ENOMEM : ret;
> > }
> > 
> > static s64 hvs_stream_has_data(struct vsock_sock *vsk)
> > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > index 920578597bb9..d5780599fe93 100644
> > --- a/net/vmw_vsock/virtio_transport_common.c
> > +++ b/net/vmw_vsock/virtio_transport_common.c
> > @@ -23,9 +23,6 @@
> > /* How long to wait for graceful shutdown of a connection */
> > #define VSOCK_CLOSE_TIMEOUT (8 * HZ)
> > 
> > -/* Threshold for detecting small packets to copy */
> > -#define GOOD_COPY_LEN  128
> > -
> > static const struct virtio_transport *
> > virtio_transport_get_ops(struct vsock_sock *vsk)
> > {
> > diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> > index b14f0ed7427b..c927a90dc859 100644
> > --- a/net/vmw_vsock/vmci_transport.c
> > +++ b/net/vmw_vsock/vmci_transport.c
> > @@ -1838,7 +1838,14 @@ static ssize_t vmci_transport_stream_enqueue(
> > 	struct msghdr *msg,
> > 	size_t len)
> > {
> > -	return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
> > +	int err;
> > +
> > +	err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
> > +
> > +	if (err < 0)
> > +		err = -ENOMEM;
> > +
> > +	return err;
> > }
> > 
> > static s64 vmci_transport_stream_has_data(struct vsock_sock *vsk)
> > -- 
> > 2.35.1
> > 
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Bobby Eshleman]

On Mon, Sep 26, 2022 at 03:42:19PM +0200, Stefano Garzarella wrote:
> Hi,
> 
> On Mon, Aug 15, 2022 at 10:56:03AM -0700, Bobby Eshleman wrote:
> > Hey everybody,
> > 
> > This series introduces datagrams, packet scheduling, and sk_buff usage
> > to virtio vsock.
> 
> Just a reminder for those who are interested, tomorrow Sep 27 @ 16:00 UTC we
> will discuss more about the next steps for this series in this room:
> https://meet.google.com/fxi-vuzr-jjb
> (I'll try to record it and take notes that we will share)
> 
> Bobby, thank you so much for working on this! It would be great to solve the
> fairness issue and support datagram!
>

I appreciate that, thanks!

> I took a look at the series, left some comments in the individual patches,
> and add some advice here that we could pick up tomorrow:
> - it would be nice to run benchmarks (e.g., iperf-vsock, uperf, etc.) to
>   see how much the changes cost (e.g. sk_buff use)
> - we should take care also of other transports (i.e. vmci, hyperv), the
> uAPI should be as close as possible regardless of the transport
>

Duly noted. I have some measurements with uperf, I'll put the data
together and send that out here.

Regarding the uAPI topic, I'll save that topic for our conversation
tomorrow as I think the netdev topic will weigh on it.

> About the use of netdev, it seems the most controversial point and I
> understand Jakub and Michael's concerns. Tomorrow would be great if you can
> update us if you have found any way to avoid it, just reusing a packet
> scheduler somehow.
> It would be great if we could make it available for all transports (I'm not
> asking you to implement it for all, but to have a generic api that others
> can use).
>
> But we can talk about that tomorrow!

Sounds good, talk to you then!

Best,
Bobby

Re: [PATCH 0/6] virtio/vsock: introduce dgrams, sk_buff, and qdisc

[Stefano Garzarella]

On Mon, Sep 26, 2022 at 03:42:19PM +0200, Stefano Garzarella wrote:
>Hi,
>
>On Mon, Aug 15, 2022 at 10:56:03AM -0700, Bobby Eshleman wrote:
>>Hey everybody,
>>
>>This series introduces datagrams, packet scheduling, and sk_buff usage
>>to virtio vsock.
>
>Just a reminder for those who are interested, tomorrow Sep 27 @ 16:00 
>UTC we will discuss more about the next steps for this series in this 
>room: https://meet.google.com/fxi-vuzr-jjb
>(I'll try to record it and take notes that we will share)
>

Thank you all for participating in the call!
I'm attaching video/audio recording and notes (feel free to update it).

Notes: 
https://docs.google.com/document/d/14UHH0tEaBKfElLZjNkyKUs_HnOgHhZZBqIS86VEIqR0/edit?usp=sharing
Video recording: 
https://drive.google.com/file/d/1vUvTc_aiE1mB30tLPeJjANnb915-CIKa/view?usp=sharing

Thanks,
Stefano