Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bfbfc15f-3d16-f127-0642-d5f78147a58d@bytedance.com>
Date:   Mon, 15 May 2023 18:20:21 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.10.1
Subject: Re: Re: [PATCH] sock: Fix misuse of sk_under_memory_pressure()
Content-Language: en-US
To:     Eric Dumazet <edumazet@google.com>
Cc:     Paolo Abeni <pabeni@redhat.com>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230506085903.96133-1-wuyun.abel@bytedance.com>
 <588689343dcd6c904e7fc142a001043015e5b14e.camel@redhat.com>
 <d2abfe0c-0152-860c-60f7-2787973c95d0@bytedance.com>
 <6b355d57-30b4-748d-87f4-d79a50fe5487@bytedance.com>
 <CANn89iJqzE6r9dm2hoHxgYeLQvStZYvRhCxFVmpV_LczaO-4xw@mail.gmail.com>
From:   Abel Wu <wuyun.abel@bytedance.com>
In-Reply-To: <CANn89iJqzE6r9dm2hoHxgYeLQvStZYvRhCxFVmpV_LczaO-4xw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/15/23 3:21 PM, Eric Dumazet wrote:>
> I still do not understand the patch.
> 
> If I do not understand the patch and its changelog now in May 2023,
> how will anyone understand it later
> when/if a regression is investigated ?
> 
> I repeat :
> 
> Changelog is evasive, I do not see what practical problem you want to solve.
> 
> 
> sk_has_memory_pressure() is not about memcg, simply the fact that a
> proto has a non NULL memory_pressure pointer.

Yes, it has nothing to do with sk_has_memory_pressure(), this accessor
is removed only due to it is not used anymore after this fix. I really
should have put this into a separate patch.

> 
> I suggest that you answer these questions, and send a V2 with an
> updated changelog.

OK, I will.

> 
> Again, what is the practical problem you want to solve ?
> What is the behavior of the current stack that you think is a problem ?

The status of global tcp_mem pressure is updated when:

   a) __sk_mem_raise_allocated():

	enter: sk_memory_allocated(sk) >  tcp_mem[1]
	leave: sk_memory_allocated(sk) <= tcp_mem[0]

   b) __sk_mem_reduce_allocated():

	leave: sk_under_memory_pressure(sk) &&
		sk_memory_allocated(sk) < tcp_mem[0]

So the conditions of leaving global pressure are inconstant, which may
lead to the situation that one pressured memcg prevents the global
pressure from being cleared when there is indeed no global pressure,
thus the global constrains are still in effect unexpectedly on the other
sockets. The patch fixes this by removing the condition of net-memcg's
pressure in __sk_mem_reduce_allocated().

As for the changes in __sk_mem_raise_allocated(), I don't think it is
the right place to check the pressure of the @sk's memcg. That piece of
code was originally only trying to be fair between all the sockets if
there is global pressure. And if we really want to forbid the socket
memory from being raised when the socket's memcg is in pressure, the
condition should be in the first place inside this function.

So I plan to split this patch into three in v2:

   [1/3] fix inconstant condition in __sk_mem_reduce_allocated()
   [2/3] remove unrelated check in __sk_mem_raise_allocated()
   [3/3] remove sk_has_memory_pressure() since it is no longer used

Does this make sense to you?

Thanks & Best,
	Abel