lkml.org 
[lkml]   [2024]   [Feb]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 14/35] lib: introduce support for page allocation tagging
    On Fri, Feb 16, 2024 at 1:45 AM Vlastimil Babka <vbabka@suse.cz> wrote:
    >
    > On 2/12/24 22:39, Suren Baghdasaryan wrote:
    > > Introduce helper functions to easily instrument page allocators by
    > > storing a pointer to the allocation tag associated with the code that
    > > allocated the page in a page_ext field.
    > >
    > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
    > > Co-developed-by: Kent Overstreet <kent.overstreet@linux.dev>
    > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
    > > +
    > > +#ifdef CONFIG_MEM_ALLOC_PROFILING
    > > +
    > > +#include <linux/page_ext.h>
    > > +
    > > +extern struct page_ext_operations page_alloc_tagging_ops;
    > > +extern struct page_ext *page_ext_get(struct page *page);
    > > +extern void page_ext_put(struct page_ext *page_ext);
    > > +
    > > +static inline union codetag_ref *codetag_ref_from_page_ext(struct page_ext *page_ext)
    > > +{
    > > + return (void *)page_ext + page_alloc_tagging_ops.offset;
    > > +}
    > > +
    > > +static inline struct page_ext *page_ext_from_codetag_ref(union codetag_ref *ref)
    > > +{
    > > + return (void *)ref - page_alloc_tagging_ops.offset;
    > > +}
    > > +
    > > +static inline union codetag_ref *get_page_tag_ref(struct page *page)
    > > +{
    > > + if (page && mem_alloc_profiling_enabled()) {
    > > + struct page_ext *page_ext = page_ext_get(page);
    > > +
    > > + if (page_ext)
    > > + return codetag_ref_from_page_ext(page_ext);
    >
    > I think when structured like this, you're not getting the full benefits of
    > static keys, and the compiler probably can't improve that on its own.
    >
    > - page is tested before the static branch is evaluated
    > - when disabled, the result is NULL, and that's again tested in the callers

    Yes, that sounds right. I'll move the static branch check earlier like
    you suggested. Thanks!

    >
    > > + }
    > > + return NULL;
    > > +}
    > > +
    > > +static inline void put_page_tag_ref(union codetag_ref *ref)
    > > +{
    > > + page_ext_put(page_ext_from_codetag_ref(ref));
    > > +}
    > > +
    > > +static inline void pgalloc_tag_add(struct page *page, struct task_struct *task,
    > > + unsigned int order)
    > > +{
    > > + union codetag_ref *ref = get_page_tag_ref(page);
    >
    > So the more optimal way would be to test mem_alloc_profiling_enabled() here
    > as the very first thing before trying to get the ref.
    >
    > > + if (ref) {
    > > + alloc_tag_add(ref, task->alloc_tag, PAGE_SIZE << order);
    > > + put_page_tag_ref(ref);
    > > + }
    > > +}
    > > +
    > > +static inline void pgalloc_tag_sub(struct page *page, unsigned int order)
    > > +{
    > > + union codetag_ref *ref = get_page_tag_ref(page);
    >
    > And same here.
    >
    > > + if (ref) {
    > > + alloc_tag_sub(ref, PAGE_SIZE << order);
    > > + put_page_tag_ref(ref);
    > > + }
    > > +}
    > > +
    > > +#else /* CONFIG_MEM_ALLOC_PROFILING */
    > > +
    > > +static inline void pgalloc_tag_add(struct page *page, struct task_struct *task,
    > > + unsigned int order) {}
    > > +static inline void pgalloc_tag_sub(struct page *page, unsigned int order) {}
    > > +
    > > +#endif /* CONFIG_MEM_ALLOC_PROFILING */
    > > +
    > > +#endif /* _LINUX_PGALLOC_TAG_H */
    > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
    > > index 78d258ca508f..7bbdb0ddb011 100644
    > > --- a/lib/Kconfig.debug
    > > +++ b/lib/Kconfig.debug
    > > @@ -978,6 +978,7 @@ config MEM_ALLOC_PROFILING
    > > depends on PROC_FS
    > > depends on !DEBUG_FORCE_WEAK_PER_CPU
    > > select CODE_TAGGING
    > > + select PAGE_EXTENSION
    > > help
    > > Track allocation source code and record total allocation size
    > > initiated at that code location. The mechanism can be used to track
    > > diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
    > > index 4fc031f9cefd..2d5226d9262d 100644
    > > --- a/lib/alloc_tag.c
    > > +++ b/lib/alloc_tag.c
    > > @@ -3,6 +3,7 @@
    > > #include <linux/fs.h>
    > > #include <linux/gfp.h>
    > > #include <linux/module.h>
    > > +#include <linux/page_ext.h>
    > > #include <linux/proc_fs.h>
    > > #include <linux/seq_buf.h>
    > > #include <linux/seq_file.h>
    > > @@ -124,6 +125,22 @@ static bool alloc_tag_module_unload(struct codetag_type *cttype,
    > > return module_unused;
    > > }
    > >
    > > +static __init bool need_page_alloc_tagging(void)
    > > +{
    > > + return true;
    >
    > So this means the page_ext memory overead is paid unconditionally once
    > MEM_ALLOC_PROFILING is compile time enabled, even if never enabled during
    > runtime? That makes it rather costly to be suitable for generic distro
    > kernels where the code could be compile time enabled, and runtime enabling
    > suggested in a debugging/support scenario. It's what we do with page_owner,
    > debug_pagealloc, slub_debug etc.
    >
    > Ideally we'd have some vmalloc based page_ext flavor for later-than-boot
    > runtime enablement, as we now have for stackdepot. But that could be
    > explored later. For now it would be sufficient to add an early_param boot
    > parameter to control the enablement including page_ext, like page_owner and
    > other features do.

    Sounds reasonable. In v1 of this patchset we used early boot parameter
    but after LSF/MM discussion that was changed to runtime controls.
    Sounds like we would need both here. Should be easy to add.

    Allocating/reclaiming dynamically the space for page_ext, slab_ext,
    etc is not trivial and if done would be done separately. I looked into
    it before and listed the encountered issues in the cover letter of v2
    [1], see "things we could not address" section.

    [1] https://lore.kernel.org/all/20231024134637.3120277-1-surenb@google.com/

    >
    > > +}
    > > +
    > > +static __init void init_page_alloc_tagging(void)
    > > +{
    > > +}
    > > +
    > > +struct page_ext_operations page_alloc_tagging_ops = {
    > > + .size = sizeof(union codetag_ref),
    > > + .need = need_page_alloc_tagging,
    > > + .init = init_page_alloc_tagging,
    > > +};
    > > +EXPORT_SYMBOL(page_alloc_tagging_ops);
    >
    >
    > --
    > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
    >

    \
     
     \ /
      Last update: 2024-05-27 15:07    [W:4.439 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site