On Thu 21-07-22 08:58:06, Yosry Ahmed wrote: > On Thu, Jul 21, 2022 at 4:44 AM Michal Hocko <mhocko@xxxxxxxx> wrote: > > > > On Wed 20-07-22 11:02:56, Yosry Ahmed wrote: > > > On Wed, Jul 20, 2022 at 10:50 AM Shakeel Butt <shakeelb@xxxxxxxxxx> wrote: > > > > > > > > On Wed, Jul 20, 2022 at 2:24 AM Michal Hocko <mhocko@xxxxxxxx> wrote: > > > > > > > > > [...] > > > > > > > > > > I think what we are missing here is > > > > > - explain that this doesn't have any effect on existing users of > > > > > vmpressure user interface because that is cgroup v1 and memory.reclaim > > > > > is v2 feature. This is a trivial statement but quite useful for future > > > > > readers of this commit > > > > > - explain the effect on the networking layer and typical usecases > > > > > memory.reclaim is used for currently and ideally document that. > > > > > > > > I agree with the above two points (Yosry, please address those) but > > > > the following third point is orthogonal and we don't really need to > > > > have an answer for this patch to be accepted. > > > > > > > > > > That's great feedback, thanks Michal and Shakeel! > > > > > > How do you feel about the following commit message instead? Does it > > > address your concerns?: > > > > > > memory.reclaim is a cgroup v2 interface that allows users to > > > proactively reclaim memory from a memcg, without real memory pressure. > > > Reclaim operations invoke vmpressure, which is used in cgroup v1 to > > > notify userspace of reclaim efficiency, and used in both v1 and v2 as > > > a signal for a memcg being under memory pressure for networking (see > > > mem_cgroup_under_socket_pressure()). For the former, vmpressure > > > notifications in v1 are not affected by this change since > > > memory.reclaim is a v2 feature. > > > > > > For the latter, the effects of the vmpressure signal (according to > > > Shakeel [1]) are as follows: > > > 1. Reducing send and receive buffers of the current socket. > > > 2. May drop packets on the rx path. > > > 3. May throttle current thread on the tx path. > > > > > > Since proactive reclaim is invoked directly by userspace, not by > > > memory pressure, it makes sense not to throttle networking. Hence, > > > this change makes sure that proactive reclaim caused by memory.reclaim > > > does not trigger vmpressure. > > > > OK, looks much better. Please also add a note to the documentation about > > this side effect. > > I don't want to add something to the documentation about throttling > networking because it seems like these are implementation details that > we may change in the future. I don't know if we can document this > behavior today and then change it later. The exact mechanism on how the throttling is done is one thing. This can change. But the fact that _no_ throttling is applied is something that we shouldn't change of course. If we were really strict we shouldn't change it even now but considering that the interface is new and usecases still shaping then better now than later. > How about we document a more generic statement in memory.reclaim > documentation, like: > > "With reactive reclaim operations triggered by the kernel, the kernel > may take further actions to alleviate memory pressure (such as > throttling networking memory consumption). For proactive reclaim > operations triggered by this interface, the kernel may choose to skip > such actions as reclaim is not an indication of memory pressure." IDK, this sounds too much word lawyering to me TBH. It is better to be clear about explicitly known side effects. For example where do shrinkers stand in the light of above wording? Kernel can chose to do almost anything and I do not think we want to control which shrinkers are triggered and what they do. So I would really prefer to say something like: " Please note that the proactive reclaim (triggered by this interface) is not meant to indicate memory pressure on the memory cgroup. Therefore socket memory balancing triggered by the memory reclaim normally is not exercised in this case. This means that the networking layer will not adapt based on reclaim induced by memory.reclaim. " -- Michal Hocko SUSE Labs
