| Message ID | 20210127125018.7059-1-kuniyu@amazon.co.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: Remove redundant calls of sk_tx_queue_clear(). | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 7 maintainers not CCed: keescook@chromium.org pabeni@redhat.com jakub@cloudflare.com daniel@iogearbox.net bjorn@kernel.org linmiaohe@huawei.com ast@kernel.org |
| 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: 5 this patch: 5 |
| 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, 14 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 5 this patch: 5 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
From: Kuniyuki Iwashima <kuniyu@amazon.co.jp> Date: Wed, 27 Jan 2021 21:50:18 +0900 > The commit 41b14fb8724d ("net: Do not clear the sock TX queue in > sk_set_socket()") removes sk_tx_queue_clear() from sk_set_socket() and adds > it instead in sk_alloc() and sk_clone_lock() to fix an issue introduced in > the commit e022f0b4a03f ("net: Introduce sk_tx_queue_mapping"). However, > the original commit had already put sk_tx_queue_clear() in sk_prot_alloc(): > the callee of sk_alloc() and sk_clone_lock(). Thus sk_tx_queue_clear() is > called twice in each path currently. > > This patch removes the redundant calls of sk_tx_queue_clear() in sk_alloc() > and sk_clone_lock(). > > Fixes: 41b14fb8724d ("net: Do not clear the sock TX queue in sk_set_socket()") > CC: Tariq Toukan <tariqt@mellanox.com> > CC: Boris Pismenny <borisp@mellanox.com> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> > Reviewed-by: Amit Shah <aams@amazon.de> I'm sorry, I have respun the v2 patch. So, please ignore v1. v2: https://lore.kernel.org/netdev/20210127132215.10842-1-kuniyu@amazon.co.jp/ Best regards, Kuniyuki
On Wed, Jan 27, 2021 at 1:50 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > The commit 41b14fb8724d ("net: Do not clear the sock TX queue in > sk_set_socket()") removes sk_tx_queue_clear() from sk_set_socket() and adds > it instead in sk_alloc() and sk_clone_lock() to fix an issue introduced in > the commit e022f0b4a03f ("net: Introduce sk_tx_queue_mapping"). However, > the original commit had already put sk_tx_queue_clear() in sk_prot_alloc(): > the callee of sk_alloc() and sk_clone_lock(). Thus sk_tx_queue_clear() is > called twice in each path currently. Are you sure ? I do not clearly see the sk_tx_queue_clear() call from the cloning part. Please elaborate. In any case, this seems to be a candidate for net-next, this is not fixing a bug, this would be an optimization at most, and potentially adding a bug. So if you resend this patch, you can mention the old commit in the changelog, but do not add a dubious Fixes: tag > > This patch removes the redundant calls of sk_tx_queue_clear() in sk_alloc() > and sk_clone_lock(). > > Fixes: 41b14fb8724d ("net: Do not clear the sock TX queue in sk_set_socket()") > CC: Tariq Toukan <tariqt@mellanox.com> > CC: Boris Pismenny <borisp@mellanox.com> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> > Reviewed-by: Amit Shah <aams@amazon.de> > --- > net/core/sock.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/net/core/sock.c b/net/core/sock.c > index bbcd4b97eddd..5c665ee14159 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -1759,7 +1759,6 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority, > cgroup_sk_alloc(&sk->sk_cgrp_data); > sock_update_classid(&sk->sk_cgrp_data); > sock_update_netprioidx(&sk->sk_cgrp_data); > - sk_tx_queue_clear(sk); > } > > return sk; > @@ -1983,7 +1982,6 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority) > */ > sk_refcnt_debug_inc(newsk); > sk_set_socket(newsk, NULL); > - sk_tx_queue_clear(newsk); > RCU_INIT_POINTER(newsk->sk_wq, NULL); > > if (newsk->sk_prot->sockets_allocated) > -- > 2.17.2 (Apple Git-113) >
From: Eric Dumazet <edumazet@google.com> Date: Wed, 27 Jan 2021 15:54:32 +0100 > On Wed, Jan 27, 2021 at 1:50 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > The commit 41b14fb8724d ("net: Do not clear the sock TX queue in > > sk_set_socket()") removes sk_tx_queue_clear() from sk_set_socket() and adds > > it instead in sk_alloc() and sk_clone_lock() to fix an issue introduced in > > the commit e022f0b4a03f ("net: Introduce sk_tx_queue_mapping"). However, > > the original commit had already put sk_tx_queue_clear() in sk_prot_alloc(): > > the callee of sk_alloc() and sk_clone_lock(). Thus sk_tx_queue_clear() is > > called twice in each path currently. > > Are you sure ? > > I do not clearly see the sk_tx_queue_clear() call from the cloning part. > > Please elaborate. If sk is not NULL in sk_prot_alloc(), sk_tx_queue_clear() is called [1]. Also the callers of sk_prot_alloc() are only sk_alloc() and sk_clone_lock(). If they finally return not NULL pointer, sk_tx_queue_clear() is called in each function [2][3]. In the cloning part, sock_copy() is called after sk_prot_alloc(), but skc_tx_queue_mapping is defined between skc_dontcopy_begin and skc_dontcopy_end in struct sock_common [4]. So, sock_copy() does not overwrite skc_tx_queue_mapping, and thus we can initialize it in sk_prot_alloc(). [1] sk_prot_alloc https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1693 [2] sk_alloc https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1762 [3] sk_clone_lock https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1986 [4] struct sock_common https://github.com/torvalds/linux/blob/master/include/net/sock.h#L218-L240 > In any case, this seems to be a candidate for net-next, this is not > fixing a bug, > this would be an optimization at most, and potentially adding a bug. > > So if you resend this patch, you can mention the old commit in the changelog, > but do not add a dubious Fixes: tag I see. I will remove the tag and resend this as a net-next candidate. Thank you, Kuniyuki > > > > This patch removes the redundant calls of sk_tx_queue_clear() in sk_alloc() > > and sk_clone_lock(). > > > > Fixes: 41b14fb8724d ("net: Do not clear the sock TX queue in sk_set_socket()") > > CC: Tariq Toukan <tariqt@mellanox.com> > > CC: Boris Pismenny <borisp@mellanox.com> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> > > Reviewed-by: Amit Shah <aams@amazon.de> > > --- > > net/core/sock.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > index bbcd4b97eddd..5c665ee14159 100644 > > --- a/net/core/sock.c > > +++ b/net/core/sock.c > > @@ -1759,7 +1759,6 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority, > > cgroup_sk_alloc(&sk->sk_cgrp_data); > > sock_update_classid(&sk->sk_cgrp_data); > > sock_update_netprioidx(&sk->sk_cgrp_data); > > - sk_tx_queue_clear(sk); > > } > > > > return sk; > > @@ -1983,7 +1982,6 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority) > > */ > > sk_refcnt_debug_inc(newsk); > > sk_set_socket(newsk, NULL); > > - sk_tx_queue_clear(newsk); > > RCU_INIT_POINTER(newsk->sk_wq, NULL); > > > > if (newsk->sk_prot->sockets_allocated) > > -- > > 2.17.2 (Apple Git-113) > >
On Wed, Jan 27, 2021 at 5:52 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > From: Eric Dumazet <edumazet@google.com> > Date: Wed, 27 Jan 2021 15:54:32 +0100 > > On Wed, Jan 27, 2021 at 1:50 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > > > The commit 41b14fb8724d ("net: Do not clear the sock TX queue in > > > sk_set_socket()") removes sk_tx_queue_clear() from sk_set_socket() and adds > > > it instead in sk_alloc() and sk_clone_lock() to fix an issue introduced in > > > the commit e022f0b4a03f ("net: Introduce sk_tx_queue_mapping"). However, > > > the original commit had already put sk_tx_queue_clear() in sk_prot_alloc(): > > > the callee of sk_alloc() and sk_clone_lock(). Thus sk_tx_queue_clear() is > > > called twice in each path currently. > > > > Are you sure ? > > > > I do not clearly see the sk_tx_queue_clear() call from the cloning part. > > > > Please elaborate. > > If sk is not NULL in sk_prot_alloc(), sk_tx_queue_clear() is called [1]. > Also the callers of sk_prot_alloc() are only sk_alloc() and sk_clone_lock(). > If they finally return not NULL pointer, sk_tx_queue_clear() is called in > each function [2][3]. > > In the cloning part, sock_copy() is called after sk_prot_alloc(), but > skc_tx_queue_mapping is defined between skc_dontcopy_begin and > skc_dontcopy_end in struct sock_common [4]. So, sock_copy() does not > overwrite skc_tx_queue_mapping, and thus we can initialize it in > sk_prot_alloc(). That is a lot of assumptions. What guarantees do we have that skc_tx_queue_mapping will never be moved out of this section ? AFAIK it was there by accident, for cache locality reasons, that might change in the future as we add more stuff in socket. I feel this optimization is risky for future changes, for a code path that is spending thousands of cycles anyway. > > [1] sk_prot_alloc > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1693 > > [2] sk_alloc > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1762 > > [3] sk_clone_lock > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1986 > > [4] struct sock_common > https://github.com/torvalds/linux/blob/master/include/net/sock.h#L218-L240 > > > > In any case, this seems to be a candidate for net-next, this is not > > fixing a bug, > > this would be an optimization at most, and potentially adding a bug. > > > > So if you resend this patch, you can mention the old commit in the changelog, > > but do not add a dubious Fixes: tag > > I see. > > I will remove the tag and resend this as a net-next candidate. > > Thank you, > Kuniyuki > > > > > > > > This patch removes the redundant calls of sk_tx_queue_clear() in sk_alloc() > > > and sk_clone_lock(). > > > > > > Fixes: 41b14fb8724d ("net: Do not clear the sock TX queue in sk_set_socket()") > > > CC: Tariq Toukan <tariqt@mellanox.com> > > > CC: Boris Pismenny <borisp@mellanox.com> > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> > > > Reviewed-by: Amit Shah <aams@amazon.de> > > > --- > > > net/core/sock.c | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > > index bbcd4b97eddd..5c665ee14159 100644 > > > --- a/net/core/sock.c > > > +++ b/net/core/sock.c > > > @@ -1759,7 +1759,6 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority, > > > cgroup_sk_alloc(&sk->sk_cgrp_data); > > > sock_update_classid(&sk->sk_cgrp_data); > > > sock_update_netprioidx(&sk->sk_cgrp_data); > > > - sk_tx_queue_clear(sk); > > > } > > > > > > return sk; > > > @@ -1983,7 +1982,6 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority) > > > */ > > > sk_refcnt_debug_inc(newsk); > > > sk_set_socket(newsk, NULL); > > > - sk_tx_queue_clear(newsk); > > > RCU_INIT_POINTER(newsk->sk_wq, NULL); > > > > > > if (newsk->sk_prot->sockets_allocated) > > > -- > > > 2.17.2 (Apple Git-113) > > >
From: Eric Dumazet <edumazet@google.com> Date: Wed, 27 Jan 2021 18:05:24 +0100 > On Wed, Jan 27, 2021 at 5:52 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > From: Eric Dumazet <edumazet@google.com> > > Date: Wed, 27 Jan 2021 15:54:32 +0100 > > > On Wed, Jan 27, 2021 at 1:50 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > > > > > The commit 41b14fb8724d ("net: Do not clear the sock TX queue in > > > > sk_set_socket()") removes sk_tx_queue_clear() from sk_set_socket() and adds > > > > it instead in sk_alloc() and sk_clone_lock() to fix an issue introduced in > > > > the commit e022f0b4a03f ("net: Introduce sk_tx_queue_mapping"). However, > > > > the original commit had already put sk_tx_queue_clear() in sk_prot_alloc(): > > > > the callee of sk_alloc() and sk_clone_lock(). Thus sk_tx_queue_clear() is > > > > called twice in each path currently. > > > > > > Are you sure ? > > > > > > I do not clearly see the sk_tx_queue_clear() call from the cloning part. > > > > > > Please elaborate. > > > > If sk is not NULL in sk_prot_alloc(), sk_tx_queue_clear() is called [1]. > > Also the callers of sk_prot_alloc() are only sk_alloc() and sk_clone_lock(). > > If they finally return not NULL pointer, sk_tx_queue_clear() is called in > > each function [2][3]. > > > > In the cloning part, sock_copy() is called after sk_prot_alloc(), but > > skc_tx_queue_mapping is defined between skc_dontcopy_begin and > > skc_dontcopy_end in struct sock_common [4]. So, sock_copy() does not > > overwrite skc_tx_queue_mapping, and thus we can initialize it in > > sk_prot_alloc(). > > That is a lot of assumptions. > > What guarantees do we have that skc_tx_queue_mapping will never be > moved out of this section ? > AFAIK it was there by accident, for cache locality reasons, that might > change in the future as we add more stuff in socket. > > I feel this optimization is risky for future changes, for a code path > that is spending thousands of cycles anyway. If someone try to move skc_tx_queue_mapping out of the section, should they take care about where it is used ? But I agree that we should not write error-prone code. Currently, sk_tx_queue_clear() is the only initialization code in sk_prot_alloc(). So, does it make sense to remove sk_tx_queue_clear() in sk_prot_alloc() so that it does only allocation and other fields are initialized in each caller ? > > > > [1] sk_prot_alloc > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1693 > > > > [2] sk_alloc > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1762 > > > > [3] sk_clone_lock > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1986 > > > > [4] struct sock_common > > https://github.com/torvalds/linux/blob/master/include/net/sock.h#L218-L240 > > > > > > > In any case, this seems to be a candidate for net-next, this is not > > > fixing a bug, > > > this would be an optimization at most, and potentially adding a bug. > > > > > > So if you resend this patch, you can mention the old commit in the changelog, > > > but do not add a dubious Fixes: tag > > > > I see. > > > > I will remove the tag and resend this as a net-next candidate. > > > > Thank you, > > Kuniyuki > > > > > > > > > > > > This patch removes the redundant calls of sk_tx_queue_clear() in sk_alloc() > > > > and sk_clone_lock(). > > > > > > > > Fixes: 41b14fb8724d ("net: Do not clear the sock TX queue in sk_set_socket()") > > > > CC: Tariq Toukan <tariqt@mellanox.com> > > > > CC: Boris Pismenny <borisp@mellanox.com> > > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> > > > > Reviewed-by: Amit Shah <aams@amazon.de> > > > > --- > > > > net/core/sock.c | 2 -- > > > > 1 file changed, 2 deletions(-) > > > > > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > > > index bbcd4b97eddd..5c665ee14159 100644 > > > > --- a/net/core/sock.c > > > > +++ b/net/core/sock.c > > > > @@ -1759,7 +1759,6 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority, > > > > cgroup_sk_alloc(&sk->sk_cgrp_data); > > > > sock_update_classid(&sk->sk_cgrp_data); > > > > sock_update_netprioidx(&sk->sk_cgrp_data); > > > > - sk_tx_queue_clear(sk); > > > > } > > > > > > > > return sk; > > > > @@ -1983,7 +1982,6 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority) > > > > */ > > > > sk_refcnt_debug_inc(newsk); > > > > sk_set_socket(newsk, NULL); > > > > - sk_tx_queue_clear(newsk); > > > > RCU_INIT_POINTER(newsk->sk_wq, NULL); > > > > > > > > if (newsk->sk_prot->sockets_allocated) > > > > -- > > > > 2.17.2 (Apple Git-113) > > > >
On Wed, Jan 27, 2021 at 6:32 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > From: Eric Dumazet <edumazet@google.com> > Date: Wed, 27 Jan 2021 18:05:24 +0100 > > On Wed, Jan 27, 2021 at 5:52 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > > > From: Eric Dumazet <edumazet@google.com> > > > Date: Wed, 27 Jan 2021 15:54:32 +0100 > > > > On Wed, Jan 27, 2021 at 1:50 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > > > > > > > The commit 41b14fb8724d ("net: Do not clear the sock TX queue in > > > > > sk_set_socket()") removes sk_tx_queue_clear() from sk_set_socket() and adds > > > > > it instead in sk_alloc() and sk_clone_lock() to fix an issue introduced in > > > > > the commit e022f0b4a03f ("net: Introduce sk_tx_queue_mapping"). However, > > > > > the original commit had already put sk_tx_queue_clear() in sk_prot_alloc(): > > > > > the callee of sk_alloc() and sk_clone_lock(). Thus sk_tx_queue_clear() is > > > > > called twice in each path currently. > > > > > > > > Are you sure ? > > > > > > > > I do not clearly see the sk_tx_queue_clear() call from the cloning part. > > > > > > > > Please elaborate. > > > > > > If sk is not NULL in sk_prot_alloc(), sk_tx_queue_clear() is called [1]. > > > Also the callers of sk_prot_alloc() are only sk_alloc() and sk_clone_lock(). > > > If they finally return not NULL pointer, sk_tx_queue_clear() is called in > > > each function [2][3]. > > > > > > In the cloning part, sock_copy() is called after sk_prot_alloc(), but > > > skc_tx_queue_mapping is defined between skc_dontcopy_begin and > > > skc_dontcopy_end in struct sock_common [4]. So, sock_copy() does not > > > overwrite skc_tx_queue_mapping, and thus we can initialize it in > > > sk_prot_alloc(). > > > > That is a lot of assumptions. > > > > What guarantees do we have that skc_tx_queue_mapping will never be > > moved out of this section ? > > AFAIK it was there by accident, for cache locality reasons, that might > > change in the future as we add more stuff in socket. > > > > I feel this optimization is risky for future changes, for a code path > > that is spending thousands of cycles anyway. > > If someone try to move skc_tx_queue_mapping out of the section, should > they take care about where it is used ? Certainly not. You hide some knowledge, without a comment or some runtime check. You can not ask us (maintainers) to remember thousands of tricks. > > But I agree that we should not write error-prone code. > > Currently, sk_tx_queue_clear() is the only initialization code in > sk_prot_alloc(). So, does it make sense to remove sk_tx_queue_clear() in > sk_prot_alloc() so that it does only allocation and other fields are > initialized in each caller ? > > > > > > > > [1] sk_prot_alloc > > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1693 > > > > > > [2] sk_alloc > > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1762 > > > > > > [3] sk_clone_lock > > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1986 > > > > > > [4] struct sock_common > > > https://github.com/torvalds/linux/blob/master/include/net/sock.h#L218-L240 > > > > > > > > > > In any case, this seems to be a candidate for net-next, this is not > > > > fixing a bug, > > > > this would be an optimization at most, and potentially adding a bug. > > > > > > > > So if you resend this patch, you can mention the old commit in the changelog, > > > > but do not add a dubious Fixes: tag > > > > > > I see. > > > > > > I will remove the tag and resend this as a net-next candidate. > > > > > > Thank you, > > > Kuniyuki > > > > > > > > > > > > > > > > This patch removes the redundant calls of sk_tx_queue_clear() in sk_alloc() > > > > > and sk_clone_lock(). > > > > > > > > > > Fixes: 41b14fb8724d ("net: Do not clear the sock TX queue in sk_set_socket()") > > > > > CC: Tariq Toukan <tariqt@mellanox.com> > > > > > CC: Boris Pismenny <borisp@mellanox.com> > > > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> > > > > > Reviewed-by: Amit Shah <aams@amazon.de> > > > > > --- > > > > > net/core/sock.c | 2 -- > > > > > 1 file changed, 2 deletions(-) > > > > > > > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > > > > index bbcd4b97eddd..5c665ee14159 100644 > > > > > --- a/net/core/sock.c > > > > > +++ b/net/core/sock.c > > > > > @@ -1759,7 +1759,6 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority, > > > > > cgroup_sk_alloc(&sk->sk_cgrp_data); > > > > > sock_update_classid(&sk->sk_cgrp_data); > > > > > sock_update_netprioidx(&sk->sk_cgrp_data); > > > > > - sk_tx_queue_clear(sk); > > > > > } > > > > > > > > > > return sk; > > > > > @@ -1983,7 +1982,6 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority) > > > > > */ > > > > > sk_refcnt_debug_inc(newsk); > > > > > sk_set_socket(newsk, NULL); > > > > > - sk_tx_queue_clear(newsk); > > > > > RCU_INIT_POINTER(newsk->sk_wq, NULL); > > > > > > > > > > if (newsk->sk_prot->sockets_allocated) > > > > > -- > > > > > 2.17.2 (Apple Git-113) > > > > >
From: Eric Dumazet <edumazet@google.com> Date: Wed, 27 Jan 2021 18:34:35 +0100 > On Wed, Jan 27, 2021 at 6:32 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > From: Eric Dumazet <edumazet@google.com> > > Date: Wed, 27 Jan 2021 18:05:24 +0100 > > > On Wed, Jan 27, 2021 at 5:52 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > > > > > From: Eric Dumazet <edumazet@google.com> > > > > Date: Wed, 27 Jan 2021 15:54:32 +0100 > > > > > On Wed, Jan 27, 2021 at 1:50 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > > > > > > > > > The commit 41b14fb8724d ("net: Do not clear the sock TX queue in > > > > > > sk_set_socket()") removes sk_tx_queue_clear() from sk_set_socket() and adds > > > > > > it instead in sk_alloc() and sk_clone_lock() to fix an issue introduced in > > > > > > the commit e022f0b4a03f ("net: Introduce sk_tx_queue_mapping"). However, > > > > > > the original commit had already put sk_tx_queue_clear() in sk_prot_alloc(): > > > > > > the callee of sk_alloc() and sk_clone_lock(). Thus sk_tx_queue_clear() is > > > > > > called twice in each path currently. > > > > > > > > > > Are you sure ? > > > > > > > > > > I do not clearly see the sk_tx_queue_clear() call from the cloning part. > > > > > > > > > > Please elaborate. > > > > > > > > If sk is not NULL in sk_prot_alloc(), sk_tx_queue_clear() is called [1]. > > > > Also the callers of sk_prot_alloc() are only sk_alloc() and sk_clone_lock(). > > > > If they finally return not NULL pointer, sk_tx_queue_clear() is called in > > > > each function [2][3]. > > > > > > > > In the cloning part, sock_copy() is called after sk_prot_alloc(), but > > > > skc_tx_queue_mapping is defined between skc_dontcopy_begin and > > > > skc_dontcopy_end in struct sock_common [4]. So, sock_copy() does not > > > > overwrite skc_tx_queue_mapping, and thus we can initialize it in > > > > sk_prot_alloc(). > > > > > > That is a lot of assumptions. > > > > > > What guarantees do we have that skc_tx_queue_mapping will never be > > > moved out of this section ? > > > AFAIK it was there by accident, for cache locality reasons, that might > > > change in the future as we add more stuff in socket. > > > > > > I feel this optimization is risky for future changes, for a code path > > > that is spending thousands of cycles anyway. > > > > If someone try to move skc_tx_queue_mapping out of the section, should > > they take care about where it is used ? I'm sorry if it might be misleading, I would like to mean someone/they is the author of a patch to move skc_tx_queue_mapping. > Certainly not. You hide some knowledge, without a comment or some runtime check. It was my bad, I should have written about sock_copy() in the changelog. > You can not ask us (maintainers) to remember thousands of tricks. I'll keep this in mind. > > > > But I agree that we should not write error-prone code. > > > > Currently, sk_tx_queue_clear() is the only initialization code in > > sk_prot_alloc(). So, does it make sense to remove sk_tx_queue_clear() in > > sk_prot_alloc() so that it does only allocation and other fields are > > initialized in each caller ? Can I ask what you think about this ? > > > > > > > > [1] sk_prot_alloc > > > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1693 > > > > > > > > [2] sk_alloc > > > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1762 > > > > > > > > [3] sk_clone_lock > > > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1986 > > > > > > > > [4] struct sock_common > > > > https://github.com/torvalds/linux/blob/master/include/net/sock.h#L218-L240 > > > > > > > > > > > > > In any case, this seems to be a candidate for net-next, this is not > > > > > fixing a bug, > > > > > this would be an optimization at most, and potentially adding a bug. > > > > > > > > > > So if you resend this patch, you can mention the old commit in the changelog, > > > > > but do not add a dubious Fixes: tag > > > > > > > > I see. > > > > > > > > I will remove the tag and resend this as a net-next candidate. > > > > > > > > Thank you, > > > > Kuniyuki > > > > > > > > > > > > > > > > > > > > This patch removes the redundant calls of sk_tx_queue_clear() in sk_alloc() > > > > > > and sk_clone_lock(). > > > > > > > > > > > > Fixes: 41b14fb8724d ("net: Do not clear the sock TX queue in sk_set_socket()") > > > > > > CC: Tariq Toukan <tariqt@mellanox.com> > > > > > > CC: Boris Pismenny <borisp@mellanox.com> > > > > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> > > > > > > Reviewed-by: Amit Shah <aams@amazon.de> > > > > > > --- > > > > > > net/core/sock.c | 2 -- > > > > > > 1 file changed, 2 deletions(-) > > > > > > > > > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > > > > > index bbcd4b97eddd..5c665ee14159 100644 > > > > > > --- a/net/core/sock.c > > > > > > +++ b/net/core/sock.c > > > > > > @@ -1759,7 +1759,6 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority, > > > > > > cgroup_sk_alloc(&sk->sk_cgrp_data); > > > > > > sock_update_classid(&sk->sk_cgrp_data); > > > > > > sock_update_netprioidx(&sk->sk_cgrp_data); > > > > > > - sk_tx_queue_clear(sk); > > > > > > } > > > > > > > > > > > > return sk; > > > > > > @@ -1983,7 +1982,6 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority) > > > > > > */ > > > > > > sk_refcnt_debug_inc(newsk); > > > > > > sk_set_socket(newsk, NULL); > > > > > > - sk_tx_queue_clear(newsk); > > > > > > RCU_INIT_POINTER(newsk->sk_wq, NULL); > > > > > > > > > > > > if (newsk->sk_prot->sockets_allocated) > > > > > > -- > > > > > > 2.17.2 (Apple Git-113) > > > > > >
On Wed, Jan 27, 2021 at 6:56 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > From: Eric Dumazet <edumazet@google.com> > Date: Wed, 27 Jan 2021 18:34:35 +0100 > > On Wed, Jan 27, 2021 at 6:32 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > > > From: Eric Dumazet <edumazet@google.com> > > > Date: Wed, 27 Jan 2021 18:05:24 +0100 > > > > On Wed, Jan 27, 2021 at 5:52 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > > > > > > > From: Eric Dumazet <edumazet@google.com> > > > > > Date: Wed, 27 Jan 2021 15:54:32 +0100 > > > > > > On Wed, Jan 27, 2021 at 1:50 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > > > > > > > > > > > The commit 41b14fb8724d ("net: Do not clear the sock TX queue in > > > > > > > sk_set_socket()") removes sk_tx_queue_clear() from sk_set_socket() and adds > > > > > > > it instead in sk_alloc() and sk_clone_lock() to fix an issue introduced in > > > > > > > the commit e022f0b4a03f ("net: Introduce sk_tx_queue_mapping"). However, > > > > > > > the original commit had already put sk_tx_queue_clear() in sk_prot_alloc(): > > > > > > > the callee of sk_alloc() and sk_clone_lock(). Thus sk_tx_queue_clear() is > > > > > > > called twice in each path currently. > > > > > > > > > > > > Are you sure ? > > > > > > > > > > > > I do not clearly see the sk_tx_queue_clear() call from the cloning part. > > > > > > > > > > > > Please elaborate. > > > > > > > > > > If sk is not NULL in sk_prot_alloc(), sk_tx_queue_clear() is called [1]. > > > > > Also the callers of sk_prot_alloc() are only sk_alloc() and sk_clone_lock(). > > > > > If they finally return not NULL pointer, sk_tx_queue_clear() is called in > > > > > each function [2][3]. > > > > > > > > > > In the cloning part, sock_copy() is called after sk_prot_alloc(), but > > > > > skc_tx_queue_mapping is defined between skc_dontcopy_begin and > > > > > skc_dontcopy_end in struct sock_common [4]. So, sock_copy() does not > > > > > overwrite skc_tx_queue_mapping, and thus we can initialize it in > > > > > sk_prot_alloc(). > > > > > > > > That is a lot of assumptions. > > > > > > > > What guarantees do we have that skc_tx_queue_mapping will never be > > > > moved out of this section ? > > > > AFAIK it was there by accident, for cache locality reasons, that might > > > > change in the future as we add more stuff in socket. > > > > > > > > I feel this optimization is risky for future changes, for a code path > > > > that is spending thousands of cycles anyway. > > > > > > If someone try to move skc_tx_queue_mapping out of the section, should > > > they take care about where it is used ? > > I'm sorry if it might be misleading, I would like to mean someone/they is > the author of a patch to move skc_tx_queue_mapping. > > > > Certainly not. You hide some knowledge, without a comment or some runtime check. > > It was my bad, I should have written about sock_copy() in the changelog. I think you also want to add some compile time check. BUILD_BUG_ON( skc_tx_queue_mapping is in the no copy area) Because maintainers do not remember changelogs in their mind. > > > > You can not ask us (maintainers) to remember thousands of tricks. > > I'll keep this in mind. > > > > > > > > But I agree that we should not write error-prone code. > > > > > > Currently, sk_tx_queue_clear() is the only initialization code in > > > sk_prot_alloc(). So, does it make sense to remove sk_tx_queue_clear() in > > > sk_prot_alloc() so that it does only allocation and other fields are > > > initialized in each caller ? > > Can I ask what you think about this ? Yes, this would be fine. > > > > > > > > > > > > [1] sk_prot_alloc > > > > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1693 > > > > > > > > > > [2] sk_alloc > > > > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1762 > > > > > > > > > > [3] sk_clone_lock > > > > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1986 > > > > > > > > > > [4] struct sock_common > > > > > https://github.com/torvalds/linux/blob/master/include/net/sock.h#L218-L240 > > > > > > > > > > > > > > > > In any case, this seems to be a candidate for net-next, this is not > > > > > > fixing a bug, > > > > > > this would be an optimization at most, and potentially adding a bug. > > > > > > > > > > > > So if you resend this patch, you can mention the old commit in the changelog, > > > > > > but do not add a dubious Fixes: tag > > > > > > > > > > I see. > > > > > > > > > > I will remove the tag and resend this as a net-next candidate. > > > > > > > > > > Thank you, > > > > > Kuniyuki > > > > > > > > > > > > > > > > > > > > > > > > This patch removes the redundant calls of sk_tx_queue_clear() in sk_alloc() > > > > > > > and sk_clone_lock(). > > > > > > > > > > > > > > Fixes: 41b14fb8724d ("net: Do not clear the sock TX queue in sk_set_socket()") > > > > > > > CC: Tariq Toukan <tariqt@mellanox.com> > > > > > > > CC: Boris Pismenny <borisp@mellanox.com> > > > > > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> > > > > > > > Reviewed-by: Amit Shah <aams@amazon.de> > > > > > > > --- > > > > > > > net/core/sock.c | 2 -- > > > > > > > 1 file changed, 2 deletions(-) > > > > > > > > > > > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > > > > > > index bbcd4b97eddd..5c665ee14159 100644 > > > > > > > --- a/net/core/sock.c > > > > > > > +++ b/net/core/sock.c > > > > > > > @@ -1759,7 +1759,6 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority, > > > > > > > cgroup_sk_alloc(&sk->sk_cgrp_data); > > > > > > > sock_update_classid(&sk->sk_cgrp_data); > > > > > > > sock_update_netprioidx(&sk->sk_cgrp_data); > > > > > > > - sk_tx_queue_clear(sk); > > > > > > > } > > > > > > > > > > > > > > return sk; > > > > > > > @@ -1983,7 +1982,6 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority) > > > > > > > */ > > > > > > > sk_refcnt_debug_inc(newsk); > > > > > > > sk_set_socket(newsk, NULL); > > > > > > > - sk_tx_queue_clear(newsk); > > > > > > > RCU_INIT_POINTER(newsk->sk_wq, NULL); > > > > > > > > > > > > > > if (newsk->sk_prot->sockets_allocated) > > > > > > > -- > > > > > > > 2.17.2 (Apple Git-113) > > > > > > >
From: Eric Dumazet <edumazet@google.com> Date: Wed, 27 Jan 2021 19:07:51 +0100 > On Wed, Jan 27, 2021 at 6:56 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > From: Eric Dumazet <edumazet@google.com> > > Date: Wed, 27 Jan 2021 18:34:35 +0100 > > > On Wed, Jan 27, 2021 at 6:32 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > > > > > From: Eric Dumazet <edumazet@google.com> > > > > Date: Wed, 27 Jan 2021 18:05:24 +0100 > > > > > On Wed, Jan 27, 2021 at 5:52 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > > > > > > > > > From: Eric Dumazet <edumazet@google.com> > > > > > > Date: Wed, 27 Jan 2021 15:54:32 +0100 > > > > > > > On Wed, Jan 27, 2021 at 1:50 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote: > > > > > > > > > > > > > > > > The commit 41b14fb8724d ("net: Do not clear the sock TX queue in > > > > > > > > sk_set_socket()") removes sk_tx_queue_clear() from sk_set_socket() and adds > > > > > > > > it instead in sk_alloc() and sk_clone_lock() to fix an issue introduced in > > > > > > > > the commit e022f0b4a03f ("net: Introduce sk_tx_queue_mapping"). However, > > > > > > > > the original commit had already put sk_tx_queue_clear() in sk_prot_alloc(): > > > > > > > > the callee of sk_alloc() and sk_clone_lock(). Thus sk_tx_queue_clear() is > > > > > > > > called twice in each path currently. > > > > > > > > > > > > > > Are you sure ? > > > > > > > > > > > > > > I do not clearly see the sk_tx_queue_clear() call from the cloning part. > > > > > > > > > > > > > > Please elaborate. > > > > > > > > > > > > If sk is not NULL in sk_prot_alloc(), sk_tx_queue_clear() is called [1]. > > > > > > Also the callers of sk_prot_alloc() are only sk_alloc() and sk_clone_lock(). > > > > > > If they finally return not NULL pointer, sk_tx_queue_clear() is called in > > > > > > each function [2][3]. > > > > > > > > > > > > In the cloning part, sock_copy() is called after sk_prot_alloc(), but > > > > > > skc_tx_queue_mapping is defined between skc_dontcopy_begin and > > > > > > skc_dontcopy_end in struct sock_common [4]. So, sock_copy() does not > > > > > > overwrite skc_tx_queue_mapping, and thus we can initialize it in > > > > > > sk_prot_alloc(). > > > > > > > > > > That is a lot of assumptions. > > > > > > > > > > What guarantees do we have that skc_tx_queue_mapping will never be > > > > > moved out of this section ? > > > > > AFAIK it was there by accident, for cache locality reasons, that might > > > > > change in the future as we add more stuff in socket. > > > > > > > > > > I feel this optimization is risky for future changes, for a code path > > > > > that is spending thousands of cycles anyway. > > > > > > > > If someone try to move skc_tx_queue_mapping out of the section, should > > > > they take care about where it is used ? > > > > I'm sorry if it might be misleading, I would like to mean someone/they is > > the author of a patch to move skc_tx_queue_mapping. > > > > > > > Certainly not. You hide some knowledge, without a comment or some runtime check. > > > > It was my bad, I should have written about sock_copy() in the changelog. > > I think you also want to add some compile time check. > > BUILD_BUG_ON( skc_tx_queue_mapping is in the no copy area) > > Because maintainers do not remember changelogs in their mind. I understand. The proper place to add BUILD_BUG_ON() is sock_copy() or sk_clone_lock() ? > > > > > > > You can not ask us (maintainers) to remember thousands of tricks. > > > > I'll keep this in mind. > > > > > > > > > > > > But I agree that we should not write error-prone code. > > > > > > > > Currently, sk_tx_queue_clear() is the only initialization code in > > > > sk_prot_alloc(). So, does it make sense to remove sk_tx_queue_clear() in > > > > sk_prot_alloc() so that it does only allocation and other fields are > > > > initialized in each caller ? > > > > Can I ask what you think about this ? > > Yes, this would be fine. Thank you, I will remove the sk_tx_queue_clear() in sk_prot_alloc(). > > > > > > > > > > > > > > [1] sk_prot_alloc > > > > > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1693 > > > > > > > > > > > > [2] sk_alloc > > > > > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1762 > > > > > > > > > > > > [3] sk_clone_lock > > > > > > https://github.com/torvalds/linux/blob/master/net/core/sock.c#L1986 > > > > > > > > > > > > [4] struct sock_common > > > > > > https://github.com/torvalds/linux/blob/master/include/net/sock.h#L218-L240 > > > > > > > > > > > > > > > > > > > In any case, this seems to be a candidate for net-next, this is not > > > > > > > fixing a bug, > > > > > > > this would be an optimization at most, and potentially adding a bug. > > > > > > > > > > > > > > So if you resend this patch, you can mention the old commit in the changelog, > > > > > > > but do not add a dubious Fixes: tag > > > > > > > > > > > > I see. > > > > > > > > > > > > I will remove the tag and resend this as a net-next candidate. > > > > > > > > > > > > Thank you, > > > > > > Kuniyuki > > > > > > > > > > > > > > > > > > > > > > > > > > > > This patch removes the redundant calls of sk_tx_queue_clear() in sk_alloc() > > > > > > > > and sk_clone_lock(). > > > > > > > > > > > > > > > > Fixes: 41b14fb8724d ("net: Do not clear the sock TX queue in sk_set_socket()") > > > > > > > > CC: Tariq Toukan <tariqt@mellanox.com> > > > > > > > > CC: Boris Pismenny <borisp@mellanox.com> > > > > > > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> > > > > > > > > Reviewed-by: Amit Shah <aams@amazon.de> > > > > > > > > --- > > > > > > > > net/core/sock.c | 2 -- > > > > > > > > 1 file changed, 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > > > > > > > index bbcd4b97eddd..5c665ee14159 100644 > > > > > > > > --- a/net/core/sock.c > > > > > > > > +++ b/net/core/sock.c > > > > > > > > @@ -1759,7 +1759,6 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority, > > > > > > > > cgroup_sk_alloc(&sk->sk_cgrp_data); > > > > > > > > sock_update_classid(&sk->sk_cgrp_data); > > > > > > > > sock_update_netprioidx(&sk->sk_cgrp_data); > > > > > > > > - sk_tx_queue_clear(sk); > > > > > > > > } > > > > > > > > > > > > > > > > return sk; > > > > > > > > @@ -1983,7 +1982,6 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority) > > > > > > > > */ > > > > > > > > sk_refcnt_debug_inc(newsk); > > > > > > > > sk_set_socket(newsk, NULL); > > > > > > > > - sk_tx_queue_clear(newsk); > > > > > > > > RCU_INIT_POINTER(newsk->sk_wq, NULL); > > > > > > > > > > > > > > > > if (newsk->sk_prot->sockets_allocated) > > > > > > > > -- > > > > > > > > 2.17.2 (Apple Git-113) > > > > > > > >
diff --git a/net/core/sock.c b/net/core/sock.c index bbcd4b97eddd..5c665ee14159 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1759,7 +1759,6 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority, cgroup_sk_alloc(&sk->sk_cgrp_data); sock_update_classid(&sk->sk_cgrp_data); sock_update_netprioidx(&sk->sk_cgrp_data); - sk_tx_queue_clear(sk); } return sk; @@ -1983,7 +1982,6 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority) */ sk_refcnt_debug_inc(newsk); sk_set_socket(newsk, NULL); - sk_tx_queue_clear(newsk); RCU_INIT_POINTER(newsk->sk_wq, NULL); if (newsk->sk_prot->sockets_allocated)
The commit 41b14fb8724d ("net: Do not clear the sock TX queue in sk_set_socket()") removes sk_tx_queue_clear() from sk_set_socket() and adds it instead in sk_alloc() and sk_clone_lock() to fix an issue introduced in the commit e022f0b4a03f ("net: Introduce sk_tx_queue_mapping"). However, the original commit had already put sk_tx_queue_clear() in sk_prot_alloc(): the callee of sk_alloc() and sk_clone_lock(). Thus sk_tx_queue_clear() is called twice in each path currently. This patch removes the redundant calls of sk_tx_queue_clear() in sk_alloc() and sk_clone_lock(). Fixes: 41b14fb8724d ("net: Do not clear the sock TX queue in sk_set_socket()") CC: Tariq Toukan <tariqt@mellanox.com> CC: Boris Pismenny <borisp@mellanox.com> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> Reviewed-by: Amit Shah <aams@amazon.de> --- net/core/sock.c | 2 -- 1 file changed, 2 deletions(-)