Re: [RFCv1 1/6] mm: Make get_vma_name() function public

[Pasha Tatashin <pasha.tatashin@soleen.com>]

On Mon, Nov 18, 2024 at 5:27 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Sat, Nov 16, 2024 at 05:59:17PM +0000, Pasha Tatashin wrote:
> > Page Detective will be using get_vma_name() that is currently used by
> > fs/proc to show names of VMAs in /proc/<pid>/smaps for example.
> >
> > Move this function to mm/vma.c, and make it accessible by modules.
>
> This is incorrect.
>
> mm/vma.c is for internal VMA implementation details, whose interface is
> explicitly mm/vma.h. This is so we can maintain the internal mechanism
> separate from interfaces and, importantly, are able to userland unit test
> VMA functionality.
>
> I think this _should_ be in mm/vma.c, but if it were to be exported it
> would need to be via a wrapper function in mm/mmap.c or somewhere like
> this.

Ok, I can do that in the next version.

>
> Also you broke the vma tests, go run make in tools/testing/vma/...

Hm interesting, I will take a look, this is surprising, as this patch
should not really change the behavior of anything. I guess it would be
because of the out of kernel vma.c build?

>
> Your patch also does not apply against Andrew's tree and the mm-unstable
> branch (i.e. against 6.13 in other words) which is what new mm patches
> should be based upon.
>
> Maybe I'll comment on the cover letter, but I don't agree you should be
> doing mm implementation details in a driver.
>
> The core of this should be in mm rather than exporting a bunch of stuff and
> have a driver do it. You're exposing internal implementation details
> unnecessarily.

This is not a problem, I will convert Page Detective to be in core mm.

> > @@ -3474,6 +3474,9 @@ void setattr_copy(struct mnt_idmap *, struct inode *inode,
> >
> >  extern int file_update_time(struct file *file);
> >
> > +void get_vma_name(struct vm_area_struct *vma, const struct path **path,
> > +               const char **name, const char **name_fmt);
> > +
>
> You're putting something in an mm/ C-file and the header in fs.h? Eh?

This is done so we do not have to include struct path into vma.h. fs.h
already has some vma functions like: vma_is_dax() and vma_is_fsdax().