Re: [PATCH 2/3] printk: kunit: support offstack cpumask

[Petr Mladek <pmladek@suse.com>]

On Wed 2025-07-02 13:28:35, Nathan Chancellor wrote:
> On Wed, Jul 02, 2025 at 11:51:56AM +0200, Petr Mladek wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > For large values of CONFIG_NR_CPUS, the newly added kunit test fails
> > to build:
> > 
> > kernel/printk/printk_ringbuffer_kunit_test.c: In function 'test_readerwriter':
> > kernel/printk/printk_ringbuffer_kunit_test.c:279:1: error: the frame size of 1432 bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
> > 
> > Change this to use cpumask_var_t and allocate it dynamically when
> > CONFIG_CPUMASK_OFFSTACK is set.
> > 
> > Fixes: 5ea2bcdfbf46 ("printk: ringbuffer: Add KUnit test")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > [pmladek@suse.com: Correctly handle allocation failures and freeing using KUnit test API.]
> > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > ---
> >  kernel/printk/printk_ringbuffer_kunit_test.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/printk/printk_ringbuffer_kunit_test.c b/kernel/printk/printk_ringbuffer_kunit_test.c
> > index 217dcc14670c..0c3030fde8c2 100644
> > --- a/kernel/printk/printk_ringbuffer_kunit_test.c
> > +++ b/kernel/printk/printk_ringbuffer_kunit_test.c
> > @@ -216,6 +216,7 @@ static int prbtest_reader(struct prbtest_data *test_data, unsigned long timeout_
> >  	return 0;
> >  }
> >  
> > +KUNIT_DEFINE_ACTION_WRAPPER(prbtest_cpumask_cleanup, free_cpumask_var, cpumask_var_t);
> 
> This appears to break the build for me when CONFIG_CPUMASK_OFFSTACK is not
> set, like when enabling this test on top of x86_64 defconfig:
> 
>   In file included from kernel/printk/printk_ringbuffer_kunit_test.c:14:
>   kernel/printk/printk_ringbuffer_kunit_test.c: In function 'prbtest_cpumask_cleanup':
>   include/kunit/resource.h:409:32: error: cast specifies array type
>     409 |                 arg_type arg = (arg_type)in;                    \
>         |                                ^
>   kernel/printk/printk_ringbuffer_kunit_test.c:226:1: note: in expansion of macro 'KUNIT_DEFINE_ACTION_WRAPPER'
>     226 | KUNIT_DEFINE_ACTION_WRAPPER(prbtest_cpumask_cleanup, free_cpumask_var, cpumask_var_t);
>         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Clang's error might be a little clearer with the "aka" note it provides.
> 
>   kernel/printk/printk_ringbuffer_kunit_test.c:226:1: error: used type 'cpumask_var_t' (aka 'struct cpumask[1]') where arithmetic or pointer type is required
>     226 | KUNIT_DEFINE_ACTION_WRAPPER(prbtest_cpumask_cleanup, free_cpumask_var, cpumask_var_t);
>         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   include/kunit/resource.h:409:18: note: expanded from macro 'KUNIT_DEFINE_ACTION_WRAPPER'
>     409 |                 arg_type arg = (arg_type)in;                    \
>         |                                ^         ~~
> 
> >  KUNIT_DEFINE_ACTION_WRAPPER(prbtest_kthread_cleanup, kthread_stop, struct task_struct *);
> >  
> >  static void prbtest_add_kthread_cleanup(struct kunit *test, struct task_struct *kthread)

Thanks a lot for the nice report.

The problem is how cpumask_var_t is defined in include/linux/cpumask_types.h:

#ifdef CONFIG_CPUMASK_OFFSTACK
typedef struct cpumask *cpumask_var_t;
#else
typedef struct cpumask cpumask_var_t[1];
#endif /* CONFIG_CPUMASK_OFFSTACK */

And KUNIT_DEFINE_ACTION_WRAPPER() expect that the 3rd parameter
is a pointer.

I am going to solve this by adding a wrapper over free_cpumask_var()
which would work with a pointer to cpumask_var_t.

It has another catch, though. It seems that the automatic clean up is
done after test_readerwriter() finishes. Therefore the variable
@test_cpus can't be on stack.

Anyway, the following seems to work with both CONFIG_CPUMASK_OFFSTACK
enabled and disabled:

--- a/kernel/printk/printk_ringbuffer_kunit_test.c
+++ b/kernel/printk/printk_ringbuffer_kunit_test.c
@@ -223,7 +223,17 @@ static int prbtest_reader(struct prbtest_data *test_data, unsigned long timeout_
 	return 0;
 }
 
-KUNIT_DEFINE_ACTION_WRAPPER(prbtest_cpumask_cleanup, free_cpumask_var, cpumask_var_t);
+/*
+ * Add a custom wrapper around free_cpumask_var() to be used by
+ * KUNIT_DEFINE_ACTION_WRAPPER(). It allows to pass @mask using
+ * a pointer even when CONFIG_CPUMASK_OFFSTACK is disabled.
+ */
+static void prbtest_free_cpumask_var(cpumask_var_t *mask)
+{
+	free_cpumask_var(*mask);
+}
+
+KUNIT_DEFINE_ACTION_WRAPPER(prbtest_cpumask_cleanup, prbtest_free_cpumask_var, cpumask_var_t *);
 KUNIT_DEFINE_ACTION_WRAPPER(prbtest_kthread_cleanup, kthread_stop, struct task_struct *);
 
 static void prbtest_add_kthread_cleanup(struct kunit *test, struct task_struct *kthread)
@@ -244,16 +254,19 @@ static void test_readerwriter(struct kunit *test)
 {
 	/* Equivalent to CONFIG_LOG_BUF_SHIFT=13 */
 	DEFINE_PRINTKRB(test_rb, 8, 5);
-
+	/*
+	 * @test_cpus can't be on stack. THe pointer to this variable is passed
+	 * to an automatic clean up action, see prbtest_free_cpumask_var().
+	 */
+	static cpumask_var_t test_cpus;
 	struct prbtest_thread_data *thread_data;
 	struct prbtest_data *test_data;
 	struct task_struct *thread;
-	cpumask_var_t test_cpus;
 	int cpu, reader_cpu;
 	int err;
 
 	KUNIT_ASSERT_TRUE(test, alloc_cpumask_var(&test_cpus, GFP_KERNEL));
-	err = kunit_add_action_or_reset(test, prbtest_cpumask_cleanup, test_cpus);
+	err = kunit_add_action_or_reset(test, prbtest_cpumask_cleanup, &test_cpus);
 	KUNIT_ASSERT_EQ(test, err, 0);
 
 	cpus_read_lock();