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
> >
