On Mon, Nov 18, 2024 at 5:27 AM Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> 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().
