| Message ID | 0abeec42-a11d-3a51-453b-6acf76604f2e@sberdevices.ru (mailing list archive) |
|---|---|
| Headers | show |
| Series | several updates to virtio/vsock | expand |
On 09.03.2023 19:21, Stefano Garzarella wrote: > On Thu, Mar 09, 2023 at 01:10:36PM +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 udpate >> 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/ >> >> 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. > > Yes, but they solve the problem, so we should use the tag (I think at > least in patch 1 and 3). > > We usually use the tag when we are fixing a problem introduced by a > previous change. So we need to backport the patch to the stable branches > as well, and we need the tag to figure out which branches have the patch > or not. Ahh, sorry. Ok. I see now :) Thanks, Arseniy > > Thanks, > Stefano > >> >> 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(-) >> >> -- >> 2.25.1 >> >
On Thu, Mar 09, 2023 at 01:10:36PM +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 udpate > 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/ > >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. Yes, but they solve the problem, so we should use the tag (I think at least in patch 1 and 3). We usually use the tag when we are fixing a problem introduced by a previous change. So we need to backport the patch to the stable branches as well, and we need the tag to figure out which branches have the patch or not. Thanks, Stefano > >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(-) > >-- >2.25.1 >
On Thu, Mar 09, 2023 at 07:20:20PM +0300, Arseniy Krasnov wrote: > > >On 09.03.2023 19:21, Stefano Garzarella wrote: >> On Thu, Mar 09, 2023 at 01:10:36PM +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 udpate >>> 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/ >>> >>> 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. >> >> Yes, but they solve the problem, so we should use the tag (I think at >> least in patch 1 and 3). >> >> We usually use the tag when we are fixing a problem introduced by a >> previous change. So we need to backport the patch to the stable branches >> as well, and we need the tag to figure out which branches have the patch >> or not. >Ahh, sorry. Ok. I see now :) No problem at all :-) I think also patch 2 can have the Fixes tag. Thanks, Stefano > >Thanks, Arseniy >> >> Thanks, >> Stefano >> >>> >>> 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(-) >>> >>> -- >>> 2.25.1 >>> >> >
On 09.03.2023 19:32, Stefano Garzarella wrote: > On Thu, Mar 09, 2023 at 07:20:20PM +0300, Arseniy Krasnov wrote: >> >> >> On 09.03.2023 19:21, Stefano Garzarella wrote: >>> On Thu, Mar 09, 2023 at 01:10:36PM +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 udpate >>>> 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/ >>>> >>>> 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. >>> >>> Yes, but they solve the problem, so we should use the tag (I think at >>> least in patch 1 and 3). >>> >>> We usually use the tag when we are fixing a problem introduced by a >>> previous change. So we need to backport the patch to the stable branches >>> as well, and we need the tag to figure out which branches have the patch >>> or not. >> Ahh, sorry. Ok. I see now :) > > No problem at all :-) > > I think also patch 2 can have the Fixes tag. > Done, fixed everything in v4. Thanks, Arseniy > Thanks, > Stefano > >> >> Thanks, Arseniy >>> >>> Thanks, >>> Stefano >>> >>>> >>>> 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(-) >>>> >>>> -- >>>> 2.25.1 >>>> >>> >> >
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 udpate 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/ 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. 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(-)