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

[Liam R. Howlett]

* 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,
Liam