| Message ID | 20241116175922.3265872-2-pasha.tatashin@soleen.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Page Detective | expand |
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. Also you broke the vma tests, go run make in tools/testing/vma/... 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. > > Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com> > --- > fs/proc/task_mmu.c | 61 ---------------------------------------------- > include/linux/fs.h | 3 +++ > mm/vma.c | 60 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 63 insertions(+), 61 deletions(-) > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > index e52bd96137a6..b28c42b7a591 100644 > --- a/fs/proc/task_mmu.c > +++ b/fs/proc/task_mmu.c > @@ -240,67 +240,6 @@ static int do_maps_open(struct inode *inode, struct file *file, > sizeof(struct proc_maps_private)); > } > > -static void get_vma_name(struct vm_area_struct *vma, > - const struct path **path, > - const char **name, > - const char **name_fmt) > -{ > - struct anon_vma_name *anon_name = vma->vm_mm ? anon_vma_name(vma) : NULL; > - > - *name = NULL; > - *path = NULL; > - *name_fmt = NULL; > - > - /* > - * Print the dentry name for named mappings, and a > - * special [heap] marker for the heap: > - */ > - if (vma->vm_file) { > - /* > - * If user named this anon shared memory via > - * prctl(PR_SET_VMA ..., use the provided name. > - */ > - if (anon_name) { > - *name_fmt = "[anon_shmem:%s]"; > - *name = anon_name->name; > - } else { > - *path = file_user_path(vma->vm_file); > - } > - return; > - } > - > - if (vma->vm_ops && vma->vm_ops->name) { > - *name = vma->vm_ops->name(vma); > - if (*name) > - return; > - } > - > - *name = arch_vma_name(vma); > - if (*name) > - return; > - > - if (!vma->vm_mm) { > - *name = "[vdso]"; > - return; > - } > - > - if (vma_is_initial_heap(vma)) { > - *name = "[heap]"; > - return; > - } > - > - if (vma_is_initial_stack(vma)) { > - *name = "[stack]"; > - return; > - } > - > - if (anon_name) { > - *name_fmt = "[anon:%s]"; > - *name = anon_name->name; > - return; > - } > -} > - > static void show_vma_header_prefix(struct seq_file *m, > unsigned long start, unsigned long end, > vm_flags_t flags, unsigned long long pgoff, > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 3559446279c1..a25b72397af5 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -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? > static inline bool vma_is_dax(const struct vm_area_struct *vma) > { > return vma->vm_file && IS_DAX(vma->vm_file->f_mapping->host); > diff --git a/mm/vma.c b/mm/vma.c > index 7621384d64cf..1bd589fbc3c7 100644 > --- a/mm/vma.c > +++ b/mm/vma.c > @@ -2069,3 +2069,63 @@ void mm_drop_all_locks(struct mm_struct *mm) > > mutex_unlock(&mm_all_locks_mutex); > } > + > +void get_vma_name(struct vm_area_struct *vma, const struct path **path, > + const char **name, const char **name_fmt) > +{ > + struct anon_vma_name *anon_name = vma->vm_mm ? anon_vma_name(vma) : NULL; If we were to export this (I'm very dubious about that) I'd want to assert some lock state and that the vma exists too. Because we're just assuming the VMA won't disappear from under us and now the driver will too, and also assuming we won't be passed NULL's... But in general I'm not in favour of having this exported. > + > + *name = NULL; > + *path = NULL; > + *name_fmt = NULL; > + > + /* > + * Print the dentry name for named mappings, and a > + * special [heap] marker for the heap: > + */ > + if (vma->vm_file) { > + /* > + * If user named this anon shared memory via > + * prctl(PR_SET_VMA ..., use the provided name. > + */ > + if (anon_name) { > + *name_fmt = "[anon_shmem:%s]"; > + *name = anon_name->name; > + } else { > + *path = file_user_path(vma->vm_file); > + } > + return; > + } > + > + if (vma->vm_ops && vma->vm_ops->name) { > + *name = vma->vm_ops->name(vma); > + if (*name) > + return; > + } > + > + *name = arch_vma_name(vma); > + if (*name) > + return; > + > + if (!vma->vm_mm) { > + *name = "[vdso]"; > + return; > + } > + > + if (vma_is_initial_heap(vma)) { > + *name = "[heap]"; > + return; > + } > + > + if (vma_is_initial_stack(vma)) { > + *name = "[stack]"; > + return; > + } > + > + if (anon_name) { > + *name_fmt = "[anon:%s]"; > + *name = anon_name->name; > + return; > + } > +} > +EXPORT_SYMBOL_GPL(get_vma_name); > -- > 2.47.0.338.g60cca15819-goog >
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().
On Mon, Nov 18, 2024 at 03:40:57PM -0500, Pasha Tatashin wrote: > > 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(). Yes, but DAX is a monumental layering violation, not something to be emulated.
On Mon, Nov 18, 2024 at 3:44 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Mon, Nov 18, 2024 at 03:40:57PM -0500, Pasha Tatashin wrote: > > > 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(). > > Yes, but DAX is a monumental layering violation, not something to be > emulated. Fair enough, but I do not like adding "struct path" dependency to vma.h, is there a better place to put it? Pasha
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index e52bd96137a6..b28c42b7a591 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -240,67 +240,6 @@ static int do_maps_open(struct inode *inode, struct file *file, sizeof(struct proc_maps_private)); } -static void get_vma_name(struct vm_area_struct *vma, - const struct path **path, - const char **name, - const char **name_fmt) -{ - struct anon_vma_name *anon_name = vma->vm_mm ? anon_vma_name(vma) : NULL; - - *name = NULL; - *path = NULL; - *name_fmt = NULL; - - /* - * Print the dentry name for named mappings, and a - * special [heap] marker for the heap: - */ - if (vma->vm_file) { - /* - * If user named this anon shared memory via - * prctl(PR_SET_VMA ..., use the provided name. - */ - if (anon_name) { - *name_fmt = "[anon_shmem:%s]"; - *name = anon_name->name; - } else { - *path = file_user_path(vma->vm_file); - } - return; - } - - if (vma->vm_ops && vma->vm_ops->name) { - *name = vma->vm_ops->name(vma); - if (*name) - return; - } - - *name = arch_vma_name(vma); - if (*name) - return; - - if (!vma->vm_mm) { - *name = "[vdso]"; - return; - } - - if (vma_is_initial_heap(vma)) { - *name = "[heap]"; - return; - } - - if (vma_is_initial_stack(vma)) { - *name = "[stack]"; - return; - } - - if (anon_name) { - *name_fmt = "[anon:%s]"; - *name = anon_name->name; - return; - } -} - static void show_vma_header_prefix(struct seq_file *m, unsigned long start, unsigned long end, vm_flags_t flags, unsigned long long pgoff, diff --git a/include/linux/fs.h b/include/linux/fs.h index 3559446279c1..a25b72397af5 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -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); + static inline bool vma_is_dax(const struct vm_area_struct *vma) { return vma->vm_file && IS_DAX(vma->vm_file->f_mapping->host); diff --git a/mm/vma.c b/mm/vma.c index 7621384d64cf..1bd589fbc3c7 100644 --- a/mm/vma.c +++ b/mm/vma.c @@ -2069,3 +2069,63 @@ void mm_drop_all_locks(struct mm_struct *mm) mutex_unlock(&mm_all_locks_mutex); } + +void get_vma_name(struct vm_area_struct *vma, const struct path **path, + const char **name, const char **name_fmt) +{ + struct anon_vma_name *anon_name = vma->vm_mm ? anon_vma_name(vma) : NULL; + + *name = NULL; + *path = NULL; + *name_fmt = NULL; + + /* + * Print the dentry name for named mappings, and a + * special [heap] marker for the heap: + */ + if (vma->vm_file) { + /* + * If user named this anon shared memory via + * prctl(PR_SET_VMA ..., use the provided name. + */ + if (anon_name) { + *name_fmt = "[anon_shmem:%s]"; + *name = anon_name->name; + } else { + *path = file_user_path(vma->vm_file); + } + return; + } + + if (vma->vm_ops && vma->vm_ops->name) { + *name = vma->vm_ops->name(vma); + if (*name) + return; + } + + *name = arch_vma_name(vma); + if (*name) + return; + + if (!vma->vm_mm) { + *name = "[vdso]"; + return; + } + + if (vma_is_initial_heap(vma)) { + *name = "[heap]"; + return; + } + + if (vma_is_initial_stack(vma)) { + *name = "[stack]"; + return; + } + + if (anon_name) { + *name_fmt = "[anon:%s]"; + *name = anon_name->name; + return; + } +} +EXPORT_SYMBOL_GPL(get_vma_name);
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. Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com> --- fs/proc/task_mmu.c | 61 ---------------------------------------------- include/linux/fs.h | 3 +++ mm/vma.c | 60 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 61 deletions(-)