Re: [PATCH RFC 1/4] vsock/virtio: reduce credit update messages
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(>rx_lock); > > @@ -288,9 +290,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock > > *vsk, > > } > > spin_unlock_bh(>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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH RFC 1/4] vsock/virtio: reduce credit update messages
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
---
include/linux/virtio_vsock.h| 1 +
net/vmw_vsock/virtio_transport_common.c | 14 +++---
2 files changed, 12 insertions(+), 3 deletions(-)
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(>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(>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(>rx_lock);
@@ -288,9 +290,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
}
spin_unlock_bh(>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;
--
2.20.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RFC 1/4] vsock/virtio: reduce credit update messages
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(>rx_lock); > > > @@ -288,9 +290,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock > > > *vsk, > > > } > > > spin_unlock_bh(>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 signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RFC 1/4] vsock/virtio: reduce credit update messages
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(>rx_lock);
> @@ -288,9 +290,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock
> *vsk,
> }
> spin_unlock_bh(>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);
> + }
signature.asc
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
