If there is no net-memcg associated with the sock, don't bother
calculating its memory usage for charge.
Signed-off-by: Abel Wu <[email protected]>
---
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);
--
2.37.3
On Mon, Jun 19, 2023 at 10:26 AM Abel Wu <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]> wrote:
>
> On 6/19/23 6:08 PM, Eric Dumazet wrote:
> > On Mon, Jun 19, 2023 at 10:26 AM Abel Wu <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]> wrote:
>>
>> On 6/19/23 6:08 PM, Eric Dumazet wrote:
>>> On Mon, Jun 19, 2023 at 10:26 AM Abel Wu <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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