Stefano Garzarella
2023-Sep-18 14:50 UTC
[PATCH net-next v9 4/4] vsock/virtio: MSG_ZEROCOPY flag support
On Sat, Sep 16, 2023 at 04:09:18PM +0300, Arseniy Krasnov wrote:>This adds handling of MSG_ZEROCOPY flag on transmission path: > >1) If this flag is set and zerocopy transmission is possible (enabled > in socket options and transport allows zerocopy), then non-linear > skb will be created and filled with the pages of user's buffer. > Pages of user's buffer are locked in memory by 'get_user_pages()'. >2) Replaces way of skb owning: instead of 'skb_set_owner_sk_safe()' it > calls 'skb_set_owner_w()'. Reason of this change is that > '__zerocopy_sg_from_iter()' increments 'sk_wmem_alloc' of socket, so > to decrease this field correctly, proper skb destructor is needed: > 'sock_wfree()'. This destructor is set by 'skb_set_owner_w()'. >3) Adds new callback to 'struct virtio_transport': 'can_msgzerocopy'. > If this callback is set, then transport needs extra check to be able > to send provided number of buffers in zerocopy mode. Currently, the > only transport that needs this callback set is virtio, because this > transport adds new buffers to the virtio queue and we need to check, > that number of these buffers is less than size of the queue (it is > required by virtio spec). vhost and loopback transports don't need > this check. > >Signed-off-by: Arseniy Krasnov <avkrasnov at salutedevices.com> >--- > Changelog: > v5(big patchset) -> v1: > * Refactorings of 'if' conditions. > * Remove extra blank line. > * Remove 'frag_off' field unneeded init. > * Add function 'virtio_transport_fill_skb()' which fills both linear > and non-linear skb with provided data. > v1 -> v2: > * Use original order of last four arguments in 'virtio_transport_alloc_skb()'. > v2 -> v3: > * Add new transport callback: 'msgzerocopy_check_iov'. It checks that > provided 'iov_iter' with data could be sent in a zerocopy mode. > If this callback is not set in transport - transport allows to send > any 'iov_iter' in zerocopy mode. Otherwise - if callback returns 'true' > then zerocopy is allowed. Reason of this callback is that in case of > G2H transmission we insert whole skb to the tx virtio queue and such > skb must fit to the size of the virtio queue to be sent in a single > iteration (may be tx logic in 'virtio_transport.c' could be reworked > as in vhost to support partial send of current skb). This callback > will be enabled only for G2H path. For details pls see comment > 'Check that tx queue...' below. > v3 -> v4: > * 'msgzerocopy_check_iov' moved from 'struct vsock_transport' to > 'struct virtio_transport' as it is virtio specific callback and > never needed in other transports. > v4 -> v5: > * 'msgzerocopy_check_iov' renamed to 'can_msgzerocopy' and now it > uses number of buffers to send as input argument. I think there is > no need to pass iov to this callback (at least today, it is used only > by guest side of virtio transport), because the only thing that this > callback does is comparison of number of buffers to be inserted to > the tx queue and size of this queue. > * Remove any checks for type of current 'iov_iter' with payload (is it > 'iovec' or 'ubuf'). These checks left from the earlier versions where I > didn't use already implemented kernel API which handles every type of > 'iov_iter'. > v5 -> v6: > * Refactor 'virtio_transport_fill_skb()'. > * Add 'WARN_ON_ONCE()' and comment on invalid combination of destination > socket and payload in 'virtio_transport_alloc_skb()'. > v7 -> v8: > * Move '+1' addition from 'can_msgzerocopy' callback body to the caller. > This addition means packet header. > * In 'virtio_transport_can_zcopy()' rename 'max_to_send' argument to > 'pkt_len'. > * Update commit message by adding details about new 'can_msgzerocopy' > callback. > * In 'virtio_transport_init_hdr()' move 'len' argument directly after > 'info'. > * Add comment about processing last skb in tx loop. > * Update comment for 'can_msgzerocopy' callback for more details. > v8 -> v9: > * Return and update comment for 'virtio_transport_alloc_skb()'. > * Pass pointer to transport ops to 'virtio_transport_can_zcopy()', > this allows to use it directly without calling virtio_transport_get_ops()'. > * Remove redundant call for 'msg_data_left()' in 'virtio_transport_fill_skb()'. > * Do not pass 'struct vsock_sock*' to 'virtio_transport_alloc_skb()', > use same pointer from already passed 'struct virtio_vsock_pkt_info*'. > * Fix setting 'end of message' bit for SOCK_SEQPACKET (add call for > 'msg_data_left()' == 0). > * Add 'zcopy' parameter to packet allocation trace event.Thanks for addressing the comments!> > include/linux/virtio_vsock.h | 9 + > .../events/vsock_virtio_transport_common.h | 12 +- > net/vmw_vsock/virtio_transport.c | 32 +++ > net/vmw_vsock/virtio_transport_common.c | 250 ++++++++++++++---- > 4 files changed, 241 insertions(+), 62 deletions(-)LGTM! Reviewed-by: Stefano Garzarella <sgarzare at redhat.com>