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!
>
> Thanks,
> Liam
>
