| Message ID | 20210721143001.182009-1-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/1] virtio/vsock: Make vsock virtio packet buff size configurable | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | fail | Was 0 now: 1 |
| netdev/build_32bit | fail | Errors and warnings before: 7 this patch: 7 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable WARNING: line length of 87 exceeds 80 columns |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 7 this patch: 7 |
| netdev/header_inline | success | Link |
On Wed, Jul 21, 2021 at 03:30:00PM +0100, Lee Jones wrote: >From: Ram Muthiah <rammuthiah@google.com> > >After a virtual device has been running for some time, the SLAB >sustains ever increasing fragmentation. Contributing to this >fragmentation are the virtio packet buffer allocations which >are a drain on 64Kb compound pages. Eventually these can't be >allocated due to fragmentation. > >To enable successful allocations for this packet buffer, the >packet buffer's size needs to be reduced. > >In order to enable a reduction without impacting current users, >this variable is being exposed as a command line parameter. > >Cc: "Michael S. Tsirkin" <mst@redhat.com> >Cc: Jason Wang <jasowang@redhat.com> >Cc: Stefan Hajnoczi <stefanha@redhat.com> >Cc: Stefano Garzarella <sgarzare@redhat.com> >Cc: "David S. Miller" <davem@davemloft.net> >Cc: Jakub Kicinski <kuba@kernel.org> >Cc: virtualization@lists.linux-foundation.org >Cc: kvm@vger.kernel.org >Cc: netdev@vger.kernel.org >Signed-off-by: Ram Muthiah <rammuthiah@google.com> >Signed-off-by: Lee Jones <lee.jones@linaro.org> >--- > include/linux/virtio_vsock.h | 4 +++- > net/vmw_vsock/virtio_transport_common.c | 4 ++++ > 2 files changed, 7 insertions(+), 1 deletion(-) > >diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h >index 35d7eedb5e8e4..8c77d60a74d34 100644 >--- a/include/linux/virtio_vsock.h >+++ b/include/linux/virtio_vsock.h >@@ -7,9 +7,11 @@ > #include <net/sock.h> > #include <net/af_vsock.h> > >+extern uint virtio_transport_max_vsock_pkt_buf_size; >+ > #define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE (1024 * 4) > #define VIRTIO_VSOCK_MAX_BUF_SIZE 0xFFFFFFFFUL >-#define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (1024 * 64) >+#define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE virtio_transport_max_vsock_pkt_buf_size > > enum { > VSOCK_VQ_RX = 0, /* for host to guest data */ >diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c >index 169ba8b72a630..d0d913afec8b6 100644 >--- a/net/vmw_vsock/virtio_transport_common.c >+++ b/net/vmw_vsock/virtio_transport_common.c >@@ -26,6 +26,10 @@ > /* Threshold for detecting small packets to copy */ > #define GOOD_COPY_LEN 128 > >+uint virtio_transport_max_vsock_pkt_buf_size = 1024 * 64; >+module_param(virtio_transport_max_vsock_pkt_buf_size, uint, 0444); >+EXPORT_SYMBOL_GPL(virtio_transport_max_vsock_pkt_buf_size); >+ Maybe better to add an entry under sysfs similar to what Jiang proposed here: https://lists.linuxfoundation.org/pipermail/virtualization/2021-June/054769.html Thanks, Stefano
On Thu, Jul 22, 2021 at 02:55:19PM +0200, Stefano Garzarella wrote: > > > > +uint virtio_transport_max_vsock_pkt_buf_size = 1024 * 64; > > +module_param(virtio_transport_max_vsock_pkt_buf_size, uint, 0444); > > +EXPORT_SYMBOL_GPL(virtio_transport_max_vsock_pkt_buf_size); > > + I'm interested on this functionality, so I could take this on. > > Maybe better to add an entry under sysfs similar to what Jiang proposed > here: > https://lists.linuxfoundation.org/pipermail/virtualization/2021-June/054769.html Having a look at Jiang's RFC patch it seems the proposed sysfs node hangs off from the main kernel object e.g. /sys/kernel. So I wonder if there is a more appropriate parent for this knob? Also, I noticed that Ram's patch here is using read-only permissions for the module parameter and switching to sysfs would mean opening this knob up to be dynamically configured? I'd need to be careful here. -- Carlos Llamas
On Fri, Dec 09, 2022 at 07:48:02PM +0000, Carlos Llamas wrote: >On Thu, Jul 22, 2021 at 02:55:19PM +0200, Stefano Garzarella wrote: >> > >> > +uint virtio_transport_max_vsock_pkt_buf_size = 1024 * 64; >> > +module_param(virtio_transport_max_vsock_pkt_buf_size, uint, 0444); >> > +EXPORT_SYMBOL_GPL(virtio_transport_max_vsock_pkt_buf_size); >> > + > >I'm interested on this functionality, so I could take this on. Great! We are changing the packet handling using sk_buff [1], so I think it's better to rebase on that work that should be merged in net-next after the current merge window will close. > >> >> Maybe better to add an entry under sysfs similar to what Jiang proposed >> here: >> https://lists.linuxfoundation.org/pipermail/virtualization/2021-June/054769.html > >Having a look at Jiang's RFC patch it seems the proposed sysfs node >hangs off from the main kernel object e.g. /sys/kernel. So I wonder if >there is a more appropriate parent for this knob? Agree, what about /sys/devices ? I would take a closer look at what is recommend in this case. > >Also, I noticed that Ram's patch here is using read-only permissions for >the module parameter and switching to sysfs would mean opening this knob >up to be dynamically configured? I'd need to be careful here. > True, but even if it's changed while we're running, I don't think it's a big problem. Maybe the problem here would be the allocation of RX buffers made during the probe. Could this be a good reason to use a module parameter? Thanks, Stefano [1] https://lore.kernel.org/lkml/20221202173520.10428-1-bobby.eshleman@bytedance.com/
diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h index 35d7eedb5e8e4..8c77d60a74d34 100644 --- a/include/linux/virtio_vsock.h +++ b/include/linux/virtio_vsock.h @@ -7,9 +7,11 @@ #include <net/sock.h> #include <net/af_vsock.h> +extern uint virtio_transport_max_vsock_pkt_buf_size; + #define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE (1024 * 4) #define VIRTIO_VSOCK_MAX_BUF_SIZE 0xFFFFFFFFUL -#define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (1024 * 64) +#define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE virtio_transport_max_vsock_pkt_buf_size enum { VSOCK_VQ_RX = 0, /* for host to guest data */ diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index 169ba8b72a630..d0d913afec8b6 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -26,6 +26,10 @@ /* Threshold for detecting small packets to copy */ #define GOOD_COPY_LEN 128 +uint virtio_transport_max_vsock_pkt_buf_size = 1024 * 64; +module_param(virtio_transport_max_vsock_pkt_buf_size, uint, 0444); +EXPORT_SYMBOL_GPL(virtio_transport_max_vsock_pkt_buf_size); + static const struct virtio_transport * virtio_transport_get_ops(struct vsock_sock *vsk) {