Re: [PATCH 2/2 - v2] MM: Discard NR_UNSTABLE_NFS, use NR_WRITEBACK instead.

Michal Hocko
2020-04-06T07:41:24Z

On Mon 06-04-20 10:14:16, Neil Brown wrote:
> On Fri, Apr 03 2020, Michal Hocko wrote:
>
> > On Fri 03-04-20 11:42:20, Jan Kara wrote:
> > [...]
> >> > diff --git a/mm/vmstat.c b/mm/vmstat.c
> >> > index 78d53378db99..d1291537bbb9 100644
> >> > --- a/mm/vmstat.c
> >> > +++ b/mm/vmstat.c
> >> > @@ -1162,7 +1162,6 @@ const char * const vmstat_text[] = {
> >> > "nr_file_hugepages",
> >> > "nr_file_pmdmapped",
> >> > "nr_anon_transparent_hugepages",
> >> > - "nr_unstable",
> >> > "nr_vmscan_write",
> >> > "nr_vmscan_immediate_reclaim",
> >> > "nr_dirtied",
> >>
> >> This is probably the most tricky to deal with given how /proc/vmstat is
> >> formatted. OTOH for this file there's good chance we'd get away with just
> >> deleting nr_unstable line because there are entries added to it in the
> >> middle (e.g. in 60fbf0ab5da1 last September) and nobody complained yet.
> >>
> >> What do mm people think? How were changes to vmstat counters handled in the
> >> past?
> >
> > Adding new counters in the middle seems to be generally OK. I would be
> > more worried about removing counters though. So if we can simply print a
> > phone value at the very end then this should be a reasonable workaround.
>
> At the very end?
> Do you mean not have "nr_unstable 0" appear at all, but having "dummy 0"
> appear at the end just so that the number of lines doesn't decrease?
> Am I misunderstanding?

Sorry for not being clear. I meant semething like
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 78d53378db99..836e3f7a7aff 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1705,8 +1705,16 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
static void *vmstat_next(struct seq_file *m, void *arg, loff_t *pos)
{
(*pos)++;
- if (*pos >= NR_VMSTAT_ITEMS)
+ if (*pos >= NR_VMSTAT_ITEMS) {
+ /*
+ * deprecated counters which are no longer represented
+ * in vmstat arrays. We just lie about them to be always
+ * 0 to not break userspace which might expect them in
+ * int the output.
+ */
+ seq_puts(m, "nr_unstable 0")
return NULL;
+ }
return (unsigned long *)m->private + *pos;
}

--
Michal Hocko
SUSE Labs