| Message ID | 1804d100-1652-d463-8627-da93cb61144e@sberdevices.ru (mailing list archive) |
|---|---|
| Headers | show |
| Series | several updates to virtio/vsock | expand |
Hi Arseniy, On Thu, Mar 09, 2023 at 11:24:42PM +0300, Arseniy Krasnov wrote: >Hello, > >this patchset evolved from previous v2 version (see link below). It does >several updates to virtio/vsock: >1) Changes 'virtio_transport_inc/dec_rx_pkt()' interface. Now instead of > using skbuff state ('head' and 'data' pointers) to update 'fwd_cnt' > and 'rx_bytes', integer value is passed as an input argument. This > makes code more simple, because in this case we don't need to update > skbuff state before calling 'virtio_transport_inc/dec_rx_pkt()'. In > more common words - we don't need to change skbuff state to update > 'rx_bytes' and 'fwd_cnt' correctly. >2) For SOCK_STREAM, when copying data to user fails, current skbuff is > not dropped. Next read attempt will use same skbuff and last offset. > Instead of 'skb_dequeue()', 'skb_peek()' + '__skb_unlink()' are used. > This behaviour was implemented before skbuff support. >3) For SOCK_SEQPACKET it removes unneeded 'skb_pull()' call, because for > this type of socket each skbuff is used only once: after removing it > from socket's queue, it will be freed anyway. thanks for the fixes, I would wait a few days to see if there are any comments and then I think you can send it on net without RFC. @Bobby if you can take a look, your ack would be appreciated :-) Thanks, Stefano
On 10.03.2023 12:09, Stefano Garzarella wrote: > Hi Arseniy, > > On Thu, Mar 09, 2023 at 11:24:42PM +0300, Arseniy Krasnov wrote: >> Hello, >> >> this patchset evolved from previous v2 version (see link below). It does >> several updates to virtio/vsock: >> 1) Changes 'virtio_transport_inc/dec_rx_pkt()' interface. Now instead of >> using skbuff state ('head' and 'data' pointers) to update 'fwd_cnt' >> and 'rx_bytes', integer value is passed as an input argument. This >> makes code more simple, because in this case we don't need to update >> skbuff state before calling 'virtio_transport_inc/dec_rx_pkt()'. In >> more common words - we don't need to change skbuff state to update >> 'rx_bytes' and 'fwd_cnt' correctly. >> 2) For SOCK_STREAM, when copying data to user fails, current skbuff is >> not dropped. Next read attempt will use same skbuff and last offset. >> Instead of 'skb_dequeue()', 'skb_peek()' + '__skb_unlink()' are used. >> This behaviour was implemented before skbuff support. >> 3) For SOCK_SEQPACKET it removes unneeded 'skb_pull()' call, because for >> this type of socket each skbuff is used only once: after removing it >> from socket's queue, it will be freed anyway. > > thanks for the fixes, I would wait a few days to see if there are any > comments and then I think you can send it on net without RFC. > > @Bobby if you can take a look, your ack would be appreciated :-) Ok, thanks for review. I'll wait for several days and also wait until net-next will be opened. Then i'll resend this patchset with net-next Thanks, Arseniy > > Thanks, > Stefano >
On Fri, Mar 10, 2023 at 12:42:13PM +0300, Arseniy Krasnov wrote: > > >On 10.03.2023 12:09, Stefano Garzarella wrote: >> Hi Arseniy, >> >> On Thu, Mar 09, 2023 at 11:24:42PM +0300, Arseniy Krasnov wrote: >>> Hello, >>> >>> this patchset evolved from previous v2 version (see link below). It does >>> several updates to virtio/vsock: >>> 1) Changes 'virtio_transport_inc/dec_rx_pkt()' interface. Now instead of >>> using skbuff state ('head' and 'data' pointers) to update 'fwd_cnt' >>> and 'rx_bytes', integer value is passed as an input argument. This >>> makes code more simple, because in this case we don't need to update >>> skbuff state before calling 'virtio_transport_inc/dec_rx_pkt()'. In >>> more common words - we don't need to change skbuff state to update >>> 'rx_bytes' and 'fwd_cnt' correctly. >>> 2) For SOCK_STREAM, when copying data to user fails, current skbuff is >>> not dropped. Next read attempt will use same skbuff and last offset. >>> Instead of 'skb_dequeue()', 'skb_peek()' + '__skb_unlink()' are used. >>> This behaviour was implemented before skbuff support. >>> 3) For SOCK_SEQPACKET it removes unneeded 'skb_pull()' call, because for >>> this type of socket each skbuff is used only once: after removing it >>> from socket's queue, it will be freed anyway. >> >> thanks for the fixes, I would wait a few days to see if there are any >> comments and then I think you can send it on net without RFC. >> >> @Bobby if you can take a look, your ack would be appreciated :-) >Ok, thanks for review. I'll wait for several days and also wait until >net-next will be opened. Then i'll resend this patchset with net-next Since they are fixes, they should go with the net tree, not net-next. Cheers, Stefano
On 10.03.2023 14:40, Stefano Garzarella wrote: > On Fri, Mar 10, 2023 at 12:42:13PM +0300, Arseniy Krasnov wrote: >> >> >> On 10.03.2023 12:09, Stefano Garzarella wrote: >>> Hi Arseniy, >>> >>> On Thu, Mar 09, 2023 at 11:24:42PM +0300, Arseniy Krasnov wrote: >>>> Hello, >>>> >>>> this patchset evolved from previous v2 version (see link below). It does >>>> several updates to virtio/vsock: >>>> 1) Changes 'virtio_transport_inc/dec_rx_pkt()' interface. Now instead of >>>> using skbuff state ('head' and 'data' pointers) to update 'fwd_cnt' >>>> and 'rx_bytes', integer value is passed as an input argument. This >>>> makes code more simple, because in this case we don't need to update >>>> skbuff state before calling 'virtio_transport_inc/dec_rx_pkt()'. In >>>> more common words - we don't need to change skbuff state to update >>>> 'rx_bytes' and 'fwd_cnt' correctly. >>>> 2) For SOCK_STREAM, when copying data to user fails, current skbuff is >>>> not dropped. Next read attempt will use same skbuff and last offset. >>>> Instead of 'skb_dequeue()', 'skb_peek()' + '__skb_unlink()' are used. >>>> This behaviour was implemented before skbuff support. >>>> 3) For SOCK_SEQPACKET it removes unneeded 'skb_pull()' call, because for >>>> this type of socket each skbuff is used only once: after removing it >>>> from socket's queue, it will be freed anyway. >>> >>> thanks for the fixes, I would wait a few days to see if there are any >>> comments and then I think you can send it on net without RFC. >>> >>> @Bobby if you can take a look, your ack would be appreciated :-) >> Ok, thanks for review. I'll wait for several days and also wait until >> net-next will be opened. Then i'll resend this patchset with net-next > > Since they are fixes, they should go with the net tree, not net-next. Ah ok, for net tree i can send it no matter that net-next is closed. Thanks, Arseniy > > Cheers, > Stefano >
Hello, this patchset evolved from previous v2 version (see link below). It does several updates to virtio/vsock: 1) Changes 'virtio_transport_inc/dec_rx_pkt()' interface. Now instead of using skbuff state ('head' and 'data' pointers) to update 'fwd_cnt' and 'rx_bytes', integer value is passed as an input argument. This makes code more simple, because in this case we don't need to update skbuff state before calling 'virtio_transport_inc/dec_rx_pkt()'. In more common words - we don't need to change skbuff state to update 'rx_bytes' and 'fwd_cnt' correctly. 2) For SOCK_STREAM, when copying data to user fails, current skbuff is not dropped. Next read attempt will use same skbuff and last offset. Instead of 'skb_dequeue()', 'skb_peek()' + '__skb_unlink()' are used. This behaviour was implemented before skbuff support. 3) For SOCK_SEQPACKET it removes unneeded 'skb_pull()' call, because for this type of socket each skbuff is used only once: after removing it from socket's queue, it will be freed anyway. Test for 2) also added: Test tries to 'recv()' data to NULL buffer, then does 'recv()' with valid buffer. For SOCK_STREAM second 'recv()' must return data, because skbuff must not be dropped, but for SOCK_SEQPACKET skbuff will be dropped by kernel, and 'recv()' will return EAGAIN. Link to v1 on lore: https://lore.kernel.org/netdev/c2d3e204-89d9-88e9-8a15-3fe027e56b4b@sberdevices.ru/ Link to v2 on lore: https://lore.kernel.org/netdev/a7ab414b-5e41-c7b6-250b-e8401f335859@sberdevices.ru/ Link to v3 on lore: https://lore.kernel.org/netdev/0abeec42-a11d-3a51-453b-6acf76604f2e@sberdevices.ru/ Change log: v1 -> v2: - For SOCK_SEQPACKET call 'skb_pull()' also in case of copy failure or dropping skbuff (when we just waiting message end). - Handle copy failure for SOCK_STREAM in the same manner (plus free current skbuff). - Replace bug repdroducer with new test in vsock_test.c v2 -> v3: - Replace patch which removes 'skb->len' subtraction from function 'virtio_transport_dec_rx_pkt()' with patch which updates functions 'virtio_transport_inc/dec_rx_pkt()' by passing integer argument instead of skbuff pointer. - Replace patch which drops skbuff when copying to user fails with patch which changes this behaviour by keeping skbuff in queue until it has no data. - Add patch for SOCK_SEQPACKET which removes redundant 'skb_pull()' call on read. - I remove "Fixes" tag from all patches, because all of them now change code logic, not only fix something. v3 -> v4: - Update commit messages in all patches except test. - Add "Fixes" tag to all patches except test. Arseniy Krasnov (4): virtio/vsock: don't use skbuff state to account credit virtio/vsock: remove redundant 'skb_pull()' call virtio/vsock: don't drop skbuff on copy failure test/vsock: copy to user failure test net/vmw_vsock/virtio_transport_common.c | 29 +++--- tools/testing/vsock/vsock_test.c | 118 ++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 16 deletions(-)