| Message ID | d81818b868216c774613dd03641fcfe63cc55a45.1660362668.git.bobby.eshleman@bytedance.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio/vsock: introduce dgrams, sk_buff, and qdisc | expand |
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
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
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
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 >
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 >
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 >
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
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)
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(-)