search for: virtio_transport_inc_rx_pkt

...= skb_headroom(skb) - sizeof(struct virtio_vsock_hdr); IIUC virtio_transport_dec_rx_pkt() is always called after skb_pull(), so skb_headroom() is returning the amount of space we removed. Looking at the other patches in this series, I think maybe we should change virtio_transport_dec_rx_pkt() and virtio_transport_inc_rx_pkt() by passing the value to subtract or add directly. Since some times we don't remove the whole payload, so it would be better to call it with the value in hdr->len. I mean something like this (untested): index a1581c77cf84..9e69ae7a9a96 100644 --- a/net/vmw_vsock/virtio_transport_common.c...

This series makes the credit mechanism implemented in the virtio-vsock devices more robust. Patch 1 sends an update to the remote peer when the buf_alloc change. Patch 2 prevents a malicious peer (especially the guest) can consume all the memory of the other peer, discarding packets when the credit available is not respected. Stefano Garzarella (2): vsock/virtio: send a credit update when

...} > + > /* Packet capture */ > static struct sk_buff *virtio_transport_build_skb(void *opaque) > { > @@ -190,17 +233,15 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk, > return virtio_transport_get_ops()->send_pkt(pkt); > } > > -static void virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, > - struct virtio_vsock_pkt *pkt) > +static void virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, u32 len) > { > - vvs->rx_bytes += pkt->len; > + vvs->rx_bytes += len; > } > > -static void virtio_transport_dec_rx_pkt(...

...*buf; > int err = 0; > > switch (le16_to_cpu(pkt->hdr.op)) { > case VIRTIO_VSOCK_OP_RW: > pkt->len = le32_to_cpu(pkt->hdr.len); > - pkt->off = 0; > + buf = virtio_transport_alloc_buf(pkt, true); > > - spin_lock_bh(&vvs->rx_lock); > - virtio_transport_inc_rx_pkt(vvs, pkt); > - list_add_tail(&pkt->list, &vvs->rx_queue); > - spin_unlock_bh(&vvs->rx_lock); > + if (buf) { > + spin_lock_bh(&vvs->rx_lock); > + virtio_transport_inc_rx_pkt(vvs, pkt->len); > + list_add_tail(&buf->list, &vvs->rx...

...*buf; > int err = 0; > > switch (le16_to_cpu(pkt->hdr.op)) { > case VIRTIO_VSOCK_OP_RW: > pkt->len = le32_to_cpu(pkt->hdr.len); > - pkt->off = 0; > + buf = virtio_transport_alloc_buf(pkt, true); > > - spin_lock_bh(&vvs->rx_lock); > - virtio_transport_inc_rx_pkt(vvs, pkt); > - list_add_tail(&pkt->list, &vvs->rx_queue); > - spin_unlock_bh(&vvs->rx_lock); > + if (buf) { > + spin_lock_bh(&vvs->rx_lock); > + virtio_transport_inc_rx_pkt(vvs, pkt->len); > + list_add_tail(&buf->list, &vvs->rx...

...uf) +{ + kfree(buf->addr); + kfree(buf); +} + /* Packet capture */ static struct sk_buff *virtio_transport_build_skb(void *opaque) { @@ -190,17 +233,15 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk, return virtio_transport_get_ops()->send_pkt(pkt); } -static void virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, - struct virtio_vsock_pkt *pkt) +static void virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, u32 len) { - vvs->rx_bytes += pkt->len; + vvs->rx_bytes += len; } -static void virtio_transport_dec_rx_pkt(struct virtio_vsock_sock *vvs, - str...

...t; switch (le16_to_cpu(pkt->hdr.op)) { > > case VIRTIO_VSOCK_OP_RW: > > pkt->len = le32_to_cpu(pkt->hdr.len); > > - pkt->off = 0; > > + buf = virtio_transport_alloc_buf(pkt, true); > > > > - spin_lock_bh(&vvs->rx_lock); > > - virtio_transport_inc_rx_pkt(vvs, pkt); > > - list_add_tail(&pkt->list, &vvs->rx_queue); > > - spin_unlock_bh(&vvs->rx_lock); > > + if (buf) { > > + spin_lock_bh(&vvs->rx_lock); > > + virtio_transport_inc_rx_pkt(vvs, pkt->len); > > + list_add_tail(&amp...

While I was testing this new series (v2) I discovered an huge use of memory and a memory leak in the virtio-vsock driver in the guest when I sent 1-byte packets to the guest. These issues are present since the introduction of the virtio-vsock driver. I added the patches 1 and 2 to fix them in this series in order to better track the performance trends. v1:

While I was testing this new series (v2) I discovered an huge use of memory and a memory leak in the virtio-vsock driver in the guest when I sent 1-byte packets to the guest. These issues are present since the introduction of the virtio-vsock driver. I added the patches 1 and 2 to fix them in this series in order to better track the performance trends. v1:

...} +static void +virtio_transport_recv_enqueue(struct vsock_sock *vsk, + struct virtio_vsock_pkt *pkt) +{ + struct virtio_vsock_sock *vvs = vsk->trans; + bool free_pkt = false; + + pkt->len = le32_to_cpu(pkt->hdr.len); + pkt->off = 0; + + spin_lock_bh(&vvs->rx_lock); + + virtio_transport_inc_rx_pkt(vvs, pkt); + + /* Try to copy small packets into the buffer of last packet queued, + * to avoid wasting memory queueing the entire buffer with a small + * payload. + */ + if (pkt->len <= GOOD_COPY_LEN && !list_empty(&vvs->rx_queue)) { + struct virtio_vsock_pkt *last_pkt; +...

...} +static void +virtio_transport_recv_enqueue(struct vsock_sock *vsk, + struct virtio_vsock_pkt *pkt) +{ + struct virtio_vsock_sock *vvs = vsk->trans; + bool free_pkt = false; + + pkt->len = le32_to_cpu(pkt->hdr.len); + pkt->off = 0; + + spin_lock_bh(&vvs->rx_lock); + + virtio_transport_inc_rx_pkt(vvs, pkt); + + /* Try to copy small packets into the buffer of last packet queued, + * to avoid wasting memory queueing the entire buffer with a small + * payload. + */ + if (pkt->len <= GOOD_COPY_LEN && !list_empty(&vvs->rx_queue)) { + struct virtio_vsock_pkt *last_pkt; +...

...+ > /* Packet capture */ > static struct sk_buff *virtio_transport_build_skb(void *opaque) > { > @@ -190,17 +233,15 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk, > return virtio_transport_get_ops()->send_pkt(pkt); > } > > -static void virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, > - struct virtio_vsock_pkt *pkt) > +static void virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, u32 len) > { > - vvs->rx_bytes += pkt->len; > + vvs->rx_bytes += len; > } > > -static void virtio_transport_dec_rx_p...

...+ > /* Packet capture */ > static struct sk_buff *virtio_transport_build_skb(void *opaque) > { > @@ -190,17 +233,15 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk, > return virtio_transport_get_ops()->send_pkt(pkt); > } > > -static void virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, > - struct virtio_vsock_pkt *pkt) > +static void virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, u32 len) > { > - vvs->rx_bytes += pkt->len; > + vvs->rx_bytes += len; > } > > -static void virtio_transport_dec_rx_p...

...st_cid = vsk->remote_addr.svm_cid; @@ -201,7 +202,7 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk, virtio_transport_inc_tx_pkt(vvs, pkt); - return virtio_transport_get_ops()->send_pkt(pkt); + return virtio_transport_get_ops(vsk)->send_pkt(pkt); } static void virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, -- 2.21.0

...ay be we can avoid calling 'skb_pull()' here if 'virtio_transport_dec_rx_pkt()' >>> will use integer argument? >> >> Yep, exactly! >> >>> Just call 'virtio_transport_dec_rx_pkt(skb->len)'. skb >> >> It depends on how we call virtio_transport_inc_rx_pkt(). If we use >> hdr->len there I would use the same to avoid confusion. Plus that's the >> value the other peer sent us, so definitely the right value to increase >> fwd_cnt with. But if skb->len always reflects it, then that's fine. >i've checked 'virtio_...

...st_cid = vsk->remote_addr.svm_cid; @@ -201,7 +202,7 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk, virtio_transport_inc_tx_pkt(vvs, pkt); - return virtio_transport_get_ops()->send_pkt(pkt); + return virtio_transport_get_ops(vsk)->send_pkt(pkt); } static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, -- 2.21.0

...vsock_sock *vsk, > + struct virtio_vsock_pkt *pkt) > +{ > + struct virtio_vsock_sock *vvs = vsk->trans; > + bool free_pkt = false; > + > + pkt->len = le32_to_cpu(pkt->hdr.len); > + pkt->off = 0; > + > + spin_lock_bh(&vvs->rx_lock); > + > + virtio_transport_inc_rx_pkt(vvs, pkt); > + > + /* Try to copy small packets into the buffer of last packet queued, > + * to avoid wasting memory queueing the entire buffer with a small > + * payload. > + */ > + if (pkt->len <= GOOD_COPY_LEN && !list_empty(&vvs->rx_queue)) { > + st...

...vsock_sock *vsk, > + struct virtio_vsock_pkt *pkt) > +{ > + struct virtio_vsock_sock *vvs = vsk->trans; > + bool free_pkt = false; > + > + pkt->len = le32_to_cpu(pkt->hdr.len); > + pkt->off = 0; > + > + spin_lock_bh(&vvs->rx_lock); > + > + virtio_transport_inc_rx_pkt(vvs, pkt); > + > + /* Try to copy small packets into the buffer of last packet queued, > + * to avoid wasting memory queueing the entire buffer with a small > + * payload. > + */ > + if (pkt->len <= GOOD_COPY_LEN && !list_empty(&vvs->rx_queue)) { > + st...

...here. Since we don't expect this condition for now, perhaps we can avoid setting ret with -EFAULT, but we can add a WARN_ONCE (interrupting the loop as you did here). This way we return the partial length as we did before. Thanks, Stefano >+ >+ return ret; > } > > static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs, >-- >2.25.1 >

...ret, skb_len)) >+ break; >+ } while (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 > The patch LGTM: Reviewed-by: Stefano Garzarella <sgarzare at redhat.com> Anyway, feel free to include in the same series or as separate patch also the changes to avoid useless lock in virtio_transport_put_credit() and virtio_transport_get_c...