Re: [RFC PATCH v2 2/4] virtio/vsock: remove all data from sk_buff

From: Stefano Garzarella
Date: Mon Mar 06 2023 - 12:09:05 EST


On Mon, Mar 06, 2023 at 07:00:10PM +0300, Arseniy Krasnov wrote:


On 06.03.2023 18:51, Stefano Garzarella wrote:
On Mon, Mar 06, 2023 at 06:31:22PM +0300, Arseniy Krasnov wrote:


On 06.03.2023 15:08, Stefano Garzarella wrote:
On Sun, Mar 05, 2023 at 11:07:37PM +0300, Arseniy Krasnov wrote:
In case of SOCK_SEQPACKET all sk_buffs are used once - after read some
data from it, it will be removed, so user will never read rest of the
data. Thus we need to update credit parameters of the socket like whole
sk_buff is read - so call 'skb_pull()' for the whole buffer.

Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
Signed-off-by: Arseniy Krasnov <AVKrasnov@xxxxxxxxxxxxxx>
---
net/vmw_vsock/virtio_transport_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Maybe we could avoid this patch if we directly use pkt_len as I
suggested in the previous patch.
Hm, may 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_transport_rx_work()', it calls 'virtio_vsock_skb_rx_put()' which
sets 'skb->len'. Value is used from header, so seems 'skb->len' == 'hdr->len' in this
case.

Thank you for checking it.

However, I still think it is better to use `hdr->len` (we have to assign it to `pkt_len` anyway, as in the proposal I sent for patch 1), otherwise we have to go every time to check if skb_* functions touch skb->len.

E.g. skb_pull() decrease skb->len, so I'm not sure we can call virtio_transport_dec_rx_pkt(skb->len) if we don't remove `skb_pull(skb, bytes_to_copy);` inside the loop.

Thanks,
Stefano