Stefano Garzarella
2023-Mar-20 14:29 UTC
[RFC PATCH v2] virtio/vsock: allocate multiple skbuffs on tx
On Sun, Mar 19, 2023 at 09:46:10PM +0300, Arseniy Krasnov wrote:>This adds small optimization for tx path: instead of allocating single >skbuff on every call to transport, allocate multiple skbuff's until >credit space allows, thus trying to send as much as possible data without >return to af_vsock.c. > >Signed-off-by: Arseniy Krasnov <AVKrasnov at sberdevices.ru> >--- > Link to v1: > https://lore.kernel.org/netdev/2c52aa26-8181-d37a-bccd-a86bd3cbc6e1 at sberdevices.ru/ > > Changelog: > v1 -> v2: > - If sent something, return number of bytes sent (even in > case of error). Return error only if failed to sent first > skbuff. > > net/vmw_vsock/virtio_transport_common.c | 53 ++++++++++++++++++------- > 1 file changed, 39 insertions(+), 14 deletions(-) > >diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c >index 6564192e7f20..3fdf1433ec28 100644 >--- a/net/vmw_vsock/virtio_transport_common.c >+++ b/net/vmw_vsock/virtio_transport_common.c >@@ -196,7 +196,8 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk, > const struct virtio_transport *t_ops; > struct virtio_vsock_sock *vvs; > u32 pkt_len = info->pkt_len; >- struct sk_buff *skb; >+ u32 rest_len; >+ int ret; > > info->type = virtio_transport_get_type(sk_vsock(vsk)); > >@@ -216,10 +217,6 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk, > > vvs = vsk->trans; > >- /* we can send less than pkt_len bytes */ >- if (pkt_len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE) >- pkt_len = VIRTIO_VSOCK_MAX_PKT_BUF_SIZE; >- > /* virtio_transport_get_credit might return less than pkt_len credit */ > pkt_len = virtio_transport_get_credit(vvs, pkt_len); > >@@ -227,17 +224,45 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk, > if (pkt_len == 0 && info->op == VIRTIO_VSOCK_OP_RW) > return pkt_len; > >- skb = virtio_transport_alloc_skb(info, pkt_len, >- src_cid, src_port, >- dst_cid, dst_port); >- if (!skb) { >- virtio_transport_put_credit(vvs, pkt_len); >- return -ENOMEM; >- } >+ ret = 0; >+ rest_len = pkt_len; >+ >+ do { >+ struct sk_buff *skb; >+ size_t skb_len; >+ >+ skb_len = min_t(u32, VIRTIO_VSOCK_MAX_PKT_BUF_SIZE, rest_len); >+ >+ skb = virtio_transport_alloc_skb(info, skb_len, >+ src_cid, src_port, >+ dst_cid, dst_port); >+ if (!skb) { >+ ret = -ENOMEM; >+ break; >+ } >+ >+ virtio_transport_inc_tx_pkt(vvs, skb); >+ >+ ret = t_ops->send_pkt(skb); >+ >+ if (ret < 0) >+ break; > >- virtio_transport_inc_tx_pkt(vvs, skb); >+ rest_len -= skb_len;t_ops->send_pkt() is returning the number of bytes sent. Current implementations always return `skb_len`, so there should be no problem, but it would be better to put a comment here, or we should handle the case where ret != skb_len to avoid future issues.>+ } while (rest_len); > >- return t_ops->send_pkt(skb); >+ /* Don't call this function with zero as argument: >+ * it tries to acquire spinlock and such argument >+ * makes this call useless.Good point, can we do the same also for virtio_transport_get_credit()? (Maybe in a separate patch) I'm thinking if may be better to do it directly inside the functions, but I don't have a strong opinion on that since we only call them here. Thanks, Stefano>+ */ >+ if (rest_len) >+ virtio_transport_put_credit(vvs, rest_len); >+ >+ /* Return number of bytes, if any data has been sent. */ >+ if (rest_len != pkt_len) >+ ret = pkt_len - rest_len; >+ >+ return ret; > } > > static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, >-- >2.25.1 >