[v9,2/7] lib/stackdepot: Move stack_record struct definition into the header
Commit Message
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(-)
Comments
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).
@@ -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;
/*
@@ -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;