| Message ID | AS2P194MB2170FDCADD288267B874B6AC9A002@AS2P194MB2170.EURP194.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | This patch series introduce the support for ioctl(s) in AF_VSOCK. | expand |
On Mon, 2024-04-08 at 15:37 +0200, Luigi Leonardi wrote: > This patch introduce support for stream_bytes_unsent and > seqpacket_bytes_unsent ioctl for virtio_transport, vhost_vsock > and vsock_loopback. > > For all transports the unsent bytes counter is incremented > in virtio_transport_send_pkt_info. > > In the virtio_transport (G2H) the counter is decremented each time the host > notifies the guest that it consumed the skbuffs. > In vhost-vsock (H2G) the counter is decremented after the skbuff is queued > in the virtqueue. > In vsock_loopback the counter is decremented after the skbuff is > dequeued. > > Signed-off-by: Luigi Leonardi <luigi.leonardi@outlook.com> I think this deserve an explicit ack from Stefano, and Stefano can't review patches in the next few weeks. If it's not urgent this will have to wait a bit. > --- > drivers/vhost/vsock.c | 4 ++- > include/linux/virtio_vsock.h | 7 ++++++ > net/vmw_vsock/virtio_transport.c | 4 ++- > net/vmw_vsock/virtio_transport_common.c | 33 +++++++++++++++++++++++++ > net/vmw_vsock/vsock_loopback.c | 7 ++++++ > 5 files changed, 53 insertions(+), 2 deletions(-) > > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c > index ec20ecff85c7..dba8b3ea37bf 100644 > --- a/drivers/vhost/vsock.c > +++ b/drivers/vhost/vsock.c > @@ -244,7 +244,7 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock, > restart_tx = true; > } > > - consume_skb(skb); > + virtio_transport_consume_skb_sent(skb, true); > } > } while(likely(!vhost_exceeds_weight(vq, ++pkts, total_len))); > if (added) > @@ -451,6 +451,8 @@ static struct virtio_transport vhost_transport = { > .notify_buffer_size = virtio_transport_notify_buffer_size, > .notify_set_rcvlowat = virtio_transport_notify_set_rcvlowat, > > + .unsent_bytes = virtio_transport_bytes_unsent, > + > .read_skb = virtio_transport_read_skb, > }, > > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h > index c82089dee0c8..dbb22d45d203 100644 > --- a/include/linux/virtio_vsock.h > +++ b/include/linux/virtio_vsock.h > @@ -134,6 +134,8 @@ struct virtio_vsock_sock { > u32 peer_fwd_cnt; > u32 peer_buf_alloc; > > + atomic_t bytes_unsent; This will add 2 atomic operations per packet, possibly on contended cachelines. Have you considered leveraging the existing transport-level lock to protect the counter updates? Thanks Paolo
On Thu, Apr 11, 2024 at 09:09:49AM GMT, Paolo Abeni wrote: >On Mon, 2024-04-08 at 15:37 +0200, Luigi Leonardi wrote: >> This patch introduce support for stream_bytes_unsent and >> seqpacket_bytes_unsent ioctl for virtio_transport, vhost_vsock >> and vsock_loopback. >> >> For all transports the unsent bytes counter is incremented >> in virtio_transport_send_pkt_info. >> >> In the virtio_transport (G2H) the counter is decremented each time the host >> notifies the guest that it consumed the skbuffs. >> In vhost-vsock (H2G) the counter is decremented after the skbuff is queued >> in the virtqueue. >> In vsock_loopback the counter is decremented after the skbuff is >> dequeued. >> >> Signed-off-by: Luigi Leonardi <luigi.leonardi@outlook.com> > >I think this deserve an explicit ack from Stefano, and Stefano can't >review patches in the next few weeks. If it's not urgent this will have >to wait a bit. > >> --- >> drivers/vhost/vsock.c | 4 ++- >> include/linux/virtio_vsock.h | 7 ++++++ >> net/vmw_vsock/virtio_transport.c | 4 ++- >> net/vmw_vsock/virtio_transport_common.c | 33 +++++++++++++++++++++++++ >> net/vmw_vsock/vsock_loopback.c | 7 ++++++ >> 5 files changed, 53 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c >> index ec20ecff85c7..dba8b3ea37bf 100644 >> --- a/drivers/vhost/vsock.c >> +++ b/drivers/vhost/vsock.c >> @@ -244,7 +244,7 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock, >> restart_tx = true; >> } >> >> - consume_skb(skb); >> + virtio_transport_consume_skb_sent(skb, true); >> } >> } while(likely(!vhost_exceeds_weight(vq, ++pkts, total_len))); >> if (added) >> @@ -451,6 +451,8 @@ static struct virtio_transport vhost_transport = { >> .notify_buffer_size = virtio_transport_notify_buffer_size, >> .notify_set_rcvlowat = virtio_transport_notify_set_rcvlowat, >> >> + .unsent_bytes = virtio_transport_bytes_unsent, >> + >> .read_skb = virtio_transport_read_skb, >> }, >> >> diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h >> index c82089dee0c8..dbb22d45d203 100644 >> --- a/include/linux/virtio_vsock.h >> +++ b/include/linux/virtio_vsock.h >> @@ -134,6 +134,8 @@ struct virtio_vsock_sock { >> u32 peer_fwd_cnt; >> u32 peer_buf_alloc; >> >> + atomic_t bytes_unsent; > >This will add 2 atomic operations per packet, possibly on contended >cachelines. Have you considered leveraging the existing transport-level >lock to protect the counter updates? Good point! Maybe we can handle it together with `tx_cnt` in virtio_transport_get_credit()/virtio_transport_put_credit(). Eventually these are called exactly to count the payload we are sending (`tx_cnt` is a counter that only grows, virtio_transport_put_credit() is called only to return unused credit, so we can't use it directly but always need a new variable like `bytes_unsent`). I mean something like this (untested at all): diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h index dbb22d45d203..713197c16b7f 100644 --- a/include/linux/virtio_vsock.h +++ b/include/linux/virtio_vsock.h @@ -133,8 +133,7 @@ struct virtio_vsock_sock { u32 tx_cnt; u32 peer_fwd_cnt; u32 peer_buf_alloc; - - atomic_t bytes_unsent; + u32 bytes_unsent; /* Protected by rx_lock */ u32 fwd_cnt; diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index 82a31a13dc32..b1a51db616cf 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -419,13 +419,6 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk, */ rest_len -= ret; - /* Avoid to perform an atomic_add on 0 bytes. - * This is equivalent to check on VIRTIO_VSOCK_OP_RW - * as is the only packet type with payload. - */ - if (ret) - atomic_add(ret, &vvs->bytes_unsent); - if (WARN_ONCE(ret != skb_len, "'send_pkt()' returns %i, but %zu expected\n", ret, skb_len)) @@ -479,7 +472,10 @@ void virtio_transport_consume_skb_sent(struct sk_buff *skb, bool consume) struct virtio_vsock_sock *vvs; vvs = vs->trans; - atomic_sub(skb->len, &vvs->bytes_unsent); + + spin_lock_bh(&vvs->tx_lock); + vvs->bytes_unsent -= skb->len; + spin_unlock_bh(&vvs->tx_lock); } if (consume) @@ -499,6 +495,7 @@ u32 virtio_transport_get_credit(struct virtio_vsock_sock *vvs, u32 credit) if (ret > credit) ret = credit; vvs->tx_cnt += ret; + vvs->bytes_unsent += ret; spin_unlock_bh(&vvs->tx_lock); return ret; @@ -512,6 +509,7 @@ void virtio_transport_put_credit(struct virtio_vsock_sock *vvs, u32 credit) spin_lock_bh(&vvs->tx_lock); vvs->tx_cnt -= credit; + vvs->bytes_unsent -= ret; spin_unlock_bh(&vvs->tx_lock); } EXPORT_SYMBOL_GPL(virtio_transport_put_credit); @@ -915,7 +913,6 @@ int virtio_transport_do_socket_init(struct vsock_sock *vsk, vsk->buffer_size = VIRTIO_VSOCK_MAX_BUF_SIZE; vvs->buf_alloc = vsk->buffer_size; - atomic_set(&vvs->bytes_unsent, 0); spin_lock_init(&vvs->rx_lock); spin_lock_init(&vvs->tx_lock); @@ -1118,8 +1115,13 @@ EXPORT_SYMBOL_GPL(virtio_transport_destruct); int virtio_transport_bytes_unsent(struct vsock_sock *vsk) { struct virtio_vsock_sock *vvs = vsk->trans; + int ret; - return atomic_read(&vvs->bytes_unsent); + spin_lock_bh(&vvs->tx_lock); + ret = vvs->bytes_unsent; + spin_unlock_bh(&vvs->tx_lock); + + return ret; } EXPORT_SYMBOL_GPL(virtio_transport_bytes_unsent); WDYT? Should virtio_transport_bytes_unsent() returns size_t? Thanks, Stefano
Hi Paolo and Stefano, Sorry for the (super) late reply! I haven't had much time to work on it, I hope to send a v3 by the end of this month! Thanks both of you for your comments and reviews! :) > > This will add 2 atomic operations per packet, possibly on contended > > cachelines. Have you considered leveraging the existing transport-level > > lock to protect the counter updates? > Good point! > Maybe we can handle it together with `tx_cnt` in > virtio_transport_get_credit()/virtio_transport_put_credit(). > WDYT? I'll take a look at it! That's a very good idea. > Should virtio_transport_bytes_unsent() returns size_t? Yes! int was because of atomic_int, size_t is more appropriate. Thanks, Luigi
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c index ec20ecff85c7..dba8b3ea37bf 100644 --- a/drivers/vhost/vsock.c +++ b/drivers/vhost/vsock.c @@ -244,7 +244,7 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock, restart_tx = true; } - consume_skb(skb); + virtio_transport_consume_skb_sent(skb, true); } } while(likely(!vhost_exceeds_weight(vq, ++pkts, total_len))); if (added) @@ -451,6 +451,8 @@ static struct virtio_transport vhost_transport = { .notify_buffer_size = virtio_transport_notify_buffer_size, .notify_set_rcvlowat = virtio_transport_notify_set_rcvlowat, + .unsent_bytes = virtio_transport_bytes_unsent, + .read_skb = virtio_transport_read_skb, }, diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h index c82089dee0c8..dbb22d45d203 100644 --- a/include/linux/virtio_vsock.h +++ b/include/linux/virtio_vsock.h @@ -134,6 +134,8 @@ struct virtio_vsock_sock { u32 peer_fwd_cnt; u32 peer_buf_alloc; + atomic_t bytes_unsent; + /* Protected by rx_lock */ u32 fwd_cnt; u32 last_fwd_cnt; @@ -193,6 +195,11 @@ s64 virtio_transport_stream_has_data(struct vsock_sock *vsk); s64 virtio_transport_stream_has_space(struct vsock_sock *vsk); u32 virtio_transport_seqpacket_has_data(struct vsock_sock *vsk); +int virtio_transport_bytes_unsent(struct vsock_sock *vsk); + +void virtio_transport_consume_skb_sent(struct sk_buff *skb, + bool consume); + int virtio_transport_do_socket_init(struct vsock_sock *vsk, struct vsock_sock *psk); int diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c index ee5d306a96d0..a4656d0a2567 100644 --- a/net/vmw_vsock/virtio_transport.c +++ b/net/vmw_vsock/virtio_transport.c @@ -311,7 +311,7 @@ static void virtio_transport_tx_work(struct work_struct *work) virtqueue_disable_cb(vq); while ((skb = virtqueue_get_buf(vq, &len)) != NULL) { - consume_skb(skb); + virtio_transport_consume_skb_sent(skb, true); added = true; } } while (!virtqueue_enable_cb(vq)); @@ -540,6 +540,8 @@ static struct virtio_transport virtio_transport = { .notify_buffer_size = virtio_transport_notify_buffer_size, .notify_set_rcvlowat = virtio_transport_notify_set_rcvlowat, + .unsent_bytes = virtio_transport_bytes_unsent, + .read_skb = virtio_transport_read_skb, }, diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index 16ff976a86e3..82a31a13dc32 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -419,6 +419,13 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk, */ rest_len -= ret; + /* Avoid to perform an atomic_add on 0 bytes. + * This is equivalent to check on VIRTIO_VSOCK_OP_RW + * as is the only packet type with payload. + */ + if (ret) + atomic_add(ret, &vvs->bytes_unsent); + if (WARN_ONCE(ret != skb_len, "'send_pkt()' returns %i, but %zu expected\n", ret, skb_len)) @@ -463,6 +470,23 @@ void virtio_transport_inc_tx_pkt(struct virtio_vsock_sock *vvs, struct sk_buff * } EXPORT_SYMBOL_GPL(virtio_transport_inc_tx_pkt); +void virtio_transport_consume_skb_sent(struct sk_buff *skb, bool consume) +{ + struct sock *s = skb->sk; + + if (s && skb->len) { + struct vsock_sock *vs = vsock_sk(s); + struct virtio_vsock_sock *vvs; + + vvs = vs->trans; + atomic_sub(skb->len, &vvs->bytes_unsent); + } + + if (consume) + consume_skb(skb); +} +EXPORT_SYMBOL_GPL(virtio_transport_consume_skb_sent); + u32 virtio_transport_get_credit(struct virtio_vsock_sock *vvs, u32 credit) { u32 ret; @@ -891,6 +915,7 @@ int virtio_transport_do_socket_init(struct vsock_sock *vsk, vsk->buffer_size = VIRTIO_VSOCK_MAX_BUF_SIZE; vvs->buf_alloc = vsk->buffer_size; + atomic_set(&vvs->bytes_unsent, 0); spin_lock_init(&vvs->rx_lock); spin_lock_init(&vvs->tx_lock); @@ -1090,6 +1115,14 @@ void virtio_transport_destruct(struct vsock_sock *vsk) } EXPORT_SYMBOL_GPL(virtio_transport_destruct); +int virtio_transport_bytes_unsent(struct vsock_sock *vsk) +{ + struct virtio_vsock_sock *vvs = vsk->trans; + + return atomic_read(&vvs->bytes_unsent); +} +EXPORT_SYMBOL_GPL(virtio_transport_bytes_unsent); + static int virtio_transport_reset(struct vsock_sock *vsk, struct sk_buff *skb) { diff --git a/net/vmw_vsock/vsock_loopback.c b/net/vmw_vsock/vsock_loopback.c index 6dea6119f5b2..9098613561e3 100644 --- a/net/vmw_vsock/vsock_loopback.c +++ b/net/vmw_vsock/vsock_loopback.c @@ -98,6 +98,8 @@ static struct virtio_transport loopback_transport = { .notify_buffer_size = virtio_transport_notify_buffer_size, .notify_set_rcvlowat = virtio_transport_notify_set_rcvlowat, + .unsent_bytes = virtio_transport_bytes_unsent, + .read_skb = virtio_transport_read_skb, }, @@ -123,6 +125,11 @@ static void vsock_loopback_work(struct work_struct *work) spin_unlock_bh(&vsock->pkt_queue.lock); while ((skb = __skb_dequeue(&pkts))) { + /* Decrement the bytes_sent counter without deallocating skb + * It is freed by the receiver. + */ + virtio_transport_consume_skb_sent(skb, false); + virtio_transport_deliver_tap_pkt(skb); virtio_transport_recv_pkt(&loopback_transport, skb); }
This patch introduce support for stream_bytes_unsent and seqpacket_bytes_unsent ioctl for virtio_transport, vhost_vsock and vsock_loopback. For all transports the unsent bytes counter is incremented in virtio_transport_send_pkt_info. In the virtio_transport (G2H) the counter is decremented each time the host notifies the guest that it consumed the skbuffs. In vhost-vsock (H2G) the counter is decremented after the skbuff is queued in the virtqueue. In vsock_loopback the counter is decremented after the skbuff is dequeued. Signed-off-by: Luigi Leonardi <luigi.leonardi@outlook.com> --- drivers/vhost/vsock.c | 4 ++- include/linux/virtio_vsock.h | 7 ++++++ net/vmw_vsock/virtio_transport.c | 4 ++- net/vmw_vsock/virtio_transport_common.c | 33 +++++++++++++++++++++++++ net/vmw_vsock/vsock_loopback.c | 7 ++++++ 5 files changed, 53 insertions(+), 2 deletions(-)