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

[Suren Baghdasaryan <surenb@google.com>]

On Mon, Jan 13, 2025 at 8:35 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [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@google.com>
> > > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> > > ---
> > >  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!

>
> Thanks,
> Liam
>