| Message ID | 20240929093212.40449-1-qiwu.chen@transsion.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: move get_vma_name() from procfs to mm tree | expand |
On Sun, 29 Sep 2024 17:32:12 +0800 "qiwu.chen" <qiwuchen55@gmail.com> wrote: > Commit acd4b2ecf3bb2 ("fs/procfs: extract logic for getting VMA name > constituents") introduces a generic helper to get VMA name constituents. > Currently, it's statically defined and referenced in fs/proc/task_mmu.c, > which is not opened to users who want to query VMA name more efficiently. > > This patch moves get_vma_name() from procfs to mm/mmap.c, and export it > for modules usage. We will want to see (and review) the code which calls this function and that code should be part of the mainline kernel tree.
I'm sorry to be difficult, but for me this is a no :(. I explain below. On Sun, Sep 29, 2024 at 05:32:12PM GMT, qiwu.chen wrote: > Commit acd4b2ecf3bb2 ("fs/procfs: extract logic for getting VMA name > constituents") introduces a generic helper to get VMA name constituents. > Currently, it's statically defined and referenced in fs/proc/task_mmu.c, > which is not opened to users who want to query VMA name more efficiently. I don't know what you mean by 'efficiently'? What is inefficient? This seems more like a convenience function for debugging? > > This patch moves get_vma_name() from procfs to mm/mmap.c, and export it > for modules usage. > > There should be no functional changes. I'd call exporting an arbitrary VMA operation a functional change :) but guess it's up for debate. I'm not in favour of this patch as: The VMA needs to be locked correctly for this to work and we plan to adjust how that locking works over time, in fact relatively soon. Exporting this as a module-available function means we are more constrained in how we can proceed with such changes, even if officially we do not guarantee internal kernel API/ABI stability. Also, an 'exportable' version of this function would be one that asserted the locking for you and did various more checks to make sure the input/output parameters weren't trash, but again I don't think the maintenance burden of making that available is really worth it. Ideally I'd love VMAs to be an entirely internal implementation detail. Sadly things like fault handlers for file systems prevent that. But it very much minds me against exporting things any more than that. So yeah, sorry to be difficult on something seemingly rather trivial, but it's a no. > > Signed-off-by: qiwu.chen <qiwu.chen@transsion.com> An aside, but I don't feel hugely comfortable with an email being sent from one account and signed off by another... I'm not sure on our policy on that though. > --- > fs/proc/task_mmu.c | 61 -------------------------------------- > include/linux/mm.h | 2 ++ > mm/mmap.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 76 insertions(+), 61 deletions(-) > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > index 72f14fd59c2d..7d414c39367a 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/mm.h b/include/linux/mm.h > index ecf63d2b0582..31abb857e11e 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -3418,6 +3418,8 @@ int expand_downwards(struct vm_area_struct *vma, unsigned long address); > extern struct vm_area_struct * find_vma(struct mm_struct * mm, unsigned long addr); > extern struct vm_area_struct * find_vma_prev(struct mm_struct * mm, unsigned long addr, > struct vm_area_struct **pprev); > +extern void get_vma_name(struct vm_area_struct *vma, const struct path **path, > + const char **name, const char **name_fmt); A nit (especially as I'm not in favour of this change) and you have no reason why you'd be aware of this so it's fine but - generally when we add functions here we don't include the extern keyword as it's unnecessary. > > /* > * Look up the first VMA which intersects the interval [start_addr, end_addr) > diff --git a/mm/mmap.c b/mm/mmap.c > index ee8f91eaadb9..d6ca383fc302 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -995,6 +995,80 @@ find_vma_prev(struct mm_struct *mm, unsigned long addr, > return vma; > } > > +/** > + * get_vma_name() - get VMA name with relevant pieces of data. > + * @vma: The vma to check > + * @path: The file path given to the vma > + * @name: The vma name > + * @name_fmt: The formatted vma name > + * > + * Extract generic logic to fetch relevant pieces of data to describe > + * VMA name. This could be just some string (either special constant or > + * user-provided), or a string with some formatted wrapping text (e.g., > + * "[anon_shmem:<something>]"), or, commonly, file path. > + */ > +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(get_vma_name); It's a convenience thing, so I'd have preferred an EXPORT_SYMBOL_GPL() anyway. I have no desire to provide this kind of thing to binary blobs. > + > /* > * Verify that the stack growth is acceptable and > * update accounting. This is shared with both the > -- > 2.25.1 > >
On Mon, Sep 30, 2024 at 11:05:26AM -0700, Andrew Morton wrote: > > We will want to see (and review) the code which calls this function and > that code should be part of the mainline kernel tree. > The similar code can see perf_event_mmap_event() in kernel/events/core.c, and current talking topic: https://patchwork.kernel.org/project/linux-mm/patch/20240924074341.37272-1-qiwu.chen@transsion.com/ Thanks Qiwu
On Tue, Oct 01, 2024 at 06:55:25PM GMT, chenqiwu wrote: > On Mon, Sep 30, 2024 at 11:05:26AM -0700, Andrew Morton wrote: > > > > We will want to see (and review) the code which calls this function and > > that code should be part of the mainline kernel tree. > > > The similar code can see perf_event_mmap_event() in kernel/events/core.c, > and current talking topic: > https://patchwork.kernel.org/project/linux-mm/patch/20240924074341.37272-1-qiwu.chen@transsion.com/ This doesn't explain why you're exporting things... I am very much not in favour of that, sorry. All the same objections apply as I said there. We already have dump_mm() for dumping this kind of thing, and yes that requires CONFIG_DEBUG_VM and yes I think that's valid. On panic it'd probably not be a great idea to try to take locks and walk this data which might be corrupted or otherwise unavailable, seems very risky to me. I don't really understand why you think this will help an unhandled page fault in init? But yeah I'm not in favour of this patch, still. Also please, please do not put VMA/mm-related code that iterate through VAMs etc. in random other files, they _all_ belong in mm files. I'll reply there I guess. > > Thanks > Qiwu >
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index 72f14fd59c2d..7d414c39367a 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/mm.h b/include/linux/mm.h index ecf63d2b0582..31abb857e11e 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3418,6 +3418,8 @@ int expand_downwards(struct vm_area_struct *vma, unsigned long address); extern struct vm_area_struct * find_vma(struct mm_struct * mm, unsigned long addr); extern struct vm_area_struct * find_vma_prev(struct mm_struct * mm, unsigned long addr, struct vm_area_struct **pprev); +extern void get_vma_name(struct vm_area_struct *vma, const struct path **path, + const char **name, const char **name_fmt); /* * Look up the first VMA which intersects the interval [start_addr, end_addr) diff --git a/mm/mmap.c b/mm/mmap.c index ee8f91eaadb9..d6ca383fc302 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -995,6 +995,80 @@ find_vma_prev(struct mm_struct *mm, unsigned long addr, return vma; } +/** + * get_vma_name() - get VMA name with relevant pieces of data. + * @vma: The vma to check + * @path: The file path given to the vma + * @name: The vma name + * @name_fmt: The formatted vma name + * + * Extract generic logic to fetch relevant pieces of data to describe + * VMA name. This could be just some string (either special constant or + * user-provided), or a string with some formatted wrapping text (e.g., + * "[anon_shmem:<something>]"), or, commonly, file path. + */ +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(get_vma_name); + /* * Verify that the stack growth is acceptable and * update accounting. This is shared with both the
Commit acd4b2ecf3bb2 ("fs/procfs: extract logic for getting VMA name constituents") introduces a generic helper to get VMA name constituents. Currently, it's statically defined and referenced in fs/proc/task_mmu.c, which is not opened to users who want to query VMA name more efficiently. This patch moves get_vma_name() from procfs to mm/mmap.c, and export it for modules usage. There should be no functional changes. Signed-off-by: qiwu.chen <qiwu.chen@transsion.com> --- fs/proc/task_mmu.c | 61 -------------------------------------- include/linux/mm.h | 2 ++ mm/mmap.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 61 deletions(-)