On Thu, Jul 21, 2022 at 9:42 AM Michal Hocko <mhocko@xxxxxxxx> wrote: > > 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. > " Sounds good to me! Will send v5 shortly with added doc changes and the newly agreed upon commit log. Thanks Michal! > -- > Michal Hocko > SUSE Labs
