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 <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Johannes Weiner <[email protected]>
---
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
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 <[email protected]>
> Cc: Vlastimil Babka <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Alexander Viro <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> ---
> 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');
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 <[email protected]>
> Cc: Vlastimil Babka <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Alexander Viro <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Johannes Weiner <[email protected]>
Acked-by: Vlastimil Babka <[email protected]>
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');
>
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!
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?
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 <[email protected]>
> Cc: Vlastimil Babka <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Alexander Viro <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Johannes Weiner <[email protected]>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > Cc: Vlastimil Babka <[email protected]>
> > Cc: Matthew Wilcox <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Alexander Viro <[email protected]>
> > Cc: Michal Hocko <[email protected]>
> > Cc: Johannes Weiner <[email protected]>
>
> 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.
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 <[email protected]>
>>> Cc: Vlastimil Babka <[email protected]>
>>> Cc: Matthew Wilcox <[email protected]>
>>> Cc: Andrew Morton <[email protected]>
>>> Cc: Alexander Viro <[email protected]>
>>> Cc: Michal Hocko <[email protected]>
>>> Cc: Johannes Weiner <[email protected]>
>>
>> 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.
<[email protected]>
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).
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 <[email protected]>
> >>> Cc: Vlastimil Babka <[email protected]>
> >>> Cc: Matthew Wilcox <[email protected]>
> >>> Cc: Andrew Morton <[email protected]>
> >>> Cc: Alexander Viro <[email protected]>
> >>> Cc: Michal Hocko <[email protected]>
> >>> Cc: Johannes Weiner <[email protected]>
> >>
> >> 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.
> <[email protected]>
>
> 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!
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.
>> <[email protected]>
>>
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Fri, 27 Apr 2018, Vlastimil Babka wrote:
> It was in the original thread, see e.g.
> <[email protected]>
>
> 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()?
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.
> > <[email protected]>
> >
> > 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 ...
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.
>> <[email protected]>
>>
>> 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.