| Message ID | 30aa2604-77c0-322e-44fd-ff99fc25e388@sberdevices.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | fix header length on skb merging | expand |
On Sun, Mar 26, 2023 at 01:09:25AM +0300, Arseniy Krasnov wrote: >This adds WARN_ONCE() and return from stream dequeue callback when >socket's queue is empty, but 'rx_bytes' still non-zero. Nit: I would explain why we add this, for example: This allows the detection of potential bugs due to packet merging (see previous patch). > >Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >--- > net/vmw_vsock/virtio_transport_common.c | 7 +++++++ > 1 file changed, 7 insertions(+) > >diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c >index b9144af71553..ad70531de133 100644 >--- a/net/vmw_vsock/virtio_transport_common.c >+++ b/net/vmw_vsock/virtio_transport_common.c >@@ -398,6 +398,13 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, > u32 free_space; > > spin_lock_bh(&vvs->rx_lock); >+ >+ if (WARN_ONCE(skb_queue_empty(&vvs->rx_queue) && vvs->rx_bytes, >+ "No skbuffs with non-zero 'rx_bytes'\n")) { Nit: I would rephrase it this way: "rx_queue is empty, but rx_bytes is non-zero" >+ spin_unlock_bh(&vvs->rx_lock); >+ return err; >+ } >+ > while (total < len && !skb_queue_empty(&vvs->rx_queue)) { > skb = skb_peek(&vvs->rx_queue); > >-- >2.25.1 > Anyway the patch LGTM! Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
On 28.03.2023 12:29, Stefano Garzarella wrote: > On Sun, Mar 26, 2023 at 01:09:25AM +0300, Arseniy Krasnov wrote: >> This adds WARN_ONCE() and return from stream dequeue callback when >> socket's queue is empty, but 'rx_bytes' still non-zero. > > Nit: I would explain why we add this, for example: > > This allows the detection of potential bugs due to packet merging > (see previous patch). > >> >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >> --- >> net/vmw_vsock/virtio_transport_common.c | 7 +++++++ >> 1 file changed, 7 insertions(+) > >> >> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c >> index b9144af71553..ad70531de133 100644 >> --- a/net/vmw_vsock/virtio_transport_common.c >> +++ b/net/vmw_vsock/virtio_transport_common.c >> @@ -398,6 +398,13 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, >> u32 free_space; >> >> spin_lock_bh(&vvs->rx_lock); >> + >> + if (WARN_ONCE(skb_queue_empty(&vvs->rx_queue) && vvs->rx_bytes, >> + "No skbuffs with non-zero 'rx_bytes'\n")) { > > Nit: I would rephrase it this way: > "rx_queue is empty, but rx_bytes is non-zero" > >> + spin_unlock_bh(&vvs->rx_lock); >> + return err; >> + } >> + >> while (total < len && !skb_queue_empty(&vvs->rx_queue)) { >> skb = skb_peek(&vvs->rx_queue); >> >> -- >> 2.25.1 >> > > Anyway the patch LGTM! Thanks for review! Since only string value and commit message should be updated, i can resend it with 'net' (as it is fix) and update two thing above in 'net' version? Thanks, Arseniy > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> >
On Tue, Mar 28, 2023 at 11:35 AM Arseniy Krasnov <avkrasnov@sberdevices.ru> wrote: > > > > On 28.03.2023 12:29, Stefano Garzarella wrote: > > On Sun, Mar 26, 2023 at 01:09:25AM +0300, Arseniy Krasnov wrote: > >> This adds WARN_ONCE() and return from stream dequeue callback when > >> socket's queue is empty, but 'rx_bytes' still non-zero. > > > > Nit: I would explain why we add this, for example: > > > > This allows the detection of potential bugs due to packet merging > > (see previous patch). > > > >> > >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > >> --- > >> net/vmw_vsock/virtio_transport_common.c | 7 +++++++ > >> 1 file changed, 7 insertions(+) > > > >> > >> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c > >> index b9144af71553..ad70531de133 100644 > >> --- a/net/vmw_vsock/virtio_transport_common.c > >> +++ b/net/vmw_vsock/virtio_transport_common.c > >> @@ -398,6 +398,13 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, > >> u32 free_space; > >> > >> spin_lock_bh(&vvs->rx_lock); > >> + > >> + if (WARN_ONCE(skb_queue_empty(&vvs->rx_queue) && vvs->rx_bytes, > >> + "No skbuffs with non-zero 'rx_bytes'\n")) { > > > > Nit: I would rephrase it this way: > > "rx_queue is empty, but rx_bytes is non-zero" > > > >> + spin_unlock_bh(&vvs->rx_lock); > >> + return err; > >> + } > >> + > >> while (total < len && !skb_queue_empty(&vvs->rx_queue)) { > >> skb = skb_peek(&vvs->rx_queue); > >> > >> -- > >> 2.25.1 > >> > > > > Anyway the patch LGTM! > > Thanks for review! Since only string value and commit message should be > updated, i can resend it with 'net' (as it is fix) and update two thing > above in 'net' version? Yep, sure! And you can already add my R-b ;-) Thanks, Stefano > > Thanks, Arseniy > > > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > > >
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index b9144af71553..ad70531de133 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -398,6 +398,13 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, u32 free_space; spin_lock_bh(&vvs->rx_lock); + + if (WARN_ONCE(skb_queue_empty(&vvs->rx_queue) && vvs->rx_bytes, + "No skbuffs with non-zero 'rx_bytes'\n")) { + spin_unlock_bh(&vvs->rx_lock); + return err; + } + while (total < len && !skb_queue_empty(&vvs->rx_queue)) { skb = skb_peek(&vvs->rx_queue);
This adds WARN_ONCE() and return from stream dequeue callback when socket's queue is empty, but 'rx_bytes' still non-zero. Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> --- net/vmw_vsock/virtio_transport_common.c | 7 +++++++ 1 file changed, 7 insertions(+)