| Message ID | 20190404105838.101559-2-sgarzare@redhat.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | vsock/virtio: optimizations to increase the throughput | expand |
On Thu, Apr 04, 2019 at 12:58:35PM +0200, Stefano Garzarella wrote: > @@ -256,6 +257,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, > struct virtio_vsock_sock *vvs = vsk->trans; > struct virtio_vsock_pkt *pkt; > size_t bytes, total = 0; > + s64 free_space; Why s64? buf_alloc, fwd_cnt, and last_fwd_cnt are all u32. fwd_cnt - last_fwd_cnt <= buf_alloc is always true. > int err = -EFAULT; > > spin_lock_bh(&vvs->rx_lock); > @@ -288,9 +290,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, > } > spin_unlock_bh(&vvs->rx_lock); > > - /* Send a credit pkt to peer */ > - virtio_transport_send_credit_update(vsk, VIRTIO_VSOCK_TYPE_STREAM, > - NULL); > + /* We send a credit update only when the space available seen > + * by the transmitter is less than VIRTIO_VSOCK_MAX_PKT_BUF_SIZE > + */ > + free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); Locking? These fields should be accessed under tx_lock. > + if (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE) { > + virtio_transport_send_credit_update(vsk, > + VIRTIO_VSOCK_TYPE_STREAM, > + NULL); > + }
On Thu, Apr 04, 2019 at 08:15:39PM +0100, Stefan Hajnoczi wrote: > On Thu, Apr 04, 2019 at 12:58:35PM +0200, Stefano Garzarella wrote: > > @@ -256,6 +257,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, > > struct virtio_vsock_sock *vvs = vsk->trans; > > struct virtio_vsock_pkt *pkt; > > size_t bytes, total = 0; > > + s64 free_space; > > Why s64? buf_alloc, fwd_cnt, and last_fwd_cnt are all u32. fwd_cnt - > last_fwd_cnt <= buf_alloc is always true. > Right, I'll use a u32 for free_space! Is is a leftover because initially I implemented something like virtio_transport_has_space(). > > int err = -EFAULT; > > > > spin_lock_bh(&vvs->rx_lock); > > @@ -288,9 +290,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, > > } > > spin_unlock_bh(&vvs->rx_lock); > > > > - /* Send a credit pkt to peer */ > > - virtio_transport_send_credit_update(vsk, VIRTIO_VSOCK_TYPE_STREAM, > > - NULL); > > + /* We send a credit update only when the space available seen > > + * by the transmitter is less than VIRTIO_VSOCK_MAX_PKT_BUF_SIZE > > + */ > > + free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); > > Locking? These fields should be accessed under tx_lock. > Yes, we need a lock, but looking in the code, vvs->fwd_cnd is written taking rx_lock (virtio_transport_dec_rx_pkt) and it is read with the tx_lock (virtio_transport_inc_tx_pkt). Maybe we should use another spin_lock shared between RX and TX for those fields or use atomic variables. What do you suggest? Thanks, Stefano
On Fri, Apr 05, 2019 at 10:16:48AM +0200, Stefano Garzarella wrote: > On Thu, Apr 04, 2019 at 08:15:39PM +0100, Stefan Hajnoczi wrote: > > On Thu, Apr 04, 2019 at 12:58:35PM +0200, Stefano Garzarella wrote: > > > int err = -EFAULT; > > > > > > spin_lock_bh(&vvs->rx_lock); > > > @@ -288,9 +290,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, > > > } > > > spin_unlock_bh(&vvs->rx_lock); > > > > > > - /* Send a credit pkt to peer */ > > > - virtio_transport_send_credit_update(vsk, VIRTIO_VSOCK_TYPE_STREAM, > > > - NULL); > > > + /* We send a credit update only when the space available seen > > > + * by the transmitter is less than VIRTIO_VSOCK_MAX_PKT_BUF_SIZE > > > + */ > > > + free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); > > > > Locking? These fields should be accessed under tx_lock. > > > > Yes, we need a lock, but looking in the code, vvs->fwd_cnd is written > taking rx_lock (virtio_transport_dec_rx_pkt) and it is read with the > tx_lock (virtio_transport_inc_tx_pkt). > > Maybe we should use another spin_lock shared between RX and TX for those > fields or use atomic variables. > > What do you suggest? Or make vvs->fwd_cnt atomic if it's the only field that needs to be accessed in this manner. Stefan
diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h index e223e2632edd..6d7a22cc20bf 100644 --- a/include/linux/virtio_vsock.h +++ b/include/linux/virtio_vsock.h @@ -37,6 +37,7 @@ struct virtio_vsock_sock { u32 tx_cnt; u32 buf_alloc; u32 peer_fwd_cnt; + u32 last_fwd_cnt; u32 peer_buf_alloc; /* Protected by rx_lock */ diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index 602715fc9a75..f32301d823f5 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -206,6 +206,7 @@ static void virtio_transport_dec_rx_pkt(struct virtio_vsock_sock *vvs, void virtio_transport_inc_tx_pkt(struct virtio_vsock_sock *vvs, struct virtio_vsock_pkt *pkt) { spin_lock_bh(&vvs->tx_lock); + vvs->last_fwd_cnt = vvs->fwd_cnt; pkt->hdr.fwd_cnt = cpu_to_le32(vvs->fwd_cnt); pkt->hdr.buf_alloc = cpu_to_le32(vvs->buf_alloc); spin_unlock_bh(&vvs->tx_lock); @@ -256,6 +257,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, struct virtio_vsock_sock *vvs = vsk->trans; struct virtio_vsock_pkt *pkt; size_t bytes, total = 0; + s64 free_space; int err = -EFAULT; spin_lock_bh(&vvs->rx_lock); @@ -288,9 +290,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, } spin_unlock_bh(&vvs->rx_lock); - /* Send a credit pkt to peer */ - virtio_transport_send_credit_update(vsk, VIRTIO_VSOCK_TYPE_STREAM, - NULL); + /* We send a credit update only when the space available seen + * by the transmitter is less than VIRTIO_VSOCK_MAX_PKT_BUF_SIZE + */ + free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt); + if (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE) { + virtio_transport_send_credit_update(vsk, + VIRTIO_VSOCK_TYPE_STREAM, + NULL); + } return total;
In order to reduce the number of credit update messages, we send them only when the space available seen by the transmitter is less than VIRTIO_VSOCK_MAX_PKT_BUF_SIZE. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- include/linux/virtio_vsock.h | 1 + net/vmw_vsock/virtio_transport_common.c | 14 +++++++++++--- 2 files changed, 12 insertions(+), 3 deletions(-)