| Message ID | 20230619082547.73929-1-wuyun.abel@bytedance.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] inet: Save one atomic op if no memcg to charge | expand |
On Mon, Jun 19, 2023 at 10:26 AM Abel Wu <wuyun.abel@bytedance.com> wrote: > > If there is no net-memcg associated with the sock, don't bother > calculating its memory usage for charge. > > Signed-off-by: Abel Wu <wuyun.abel@bytedance.com> > --- > net/ipv4/inet_connection_sock.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c > index 65ad4251f6fd..73798282c1ef 100644 > --- a/net/ipv4/inet_connection_sock.c > +++ b/net/ipv4/inet_connection_sock.c > @@ -706,20 +706,24 @@ struct sock *inet_csk_accept(struct sock *sk, int flags, int *err, bool kern) > out: > release_sock(sk); > if (newsk && mem_cgroup_sockets_enabled) { > - int amt; > + int amt = 0; > > /* atomically get the memory usage, set and charge the > * newsk->sk_memcg. > */ > lock_sock(newsk); > > - /* The socket has not been accepted yet, no need to look at > - * newsk->sk_wmem_queued. > - */ > - amt = sk_mem_pages(newsk->sk_forward_alloc + > - atomic_read(&newsk->sk_rmem_alloc)); > mem_cgroup_sk_alloc(newsk); > - if (newsk->sk_memcg && amt) > + if (newsk->sk_memcg) { > + /* The socket has not been accepted yet, no need > + * to look at newsk->sk_wmem_queued. > + */ > + amt = sk_mem_pages(newsk->sk_forward_alloc + > + atomic_read(&newsk->sk_rmem_alloc)); > + > + } > + > + if (amt) > mem_cgroup_charge_skmem(newsk->sk_memcg, amt, > GFP_KERNEL | __GFP_NOFAIL); This looks correct, but claiming reading an atomic_t is an 'atomic op' is a bit exaggerated.
On 6/19/23 6:08 PM, Eric Dumazet wrote: > On Mon, Jun 19, 2023 at 10:26 AM Abel Wu <wuyun.abel@bytedance.com> wrote: >> >> If there is no net-memcg associated with the sock, don't bother >> calculating its memory usage for charge. >> >> Signed-off-by: Abel Wu <wuyun.abel@bytedance.com> >> --- >> net/ipv4/inet_connection_sock.c | 18 +++++++++++------- >> 1 file changed, 11 insertions(+), 7 deletions(-) >> >> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c >> index 65ad4251f6fd..73798282c1ef 100644 >> --- a/net/ipv4/inet_connection_sock.c >> +++ b/net/ipv4/inet_connection_sock.c >> @@ -706,20 +706,24 @@ struct sock *inet_csk_accept(struct sock *sk, int flags, int *err, bool kern) >> out: >> release_sock(sk); >> if (newsk && mem_cgroup_sockets_enabled) { >> - int amt; >> + int amt = 0; >> >> /* atomically get the memory usage, set and charge the >> * newsk->sk_memcg. >> */ >> lock_sock(newsk); >> >> - /* The socket has not been accepted yet, no need to look at >> - * newsk->sk_wmem_queued. >> - */ >> - amt = sk_mem_pages(newsk->sk_forward_alloc + >> - atomic_read(&newsk->sk_rmem_alloc)); >> mem_cgroup_sk_alloc(newsk); >> - if (newsk->sk_memcg && amt) >> + if (newsk->sk_memcg) { >> + /* The socket has not been accepted yet, no need >> + * to look at newsk->sk_wmem_queued. >> + */ >> + amt = sk_mem_pages(newsk->sk_forward_alloc + >> + atomic_read(&newsk->sk_rmem_alloc)); >> + >> + } >> + >> + if (amt) >> mem_cgroup_charge_skmem(newsk->sk_memcg, amt, >> GFP_KERNEL | __GFP_NOFAIL); > > This looks correct, but claiming reading an atomic_t is an 'atomic op' > is a bit exaggerated. Yeah, shall I change subject to 'inet: Skip usage calculation if no memcg to charge'? Or do you have any suggestions? Thanks, Abel
On Tue, Jun 20, 2023 at 5:04 AM Abel Wu <wuyun.abel@bytedance.com> wrote: > > On 6/19/23 6:08 PM, Eric Dumazet wrote: > > On Mon, Jun 19, 2023 at 10:26 AM Abel Wu <wuyun.abel@bytedance.com> wrote: > >> > >> If there is no net-memcg associated with the sock, don't bother > >> calculating its memory usage for charge. > >> > >> Signed-off-by: Abel Wu <wuyun.abel@bytedance.com> > >> --- > >> net/ipv4/inet_connection_sock.c | 18 +++++++++++------- > >> 1 file changed, 11 insertions(+), 7 deletions(-) > >> > >> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c > >> index 65ad4251f6fd..73798282c1ef 100644 > >> --- a/net/ipv4/inet_connection_sock.c > >> +++ b/net/ipv4/inet_connection_sock.c > >> @@ -706,20 +706,24 @@ struct sock *inet_csk_accept(struct sock *sk, int flags, int *err, bool kern) > >> out: > >> release_sock(sk); > >> if (newsk && mem_cgroup_sockets_enabled) { > >> - int amt; > >> + int amt = 0; > >> > >> /* atomically get the memory usage, set and charge the > >> * newsk->sk_memcg. > >> */ > >> lock_sock(newsk); > >> > >> - /* The socket has not been accepted yet, no need to look at > >> - * newsk->sk_wmem_queued. > >> - */ > >> - amt = sk_mem_pages(newsk->sk_forward_alloc + > >> - atomic_read(&newsk->sk_rmem_alloc)); > >> mem_cgroup_sk_alloc(newsk); > >> - if (newsk->sk_memcg && amt) > >> + if (newsk->sk_memcg) { > >> + /* The socket has not been accepted yet, no need > >> + * to look at newsk->sk_wmem_queued. > >> + */ > >> + amt = sk_mem_pages(newsk->sk_forward_alloc + > >> + atomic_read(&newsk->sk_rmem_alloc)); > >> + > >> + } > >> + > >> + if (amt) > >> mem_cgroup_charge_skmem(newsk->sk_memcg, amt, > >> GFP_KERNEL | __GFP_NOFAIL); > > > > This looks correct, but claiming reading an atomic_t is an 'atomic op' > > is a bit exaggerated. > > Yeah, shall I change subject to 'inet: Skip usage calculation if no > memcg to charge'? Or do you have any suggestions? I would call this a cleanup or refactoring, maybe...
On 6/20/23 4:46 PM, Eric Dumazet wrote: > On Tue, Jun 20, 2023 at 5:04 AM Abel Wu <wuyun.abel@bytedance.com> wrote: >> >> On 6/19/23 6:08 PM, Eric Dumazet wrote: >>> On Mon, Jun 19, 2023 at 10:26 AM Abel Wu <wuyun.abel@bytedance.com> wrote: >>>> >>>> If there is no net-memcg associated with the sock, don't bother >>>> calculating its memory usage for charge. >>>> >>>> Signed-off-by: Abel Wu <wuyun.abel@bytedance.com> >>>> --- >>>> net/ipv4/inet_connection_sock.c | 18 +++++++++++------- >>>> 1 file changed, 11 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c >>>> index 65ad4251f6fd..73798282c1ef 100644 >>>> --- a/net/ipv4/inet_connection_sock.c >>>> +++ b/net/ipv4/inet_connection_sock.c >>>> @@ -706,20 +706,24 @@ struct sock *inet_csk_accept(struct sock *sk, int flags, int *err, bool kern) >>>> out: >>>> release_sock(sk); >>>> if (newsk && mem_cgroup_sockets_enabled) { >>>> - int amt; >>>> + int amt = 0; >>>> >>>> /* atomically get the memory usage, set and charge the >>>> * newsk->sk_memcg. >>>> */ >>>> lock_sock(newsk); >>>> >>>> - /* The socket has not been accepted yet, no need to look at >>>> - * newsk->sk_wmem_queued. >>>> - */ >>>> - amt = sk_mem_pages(newsk->sk_forward_alloc + >>>> - atomic_read(&newsk->sk_rmem_alloc)); >>>> mem_cgroup_sk_alloc(newsk); >>>> - if (newsk->sk_memcg && amt) >>>> + if (newsk->sk_memcg) { >>>> + /* The socket has not been accepted yet, no need >>>> + * to look at newsk->sk_wmem_queued. >>>> + */ >>>> + amt = sk_mem_pages(newsk->sk_forward_alloc + >>>> + atomic_read(&newsk->sk_rmem_alloc)); >>>> + >>>> + } >>>> + >>>> + if (amt) >>>> mem_cgroup_charge_skmem(newsk->sk_memcg, amt, >>>> GFP_KERNEL | __GFP_NOFAIL); >>> >>> This looks correct, but claiming reading an atomic_t is an 'atomic op' >>> is a bit exaggerated. >> >> Yeah, shall I change subject to 'inet: Skip usage calculation if no >> memcg to charge'? Or do you have any suggestions? > > I would call this a cleanup or refactoring, maybe... Alright, I have changed to 'cleanup', please take a look at v2. Yet I have another question about this condition: 'if (newsk && mem_cgroup_sockets_enabled)' IMHO in the scope of cgroup v1, 'mem_cgroup_sockets_enabled' doesn't imply socket accounting enabled for current's memcg. As the listening socket and the newly accepted socket are processing same traffic, can we make this condition more specific like this: 'if (newsk && mem_cgroup_sockets_enabled && sk->sk_memcg)' would you mind shedding some light please? Thanks! Abel
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c index 65ad4251f6fd..73798282c1ef 100644 --- a/net/ipv4/inet_connection_sock.c +++ b/net/ipv4/inet_connection_sock.c @@ -706,20 +706,24 @@ struct sock *inet_csk_accept(struct sock *sk, int flags, int *err, bool kern) out: release_sock(sk); if (newsk && mem_cgroup_sockets_enabled) { - int amt; + int amt = 0; /* atomically get the memory usage, set and charge the * newsk->sk_memcg. */ lock_sock(newsk); - /* The socket has not been accepted yet, no need to look at - * newsk->sk_wmem_queued. - */ - amt = sk_mem_pages(newsk->sk_forward_alloc + - atomic_read(&newsk->sk_rmem_alloc)); mem_cgroup_sk_alloc(newsk); - if (newsk->sk_memcg && amt) + if (newsk->sk_memcg) { + /* The socket has not been accepted yet, no need + * to look at newsk->sk_wmem_queued. + */ + amt = sk_mem_pages(newsk->sk_forward_alloc + + atomic_read(&newsk->sk_rmem_alloc)); + + } + + if (amt) mem_cgroup_charge_skmem(newsk->sk_memcg, amt, GFP_KERNEL | __GFP_NOFAIL);
If there is no net-memcg associated with the sock, don't bother calculating its memory usage for charge. Signed-off-by: Abel Wu <wuyun.abel@bytedance.com> --- net/ipv4/inet_connection_sock.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)