Re: [PATCH v9 13/17] mm/debug: print vm_refcnt state when dumping the vma

[Lorenzo Stoakes]

On Mon, Jan 13, 2025 at 09:57:54AM -0800, Suren Baghdasaryan wrote:
> On Mon, Jan 13, 2025 at 8:35 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
> >
> > * Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> [250113 11:21]:
> > > On Fri, Jan 10, 2025 at 08:26:00PM -0800, Suren Baghdasaryan wrote:
> > > > vm_refcnt encodes a number of useful states:
> > > > - whether vma is attached or detached
> > > > - the number of current vma readers
> > > > - presence of a vma writer
> > > > Let's include it in the vma dump.
> > > >
> > > > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > > > Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
> > > > ---
> > > >  mm/debug.c | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > >
> > > > diff --git a/mm/debug.c b/mm/debug.c
> > > > index 8d2acf432385..325d7bf22038 100644
> > > > --- a/mm/debug.c
> > > > +++ b/mm/debug.c
> > > > @@ -178,6 +178,17 @@ EXPORT_SYMBOL(dump_page);
> > > >
> > > >  void dump_vma(const struct vm_area_struct *vma)
> > > >  {
> > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > +   pr_emerg("vma %px start %px end %px mm %px\n"
> > > > +           "prot %lx anon_vma %px vm_ops %px\n"
> > > > +           "pgoff %lx file %px private_data %px\n"
> > > > +           "flags: %#lx(%pGv) refcnt %x\n",
> > > > +           vma, (void *)vma->vm_start, (void *)vma->vm_end, vma->vm_mm,
> > > > +           (unsigned long)pgprot_val(vma->vm_page_prot),
> > > > +           vma->anon_vma, vma->vm_ops, vma->vm_pgoff,
> > > > +           vma->vm_file, vma->vm_private_data,
> > > > +           vma->vm_flags, &vma->vm_flags, refcount_read(&vma->vm_refcnt));
> > > > +#else
> > > >     pr_emerg("vma %px start %px end %px mm %px\n"
> > > >             "prot %lx anon_vma %px vm_ops %px\n"
> > > >             "pgoff %lx file %px private_data %px\n"
> > > > @@ -187,6 +198,7 @@ void dump_vma(const struct vm_area_struct *vma)
> > > >             vma->anon_vma, vma->vm_ops, vma->vm_pgoff,
> > > >             vma->vm_file, vma->vm_private_data,
> > > >             vma->vm_flags, &vma->vm_flags);
> > > > +#endif
> > > >  }
> > >
> > > This is pretty horribly duplicative and not in line with how this kind of
> > > thing is done in the rest of the file. You're just adding one entry, so why
> > > not:
> > >
> > > void dump_vma(const struct vm_area_struct *vma)
> > > {
> > >       pr_emerg("vma %px start %px end %px mm %px\n"
> > >               "prot %lx anon_vma %px vm_ops %px\n"
> > >               "pgoff %lx file %px private_data %px\n"
> > > #ifdef CONFIG_PER_VMA_LOCK
> > >               "refcnt %x\n"
> > > #endif
> > >               "flags: %#lx(%pGv)\n",
> > >               vma, (void *)vma->vm_start, (void *)vma->vm_end, vma->vm_mm,
> > >               (unsigned long)pgprot_val(vma->vm_page_prot),
> > >               vma->anon_vma, vma->vm_ops, vma->vm_pgoff,
> > >               vma->vm_file, vma->vm_private_data,
> > >               vma->vm_flags,
> > > #ifdef CONFIG_PER_VMA_LOCK
> > >               refcount_read(&vma->vm_refcnt),
> > > #endif
> > >               &vma->vm_flags);
> > > }
> >
> > right, I had an issue with this as well.
> >
> > Another option would be:
> >
> >         pr_emerg("vma %px start %px end %px mm %px\n"
> >                 "prot %lx anon_vma %px vm_ops %px\n"
> >                 "pgoff %lx file %px private_data %px\n",
> >                 <big mess here>);
> >         dump_vma_refcnt();
> >         pr_emerg("flags:...", vma_vm_flags);
> >
> >
> > Then dump_vma_refcnt() either dumps the refcnt or does nothing,
> > depending on the config option.
> >
> > Either way is good with me.  Lorenzo's suggestion is in line with the
> > file and it's clear as to why the refcnt might be missing, but I don't
> > really see this being an issue in practice.
>
> Thanks for clarifying! Lorenzo's suggestion LGTM too. I'll adopt it. Thanks!
>

Cheers guys!

> >
> > Thanks,
> > Liam
> >