| Message ID | 20240214170157.17530-3-osalvador@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | page_owner: print stacks and their outstanding allocations | expand |
On Wed, 14 Feb 2024 at 18:00, Oscar Salvador <osalvador@suse.de> wrote: > > In order to move the heavy lifting into page_owner code, this one > needs to have access to the stack_record structure, which right now > sits in lib/stackdepot.c. > Move it to the stackdepot.h header so page_owner can access > stack_record's struct fields. > > Signed-off-by: Oscar Salvador <osalvador@suse.de> > Reviewed-by: Marco Elver <elver@google.com> > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > --- > include/linux/stackdepot.h | 47 ++++++++++++++++++++++++++++++++++++++ > lib/stackdepot.c | 45 +----------------------------------- > 2 files changed, 48 insertions(+), 44 deletions(-) > > diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h > index adcbb8f23600..c4b5ad57c066 100644 > --- a/include/linux/stackdepot.h > +++ b/include/linux/stackdepot.h > @@ -30,6 +30,53 @@ typedef u32 depot_stack_handle_t; > */ > #define STACK_DEPOT_EXTRA_BITS 5 > > +#define DEPOT_HANDLE_BITS (sizeof(depot_stack_handle_t) * 8) > + > +#define DEPOT_POOL_ORDER 2 /* Pool size order, 4 pages */ > +#define DEPOT_POOL_SIZE (1LL << (PAGE_SHIFT + DEPOT_POOL_ORDER)) > +#define DEPOT_STACK_ALIGN 4 > +#define DEPOT_OFFSET_BITS (DEPOT_POOL_ORDER + PAGE_SHIFT - DEPOT_STACK_ALIGN) > +#define DEPOT_POOL_INDEX_BITS (DEPOT_HANDLE_BITS - DEPOT_OFFSET_BITS - \ > + STACK_DEPOT_EXTRA_BITS) > + > +#ifdef CONFIG_STACKDEPOT > +/* Compact structure that stores a reference to a stack. */ > +union handle_parts { > + depot_stack_handle_t handle; > + struct { > + /* pool_index is offset by 1 */ > + u32 pool_index : DEPOT_POOL_INDEX_BITS; > + u32 offset : DEPOT_OFFSET_BITS; > + u32 extra : STACK_DEPOT_EXTRA_BITS; > + }; > +}; > + > +struct stack_record { > + struct list_head hash_list; /* Links in the hash table */ > + u32 hash; /* Hash in hash table */ > + u32 size; /* Number of stored frames */ > + union handle_parts handle; /* Constant after initialization */ > + refcount_t count; > + union { > + unsigned long entries[CONFIG_STACKDEPOT_MAX_FRAMES]; /* Frames */ > + struct { > + /* > + * An important invariant of the implementation is to > + * only place a stack record onto the freelist iff its > + * refcount is zero. Because stack records with a zero > + * refcount are never considered as valid, it is safe to > + * union @entries and freelist management state below. > + * Conversely, as soon as an entry is off the freelist > + * and its refcount becomes non-zero, the below must not > + * be accessed until being placed back on the freelist. > + */ > + struct list_head free_list; /* Links in the freelist */ > + unsigned long rcu_state; /* RCU cookie */ > + }; > + }; > +}; > +#endif > + > typedef u32 depot_flags_t; > > /* > diff --git a/lib/stackdepot.c b/lib/stackdepot.c > index c043a4186bc5..4a661a6777da 100644 > --- a/lib/stackdepot.c > +++ b/lib/stackdepot.c > @@ -36,55 +36,12 @@ > #include <linux/memblock.h> > #include <linux/kasan-enabled.h> > > -#define DEPOT_HANDLE_BITS (sizeof(depot_stack_handle_t) * 8) > - > -#define DEPOT_POOL_ORDER 2 /* Pool size order, 4 pages */ > -#define DEPOT_POOL_SIZE (1LL << (PAGE_SHIFT + DEPOT_POOL_ORDER)) > -#define DEPOT_STACK_ALIGN 4 > -#define DEPOT_OFFSET_BITS (DEPOT_POOL_ORDER + PAGE_SHIFT - DEPOT_STACK_ALIGN) > -#define DEPOT_POOL_INDEX_BITS (DEPOT_HANDLE_BITS - DEPOT_OFFSET_BITS - \ > - STACK_DEPOT_EXTRA_BITS) > #define DEPOT_POOLS_CAP 8192 > -/* The pool_index is offset by 1 so the first record does not have a 0 handle. */ > +/* The pool_index is offset by 1 so the first record does not have a 0 handle */ Why this comment change? We lost the '.' -- for future reference, it'd be good to ensure unnecessary changes don't creep into the diff. This is just nitpicking, and I've already reviewed this change, so no need to send a v+1.
On Thu, Feb 15, 2024 at 09:16:58AM +0100, Marco Elver wrote: > On Wed, 14 Feb 2024 at 18:00, Oscar Salvador <osalvador@suse.de> wrote: > > -/* The pool_index is offset by 1 so the first record does not have a 0 handle. */ > > +/* The pool_index is offset by 1 so the first record does not have a 0 handle */ > > Why this comment change? We lost the '.' -- for future reference, it'd > be good to ensure unnecessary changes don't creep into the diff. This > is just nitpicking, and I've already reviewed this change, so no need > to send a v+1. Right, this was an oversight. Andrew, please fold the following into the patch, thanks: diff --git a/lib/stackdepot.c b/lib/stackdepot.c index 4a661a6777da..514b8d40ff57 100644 --- a/lib/stackdepot.c +++ b/lib/stackdepot.c @@ -37,7 +37,7 @@ #include <linux/kasan-enabled.h> #define DEPOT_POOLS_CAP 8192 -/* The pool_index is offset by 1 so the first record does not have a 0 handle */ +/* The pool_index is offset by 1 so the first record does not have a 0 handle. */ #define DEPOT_MAX_POOLS \ (((1LL << (DEPOT_POOL_INDEX_BITS)) - 1 < DEPOT_POOLS_CAP) ? \ (1LL << (DEPOT_POOL_INDEX_BITS)) - 1 : DEPOT_POOLS_CAP)
On 2/15/24 09:16, Marco Elver wrote: > On Wed, 14 Feb 2024 at 18:00, Oscar Salvador <osalvador@suse.de> wrote: >> >> In order to move the heavy lifting into page_owner code, this one >> needs to have access to the stack_record structure, which right now >> sits in lib/stackdepot.c. >> Move it to the stackdepot.h header so page_owner can access >> stack_record's struct fields. >> >> Signed-off-by: Oscar Salvador <osalvador@suse.de> >> Reviewed-by: Marco Elver <elver@google.com> >> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> >> --- >> #define DEPOT_POOLS_CAP 8192 >> -/* The pool_index is offset by 1 so the first record does not have a 0 handle. */ >> +/* The pool_index is offset by 1 so the first record does not have a 0 handle */ > > Why this comment change? We lost the '.' -- for future reference, it'd > be good to ensure unnecessary changes don't creep into the diff. This > is just nitpicking, Agree with this part. > and I've already reviewed this change, so no need > to send a v+1. But confused by this remark. There is a number of nontrivial changes in the series from v8, and IIRC v8 was dropped from mm/ meanwhile, so a v+1 of the whole series is expected and not fixups. Which means including patches that were already reviewed. That's the usual process.
On Thu, 15 Feb 2024 at 10:30, Vlastimil Babka <vbabka@suse.cz> wrote: > > On 2/15/24 09:16, Marco Elver wrote: > > On Wed, 14 Feb 2024 at 18:00, Oscar Salvador <osalvador@suse.de> wrote: > >> > >> In order to move the heavy lifting into page_owner code, this one > >> needs to have access to the stack_record structure, which right now > >> sits in lib/stackdepot.c. > >> Move it to the stackdepot.h header so page_owner can access > >> stack_record's struct fields. > >> > >> Signed-off-by: Oscar Salvador <osalvador@suse.de> > >> Reviewed-by: Marco Elver <elver@google.com> > >> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > >> --- > > >> #define DEPOT_POOLS_CAP 8192 > >> -/* The pool_index is offset by 1 so the first record does not have a 0 handle. */ > >> +/* The pool_index is offset by 1 so the first record does not have a 0 handle */ > > > > Why this comment change? We lost the '.' -- for future reference, it'd > > be good to ensure unnecessary changes don't creep into the diff. This > > is just nitpicking, > > Agree with this part. > > > and I've already reviewed this change, so no need > > to send a v+1. > > But confused by this remark. There is a number of nontrivial changes in the > series from v8, and IIRC v8 was dropped from mm/ meanwhile, so a v+1 of the > whole series is expected and not fixups. Which means including patches that > were already reviewed. That's the usual process. This is already v9. Of course, still need to look at rest of v9 and if there are major changes needed then a v10 is needed.
On 2/15/24 10:33, Marco Elver wrote: > On Thu, 15 Feb 2024 at 10:30, Vlastimil Babka <vbabka@suse.cz> wrote: >> >> On 2/15/24 09:16, Marco Elver wrote: >> > On Wed, 14 Feb 2024 at 18:00, Oscar Salvador <osalvador@suse.de> wrote: >> >> >> >> In order to move the heavy lifting into page_owner code, this one >> >> needs to have access to the stack_record structure, which right now >> >> sits in lib/stackdepot.c. >> >> Move it to the stackdepot.h header so page_owner can access >> >> stack_record's struct fields. >> >> >> >> Signed-off-by: Oscar Salvador <osalvador@suse.de> >> >> Reviewed-by: Marco Elver <elver@google.com> >> >> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> >> >> --- >> >> >> #define DEPOT_POOLS_CAP 8192 >> >> -/* The pool_index is offset by 1 so the first record does not have a 0 handle. */ >> >> +/* The pool_index is offset by 1 so the first record does not have a 0 handle */ >> > >> > Why this comment change? We lost the '.' -- for future reference, it'd >> > be good to ensure unnecessary changes don't creep into the diff. This >> > is just nitpicking, >> >> Agree with this part. >> >> > and I've already reviewed this change, so no need >> > to send a v+1. >> >> But confused by this remark. There is a number of nontrivial changes in the >> series from v8, and IIRC v8 was dropped from mm/ meanwhile, so a v+1 of the >> whole series is expected and not fixups. Which means including patches that >> were already reviewed. That's the usual process. > > This is already v9. Of course, still need to look at rest of v9 and if > there are major changes needed then a v10 is needed. Ah sorry I misunderstood you completely. What you meant v10 isn't needed for the missing "." and I thought you were saying v9 already wasn't needed (for this particular patch).
diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h index adcbb8f23600..c4b5ad57c066 100644 --- a/include/linux/stackdepot.h +++ b/include/linux/stackdepot.h @@ -30,6 +30,53 @@ typedef u32 depot_stack_handle_t; */ #define STACK_DEPOT_EXTRA_BITS 5 +#define DEPOT_HANDLE_BITS (sizeof(depot_stack_handle_t) * 8) + +#define DEPOT_POOL_ORDER 2 /* Pool size order, 4 pages */ +#define DEPOT_POOL_SIZE (1LL << (PAGE_SHIFT + DEPOT_POOL_ORDER)) +#define DEPOT_STACK_ALIGN 4 +#define DEPOT_OFFSET_BITS (DEPOT_POOL_ORDER + PAGE_SHIFT - DEPOT_STACK_ALIGN) +#define DEPOT_POOL_INDEX_BITS (DEPOT_HANDLE_BITS - DEPOT_OFFSET_BITS - \ + STACK_DEPOT_EXTRA_BITS) + +#ifdef CONFIG_STACKDEPOT +/* Compact structure that stores a reference to a stack. */ +union handle_parts { + depot_stack_handle_t handle; + struct { + /* pool_index is offset by 1 */ + u32 pool_index : DEPOT_POOL_INDEX_BITS; + u32 offset : DEPOT_OFFSET_BITS; + u32 extra : STACK_DEPOT_EXTRA_BITS; + }; +}; + +struct stack_record { + struct list_head hash_list; /* Links in the hash table */ + u32 hash; /* Hash in hash table */ + u32 size; /* Number of stored frames */ + union handle_parts handle; /* Constant after initialization */ + refcount_t count; + union { + unsigned long entries[CONFIG_STACKDEPOT_MAX_FRAMES]; /* Frames */ + struct { + /* + * An important invariant of the implementation is to + * only place a stack record onto the freelist iff its + * refcount is zero. Because stack records with a zero + * refcount are never considered as valid, it is safe to + * union @entries and freelist management state below. + * Conversely, as soon as an entry is off the freelist + * and its refcount becomes non-zero, the below must not + * be accessed until being placed back on the freelist. + */ + struct list_head free_list; /* Links in the freelist */ + unsigned long rcu_state; /* RCU cookie */ + }; + }; +}; +#endif + typedef u32 depot_flags_t; /* diff --git a/lib/stackdepot.c b/lib/stackdepot.c index c043a4186bc5..4a661a6777da 100644 --- a/lib/stackdepot.c +++ b/lib/stackdepot.c @@ -36,55 +36,12 @@ #include <linux/memblock.h> #include <linux/kasan-enabled.h> -#define DEPOT_HANDLE_BITS (sizeof(depot_stack_handle_t) * 8) - -#define DEPOT_POOL_ORDER 2 /* Pool size order, 4 pages */ -#define DEPOT_POOL_SIZE (1LL << (PAGE_SHIFT + DEPOT_POOL_ORDER)) -#define DEPOT_STACK_ALIGN 4 -#define DEPOT_OFFSET_BITS (DEPOT_POOL_ORDER + PAGE_SHIFT - DEPOT_STACK_ALIGN) -#define DEPOT_POOL_INDEX_BITS (DEPOT_HANDLE_BITS - DEPOT_OFFSET_BITS - \ - STACK_DEPOT_EXTRA_BITS) #define DEPOT_POOLS_CAP 8192 -/* The pool_index is offset by 1 so the first record does not have a 0 handle. */ +/* The pool_index is offset by 1 so the first record does not have a 0 handle */ #define DEPOT_MAX_POOLS \ (((1LL << (DEPOT_POOL_INDEX_BITS)) - 1 < DEPOT_POOLS_CAP) ? \ (1LL << (DEPOT_POOL_INDEX_BITS)) - 1 : DEPOT_POOLS_CAP) -/* Compact structure that stores a reference to a stack. */ -union handle_parts { - depot_stack_handle_t handle; - struct { - u32 pool_index : DEPOT_POOL_INDEX_BITS; /* pool_index is offset by 1 */ - u32 offset : DEPOT_OFFSET_BITS; - u32 extra : STACK_DEPOT_EXTRA_BITS; - }; -}; - -struct stack_record { - struct list_head hash_list; /* Links in the hash table */ - u32 hash; /* Hash in hash table */ - u32 size; /* Number of stored frames */ - union handle_parts handle; /* Constant after initialization */ - refcount_t count; - union { - unsigned long entries[CONFIG_STACKDEPOT_MAX_FRAMES]; /* Frames */ - struct { - /* - * An important invariant of the implementation is to - * only place a stack record onto the freelist iff its - * refcount is zero. Because stack records with a zero - * refcount are never considered as valid, it is safe to - * union @entries and freelist management state below. - * Conversely, as soon as an entry is off the freelist - * and its refcount becomes non-zero, the below must not - * be accessed until being placed back on the freelist. - */ - struct list_head free_list; /* Links in the freelist */ - unsigned long rcu_state; /* RCU cookie */ - }; - }; -}; - static bool stack_depot_disabled; static bool __stack_depot_early_init_requested __initdata = IS_ENABLED(CONFIG_STACKDEPOT_ALWAYS_INIT); static bool __stack_depot_early_init_passed __initdata;