| Message ID | 20210628100250.570726-1-arseny.krasnov@kaspersky.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Improve SOCK_SEQPACKET receive logic | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | fail | Series longer than 15 patches |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 31 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Mon, Jun 28, 2021 at 01:02:47PM +0300, Arseny Krasnov wrote: >Also remove 'seqpacket_has_data' callback from transport. > >Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com> >--- > include/net/af_vsock.h | 1 - > net/vmw_vsock/af_vsock.c | 12 +----------- > 2 files changed, 1 insertion(+), 12 deletions(-) In order to avoid issues while bisecting the kernel, we should have commit that doesn't break the build or the runtime, so please take this in mind also for other commits. For example here we removed the seqpacket_has_data callbacks assignment before to remove where we use the callback, with a potential fault at runtime. I think you can simply put patches from 1 to 5 together in a single patch. In addition, we should move these changes after we don't need vsock_connectible_has_data() anymore, for example, where we replace the receive loop logic. Thanks, Stefano > >diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h >index ab207677e0a8..bf5ea1873e6f 100644 >--- a/include/net/af_vsock.h >+++ b/include/net/af_vsock.h >@@ -141,7 +141,6 @@ struct vsock_transport { > int (*seqpacket_enqueue)(struct vsock_sock *vsk, struct msghdr *msg, > size_t len); > bool (*seqpacket_allow)(u32 remote_cid); >- u32 (*seqpacket_has_data)(struct vsock_sock *vsk); > > /* Notification. */ > int (*notify_poll_in)(struct vsock_sock *, size_t, bool *); >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >index 21ccf450e249..59ce35da2e5b 100644 >--- a/net/vmw_vsock/af_vsock.c >+++ b/net/vmw_vsock/af_vsock.c >@@ -860,16 +860,6 @@ s64 vsock_stream_has_data(struct vsock_sock *vsk) > } > EXPORT_SYMBOL_GPL(vsock_stream_has_data); > >-static s64 vsock_connectible_has_data(struct vsock_sock *vsk) >-{ >- struct sock *sk = sk_vsock(vsk); >- >- if (sk->sk_type == SOCK_SEQPACKET) >- return vsk->transport->seqpacket_has_data(vsk); >- else >- return vsock_stream_has_data(vsk); >-} >- > s64 vsock_stream_has_space(struct vsock_sock *vsk) > { > return vsk->transport->stream_has_space(vsk); >@@ -1881,7 +1871,7 @@ static int vsock_connectible_wait_data(struct >sock *sk, > err = 0; > transport = vsk->transport; > >- while ((data = vsock_connectible_has_data(vsk)) == 0) { >+ while ((data = vsock_stream_has_data(vsk)) == 0) { > prepare_to_wait(sk_sleep(sk), wait, TASK_INTERRUPTIBLE); > > if (sk->sk_err != 0 || >-- 2.25.1 >
On 30.06.2021 15:10, Stefano Garzarella wrote: > On Mon, Jun 28, 2021 at 01:02:47PM +0300, Arseny Krasnov wrote: >> Also remove 'seqpacket_has_data' callback from transport. >> >> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com> >> --- >> include/net/af_vsock.h | 1 - >> net/vmw_vsock/af_vsock.c | 12 +----------- >> 2 files changed, 1 insertion(+), 12 deletions(-) > In order to avoid issues while bisecting the kernel, we should have > commit that doesn't break the build or the runtime, so please take this > in mind also for other commits. > > For example here we removed the seqpacket_has_data callbacks assignment > before to remove where we use the callback, with a potential fault at > runtime. > > I think you can simply put patches from 1 to 5 together in a single > patch. > > In addition, we should move these changes after we don't need > vsock_connectible_has_data() anymore, for example, where we replace the > receive loop logic. > > Thanks, > Stefano Ack > >> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h >> index ab207677e0a8..bf5ea1873e6f 100644 >> --- a/include/net/af_vsock.h >> +++ b/include/net/af_vsock.h >> @@ -141,7 +141,6 @@ struct vsock_transport { >> int (*seqpacket_enqueue)(struct vsock_sock *vsk, struct msghdr *msg, >> size_t len); >> bool (*seqpacket_allow)(u32 remote_cid); >> - u32 (*seqpacket_has_data)(struct vsock_sock *vsk); >> >> /* Notification. */ >> int (*notify_poll_in)(struct vsock_sock *, size_t, bool *); >> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >> index 21ccf450e249..59ce35da2e5b 100644 >> --- a/net/vmw_vsock/af_vsock.c >> +++ b/net/vmw_vsock/af_vsock.c >> @@ -860,16 +860,6 @@ s64 vsock_stream_has_data(struct vsock_sock *vsk) >> } >> EXPORT_SYMBOL_GPL(vsock_stream_has_data); >> >> -static s64 vsock_connectible_has_data(struct vsock_sock *vsk) >> -{ >> - struct sock *sk = sk_vsock(vsk); >> - >> - if (sk->sk_type == SOCK_SEQPACKET) >> - return vsk->transport->seqpacket_has_data(vsk); >> - else >> - return vsock_stream_has_data(vsk); >> -} >> - >> s64 vsock_stream_has_space(struct vsock_sock *vsk) >> { >> return vsk->transport->stream_has_space(vsk); >> @@ -1881,7 +1871,7 @@ static int vsock_connectible_wait_data(struct >> sock *sk, >> err = 0; >> transport = vsk->transport; >> >> - while ((data = vsock_connectible_has_data(vsk)) == 0) { >> + while ((data = vsock_stream_has_data(vsk)) == 0) { >> prepare_to_wait(sk_sleep(sk), wait, TASK_INTERRUPTIBLE); >> >> if (sk->sk_err != 0 || >> -- 2.25.1 >> >
diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h index ab207677e0a8..bf5ea1873e6f 100644 --- a/include/net/af_vsock.h +++ b/include/net/af_vsock.h @@ -141,7 +141,6 @@ struct vsock_transport { int (*seqpacket_enqueue)(struct vsock_sock *vsk, struct msghdr *msg, size_t len); bool (*seqpacket_allow)(u32 remote_cid); - u32 (*seqpacket_has_data)(struct vsock_sock *vsk); /* Notification. */ int (*notify_poll_in)(struct vsock_sock *, size_t, bool *); diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index 21ccf450e249..59ce35da2e5b 100644 --- a/net/vmw_vsock/af_vsock.c +++ b/net/vmw_vsock/af_vsock.c @@ -860,16 +860,6 @@ s64 vsock_stream_has_data(struct vsock_sock *vsk) } EXPORT_SYMBOL_GPL(vsock_stream_has_data); -static s64 vsock_connectible_has_data(struct vsock_sock *vsk) -{ - struct sock *sk = sk_vsock(vsk); - - if (sk->sk_type == SOCK_SEQPACKET) - return vsk->transport->seqpacket_has_data(vsk); - else - return vsock_stream_has_data(vsk); -} - s64 vsock_stream_has_space(struct vsock_sock *vsk) { return vsk->transport->stream_has_space(vsk); @@ -1881,7 +1871,7 @@ static int vsock_connectible_wait_data(struct sock *sk, err = 0; transport = vsk->transport; - while ((data = vsock_connectible_has_data(vsk)) == 0) { + while ((data = vsock_stream_has_data(vsk)) == 0) { prepare_to_wait(sk_sleep(sk), wait, TASK_INTERRUPTIBLE); if (sk->sk_err != 0 ||
Also remove 'seqpacket_has_data' callback from transport. Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com> --- include/net/af_vsock.h | 1 - net/vmw_vsock/af_vsock.c | 12 +----------- 2 files changed, 1 insertion(+), 12 deletions(-)