On 4/13/2018 5:43 PM, vinayak menon wrote: > On Thu, Apr 12, 2018 at 8:27 PM, Roman Gushchin <guro@xxxxxx> wrote: >> On Thu, Apr 12, 2018 at 08:52:52AM +0200, Vlastimil Babka wrote: >>> On 04/11/2018 03:56 PM, Roman Gushchin wrote: >>>> On Wed, Apr 11, 2018 at 03:16:08PM +0200, Vlastimil Babka wrote: >>>>> [+CC linux-api] >>>>> >>>>> On 03/05/2018 02:37 PM, Roman Gushchin wrote: >>>>>> This patch introduces a concept of indirectly reclaimable memory >>>>>> and adds the corresponding memory counter and /proc/vmstat item. >>>>>> >>>>>> Indirectly reclaimable memory is any sort of memory, used by >>>>>> the kernel (except of reclaimable slabs), which is actually >>>>>> reclaimable, i.e. will be released under memory pressure. >>>>>> >>>>>> The counter is in bytes, as it's not always possible to >>>>>> count such objects in pages. The name contains BYTES >>>>>> by analogy to NR_KERNEL_STACK_KB. >>>>>> >>>>>> Signed-off-by: Roman Gushchin <guro@xxxxxx> >>>>>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> >>>>>> Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx> >>>>>> Cc: Michal Hocko <mhocko@xxxxxxxx> >>>>>> Cc: Johannes Weiner <hannes@xxxxxxxxxxx> >>>>>> Cc: linux-fsdevel@xxxxxxxxxxxxxxx >>>>>> Cc: linux-kernel@xxxxxxxxxxxxxxx >>>>>> Cc: linux-mm@xxxxxxxxx >>>>>> Cc: kernel-team@xxxxxx >>>>> >>>>> Hmm, looks like I'm late and this user-visible API change was just >>>>> merged. But it's for rc1, so we can still change it, hopefully? >>>>> >>>>> One problem I see with the counter is that it's in bytes, but among >>>>> counters that use pages, and the name doesn't indicate it. >>>> >>>> Here I just followed "nr_kernel_stack" path, which is measured in kB, >>>> but this is not mentioned in the field name. >>> >>> Oh, didn't know. Bad example to follow :P >>> >>>>> Then, I don't >>>>> see why users should care about the "indirectly" part, as that's just an >>>>> implementation detail. It is reclaimable and that's what matters, right? >>>>> (I also wanted to complain about lack of Documentation/... update, but >>>>> looks like there's no general file about vmstat, ugh) >>>> >>>> I agree, that it's a bit weird, and it's probably better to not expose >>>> it at all; but this is how all vm counters work. We do expose them all >>>> in /proc/vmstat. A good number of them is useless until you are not a >>>> mm developer, so it's arguable more "debug info" rather than "api". >>> >>> Yeah the problem is that once tools start rely on them, they fall under >>> the "do not break userspace" rule, however we call them. So being >>> cautious and conservative can't hurt. >>> >>>> It's definitely not a reason to make them messy. >>>> Does "nr_indirectly_reclaimable_bytes" look better to you? >>> >>> It still has has the "indirecly" part and feels arbitrary :/ >>> >>>>> >>>>> I also kind of liked the idea from v1 rfc posting that there would be a >>>>> separate set of reclaimable kmalloc-X caches for these kind of >>>>> allocations. Besides accounting, it should also help reduce memory >>>>> fragmentation. The right variant of cache would be detected via >>>>> __GFP_RECLAIMABLE. >>>> >>>> Well, the downside is that we have to introduce X new caches >>>> just for this particular problem. I'm not strictly against the idea, >>>> but not convinced that it's much better. >>> >>> Maybe we can find more cases that would benefit from it. Heck, even slab >>> itself allocates some management structures from the generic kmalloc >>> caches, and if they are used for reclaimable caches, they could be >>> tracked as reclaimable as well. >> >> This is a good catch! >> >>> >>>>> >>>>> With that in mind, can we at least for now put the (manually maintained) >>>>> byte counter in a variable that's not directly exposed via /proc/vmstat, >>>>> and then when printing nr_slab_reclaimable, simply add the value >>>>> (divided by PAGE_SIZE), and when printing nr_slab_unreclaimable, >>>>> subtract the same value. This way we would be simply making the existing >>>>> counters more precise, in line with their semantics. >>>> >>>> Idk, I don't like the idea of adding a counter outside of the vm counters >>>> infrastructure, and I definitely wouldn't touch the exposed >>>> nr_slab_reclaimable and nr_slab_unreclaimable fields. >>> >>> We would be just making the reported values more precise wrt reality. >> >> It depends on if we believe that only slab memory can be reclaimable >> or not. If yes, this is true, otherwise not. >> >> My guess is that some drivers (e.g. networking) might have buffers, >> which are reclaimable under mempressure, and are allocated using >> the page allocator. But I have to look closer... >> > > One such case I have encountered is that of the ION page pool. The page pool > registers a shrinker. When not in any memory pressure page pool can go high > and thus cause an mmap to fail when OVERCOMMIT_GUESS is set. I can send > a patch to account ION page pool pages in NR_INDIRECTLY_RECLAIMABLE_BYTES. > > Thanks, > Vinayak > As Vinayak mentioned NR_INDIRECTLY_RECLAIMABLE_BYTES can be used to solve the issue with ION page pool when OVERCOMMIT_GUESS is set, the patch for the same can be found here https://lkml.org/lkml/2018/4/24/1288
