[PATCH] mm: don't show nr_indirectly_reclaimable in /proc/vmstat

[PATCH] mm: don't show nr_indirectly_reclaimable in /proc/vmstat

[Roman Gushchin]

Don't show nr_indirectly_reclaimable in /proc/vmstat,
because there is no need in exporting this vm counter
to the userspace, and some changes are expected
in reclaimable object accounting, which can alter
this counter.

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/vmstat.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/vmstat.c b/mm/vmstat.c
index 536332e988b8..a2b9518980ce 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1161,7 +1161,7 @@ const char * const vmstat_text[] = {
 	"nr_vmscan_immediate_reclaim",
 	"nr_dirtied",
 	"nr_written",
-	"nr_indirectly_reclaimable",
+	"", /* nr_indirectly_reclaimable */
 
 	/* enum writeback_stat_item counters */
 	"nr_dirty_threshold",
@@ -1740,6 +1740,10 @@ static int vmstat_show(struct seq_file *m, void *arg)
 	unsigned long *l = arg;
 	unsigned long off = l - (unsigned long *)m->private;
 
+	/* Skip hidden vmstat items. */
+	if (*vmstat_text[off] == '\0')
+		return 0;
+
 	seq_puts(m, vmstat_text[off]);
 	seq_put_decimal_ull(m, " ", *l);
 	seq_putc(m, '\n');
-- 
2.14.3

Re: [PATCH] mm: don't show nr_indirectly_reclaimable in /proc/vmstat

[Vlastimil Babka]

On 04/25/2018 09:14 PM, Roman Gushchin wrote:
> Don't show nr_indirectly_reclaimable in /proc/vmstat,
> because there is no need in exporting this vm counter
> to the userspace, and some changes are expected
> in reclaimable object accounting, which can alter
> this counter.

Oh, you beat me to it, thanks.

> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

Andrew, can you send this to Linus before the current rc period ends,
please?

Thanks,
Vlastimil

> ---
>  mm/vmstat.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 536332e988b8..a2b9518980ce 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1161,7 +1161,7 @@ const char * const vmstat_text[] = {
>  	"nr_vmscan_immediate_reclaim",
>  	"nr_dirtied",
>  	"nr_written",
> -	"nr_indirectly_reclaimable",
> +	"", /* nr_indirectly_reclaimable */
>  
>  	/* enum writeback_stat_item counters */
>  	"nr_dirty_threshold",
> @@ -1740,6 +1740,10 @@ static int vmstat_show(struct seq_file *m, void *arg)
>  	unsigned long *l = arg;
>  	unsigned long off = l - (unsigned long *)m->private;
>  
> +	/* Skip hidden vmstat items. */
> +	if (*vmstat_text[off] == '\0')
> +		return 0;
> +
>  	seq_puts(m, vmstat_text[off]);
>  	seq_put_decimal_ull(m, " ", *l);
>  	seq_putc(m, '\n');
> 

Re: [PATCH] mm: don't show nr_indirectly_reclaimable in /proc/vmstat

[David Rientjes]

On Wed, 25 Apr 2018, Roman Gushchin wrote:

> Don't show nr_indirectly_reclaimable in /proc/vmstat,
> because there is no need in exporting this vm counter
> to the userspace, and some changes are expected
> in reclaimable object accounting, which can alter
> this counter.
> 

I don't think it should be a per-node vmstat, in this case.  It appears 
only to be used for the global context.  Shouldn't this be handled like 
totalram_pages, total_swap_pages, totalreserve_pages, etc?

> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  mm/vmstat.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 536332e988b8..a2b9518980ce 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1161,7 +1161,7 @@ const char * const vmstat_text[] = {
>  	"nr_vmscan_immediate_reclaim",
>  	"nr_dirtied",
>  	"nr_written",
> -	"nr_indirectly_reclaimable",
> +	"", /* nr_indirectly_reclaimable */
>  
>  	/* enum writeback_stat_item counters */
>  	"nr_dirty_threshold",
> @@ -1740,6 +1740,10 @@ static int vmstat_show(struct seq_file *m, void *arg)
>  	unsigned long *l = arg;
>  	unsigned long off = l - (unsigned long *)m->private;
>  
> +	/* Skip hidden vmstat items. */
> +	if (*vmstat_text[off] == '\0')
> +		return 0;
> +
>  	seq_puts(m, vmstat_text[off]);
>  	seq_put_decimal_ull(m, " ", *l);
>  	seq_putc(m, '\n');

Re: [PATCH] mm: don't show nr_indirectly_reclaimable in /proc/vmstat

[Roman Gushchin]

On Wed, Apr 25, 2018 at 12:37:26PM -0700, David Rientjes wrote:
> On Wed, 25 Apr 2018, Roman Gushchin wrote:
> 
> > Don't show nr_indirectly_reclaimable in /proc/vmstat,
> > because there is no need in exporting this vm counter
> > to the userspace, and some changes are expected
> > in reclaimable object accounting, which can alter
> > this counter.
> > 
> 
> I don't think it should be a per-node vmstat, in this case.  It appears 
> only to be used for the global context.  Shouldn't this be handled like 
> totalram_pages, total_swap_pages, totalreserve_pages, etc?

Hi, David!

I don't see any reasons why re-using existing infrastructure for
fast vm counters is bad, and why should we re-invent it for this case.

Thanks!

Re: [PATCH] mm: don't show nr_indirectly_reclaimable in /proc/vmstat

[David Rientjes]

On Wed, 25 Apr 2018, Roman Gushchin wrote:

> > > Don't show nr_indirectly_reclaimable in /proc/vmstat,
> > > because there is no need in exporting this vm counter
> > > to the userspace, and some changes are expected
> > > in reclaimable object accounting, which can alter
> > > this counter.
> > > 
> > 
> > I don't think it should be a per-node vmstat, in this case.  It appears 
> > only to be used for the global context.  Shouldn't this be handled like 
> > totalram_pages, total_swap_pages, totalreserve_pages, etc?
> 
> Hi, David!
> 
> I don't see any reasons why re-using existing infrastructure for
> fast vm counters is bad, and why should we re-invent it for this case.
> 

Because now you have to modify the existing infrastructure for something 
that shouldn't be a vmstat in the first place?

Re: [PATCH] mm: don't show nr_indirectly_reclaimable in /proc/vmstat

[Michal Hocko]

On Wed 25-04-18 20:14:22, Roman Gushchin wrote:
> Don't show nr_indirectly_reclaimable in /proc/vmstat,
> because there is no need in exporting this vm counter
> to the userspace, and some changes are expected
> in reclaimable object accounting, which can alter
> this counter.
> 
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>

This is quite a hack. I would much rather revert the counter and fixed
it the way Vlastimil has proposed. But if there is a strong opposition
to the revert then this is probably the simples thing to do. Therefore

Unhappy-Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/vmstat.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 536332e988b8..a2b9518980ce 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1161,7 +1161,7 @@ const char * const vmstat_text[] = {
>  	"nr_vmscan_immediate_reclaim",
>  	"nr_dirtied",
>  	"nr_written",
> -	"nr_indirectly_reclaimable",
> +	"", /* nr_indirectly_reclaimable */
>  
>  	/* enum writeback_stat_item counters */
>  	"nr_dirty_threshold",
> @@ -1740,6 +1740,10 @@ static int vmstat_show(struct seq_file *m, void *arg)
>  	unsigned long *l = arg;
>  	unsigned long off = l - (unsigned long *)m->private;
>  
> +	/* Skip hidden vmstat items. */
> +	if (*vmstat_text[off] == '\0')
> +		return 0;
> +
>  	seq_puts(m, vmstat_text[off]);
>  	seq_put_decimal_ull(m, " ", *l);
>  	seq_putc(m, '\n');
> -- 
> 2.14.3

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: don't show nr_indirectly_reclaimable in /proc/vmstat

[David Rientjes]

On Thu, 26 Apr 2018, Michal Hocko wrote:

> > Don't show nr_indirectly_reclaimable in /proc/vmstat,
> > because there is no need in exporting this vm counter
> > to the userspace, and some changes are expected
> > in reclaimable object accounting, which can alter
> > this counter.
> > 
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> > Cc: Vlastimil Babka <vbabka@suse.cz>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> 
> This is quite a hack. I would much rather revert the counter and fixed
> it the way Vlastimil has proposed. But if there is a strong opposition
> to the revert then this is probably the simples thing to do. Therefore
> 

Implementing this counter as a vmstat doesn't make much sense based on how 
it's used.  Do you have a link to what Vlastimil proposed?  I haven't seen 
mention of alternative ideas.

Re: [PATCH] mm: don't show nr_indirectly_reclaimable in /proc/vmstat

[Vlastimil Babka]

On 04/26/2018 11:55 PM, David Rientjes wrote:
> On Thu, 26 Apr 2018, Michal Hocko wrote:
> 
>>> Don't show nr_indirectly_reclaimable in /proc/vmstat,
>>> because there is no need in exporting this vm counter
>>> to the userspace, and some changes are expected
>>> in reclaimable object accounting, which can alter
>>> this counter.
>>>
>>> Signed-off-by: Roman Gushchin <guro@fb.com>
>>> Cc: Vlastimil Babka <vbabka@suse.cz>
>>> Cc: Matthew Wilcox <willy@infradead.org>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>>
>> This is quite a hack. I would much rather revert the counter and fixed
>> it the way Vlastimil has proposed. But if there is a strong opposition
>> to the revert then this is probably the simples thing to do. Therefore
>>
> 
> Implementing this counter as a vmstat doesn't make much sense based on how 
> it's used.  Do you have a link to what Vlastimil proposed?  I haven't seen 
> mention of alternative ideas.

It was in the original thread, see e.g.
<08524819-14ef-81d0-fa90-d7af13c6b9d5@suse.cz>

However it will take some time to get that in mainline, and meanwhile
the current implementation does prevent a DOS. So I doubt it can be
fully reverted - as a compromise I just didn't want the counter to
become ABI. TBH though, other people at LSF/MM didn't seem concerned
that /proc/vmstat is an ABI that we can't change (i.e. counters have
been presumably removed in the past already).

Re: [PATCH] mm: don't show nr_indirectly_reclaimable in /proc/vmstat

[Roman Gushchin]

On Fri, Apr 27, 2018 at 11:17:01AM +0200, Vlastimil Babka wrote:
> On 04/26/2018 11:55 PM, David Rientjes wrote:
> > On Thu, 26 Apr 2018, Michal Hocko wrote:
> > 
> >>> Don't show nr_indirectly_reclaimable in /proc/vmstat,
> >>> because there is no need in exporting this vm counter
> >>> to the userspace, and some changes are expected
> >>> in reclaimable object accounting, which can alter
> >>> this counter.
> >>>
> >>> Signed-off-by: Roman Gushchin <guro@fb.com>
> >>> Cc: Vlastimil Babka <vbabka@suse.cz>
> >>> Cc: Matthew Wilcox <willy@infradead.org>
> >>> Cc: Andrew Morton <akpm@linux-foundation.org>
> >>> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> >>> Cc: Michal Hocko <mhocko@suse.com>
> >>> Cc: Johannes Weiner <hannes@cmpxchg.org>
> >>
> >> This is quite a hack. I would much rather revert the counter and fixed
> >> it the way Vlastimil has proposed. But if there is a strong opposition
> >> to the revert then this is probably the simples thing to do. Therefore
> >>
> > 
> > Implementing this counter as a vmstat doesn't make much sense based on how 
> > it's used.  Do you have a link to what Vlastimil proposed?  I haven't seen 
> > mention of alternative ideas.
> 
> It was in the original thread, see e.g.
> <08524819-14ef-81d0-fa90-d7af13c6b9d5@suse.cz>
> 
> However it will take some time to get that in mainline, and meanwhile
> the current implementation does prevent a DOS. So I doubt it can be
> fully reverted - as a compromise I just didn't want the counter to
> become ABI. TBH though, other people at LSF/MM didn't seem concerned
> that /proc/vmstat is an ABI that we can't change (i.e. counters have
> been presumably removed in the past already).
> 

Thank you, Vlastimil!
That pretty much matches my understanding of the case.

BTW, are you planning to work on supporting reclaimable objects
by slab allocators?

Thanks!

Re: [PATCH] mm: don't show nr_indirectly_reclaimable in /proc/vmstat

[Vlastimil Babka]

On 04/27/2018 12:55 PM, Roman Gushchin wrote:
> On Fri, Apr 27, 2018 at 11:17:01AM +0200, Vlastimil Babka wrote:
>> On 04/26/2018 11:55 PM, David Rientjes wrote:
>>>
>>> Implementing this counter as a vmstat doesn't make much sense based on how 
>>> it's used.  Do you have a link to what Vlastimil proposed?  I haven't seen 
>>> mention of alternative ideas.
>>
>> It was in the original thread, see e.g.
>> <08524819-14ef-81d0-fa90-d7af13c6b9d5@suse.cz>
>>
>> However it will take some time to get that in mainline, and meanwhile
>> the current implementation does prevent a DOS. So I doubt it can be
>> fully reverted - as a compromise I just didn't want the counter to
>> become ABI. TBH though, other people at LSF/MM didn't seem concerned
>> that /proc/vmstat is an ABI that we can't change (i.e. counters have
>> been presumably removed in the past already).
>>
> 
> Thank you, Vlastimil!
> That pretty much matches my understanding of the case.
> 
> BTW, are you planning to work on supporting reclaimable objects
> by slab allocators?

Yeah, soon!

Vlastimil

> Thanks!
> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] mm: don't show nr_indirectly_reclaimable in /proc/vmstat

[David Rientjes]

On Fri, 27 Apr 2018, Vlastimil Babka wrote:

> It was in the original thread, see e.g.
> <08524819-14ef-81d0-fa90-d7af13c6b9d5@suse.cz>
> 
> However it will take some time to get that in mainline, and meanwhile
> the current implementation does prevent a DOS. So I doubt it can be
> fully reverted - as a compromise I just didn't want the counter to
> become ABI. TBH though, other people at LSF/MM didn't seem concerned
> that /proc/vmstat is an ABI that we can't change (i.e. counters have
> been presumably removed in the past already).
> 

What prevents this from being a simple atomic_t that gets added to in 
__d_alloc(), subtracted from in __d_free_external_name(), and read in 
si_mem_available() and __vm_enough_memory()?

Re: [PATCH] mm: don't show nr_indirectly_reclaimable in /proc/vmstat

[Matthew Wilcox]

On Fri, Apr 27, 2018 at 11:41:31AM -0700, David Rientjes wrote:
> On Fri, 27 Apr 2018, Vlastimil Babka wrote:
> 
> > It was in the original thread, see e.g.
> > <08524819-14ef-81d0-fa90-d7af13c6b9d5@suse.cz>
> > 
> > However it will take some time to get that in mainline, and meanwhile
> > the current implementation does prevent a DOS. So I doubt it can be
> > fully reverted - as a compromise I just didn't want the counter to
> > become ABI. TBH though, other people at LSF/MM didn't seem concerned
> > that /proc/vmstat is an ABI that we can't change (i.e. counters have
> > been presumably removed in the past already).
> > 
> 
> What prevents this from being a simple atomic_t that gets added to in 
> __d_alloc(), subtracted from in __d_free_external_name(), and read in 
> si_mem_available() and __vm_enough_memory()?

I'd think you'd want one atomic_t per NUMA node at least ...

Re: [PATCH] mm: don't show nr_indirectly_reclaimable in /proc/vmstat

[Vlastimil Babka]

On 04/27/2018 08:41 PM, David Rientjes wrote:
> On Fri, 27 Apr 2018, Vlastimil Babka wrote:
> 
>> It was in the original thread, see e.g.
>> <08524819-14ef-81d0-fa90-d7af13c6b9d5@suse.cz>
>>
>> However it will take some time to get that in mainline, and meanwhile
>> the current implementation does prevent a DOS. So I doubt it can be
>> fully reverted - as a compromise I just didn't want the counter to
>> become ABI. TBH though, other people at LSF/MM didn't seem concerned
>> that /proc/vmstat is an ABI that we can't change (i.e. counters have
>> been presumably removed in the past already).
>>
> 
> What prevents this from being a simple atomic_t that gets added to in 
> __d_alloc(), subtracted from in __d_free_external_name(), and read in 
> si_mem_available() and __vm_enough_memory()?

The counter is already in mainline, so I think it's easier to simply
just stop printing it now than trying to replace its implementation with
one that can cause cache ping pongs.