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);
 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)
@@ -240,8 +241,13 @@ static void test_readerwriter(struct kunit *test)
 	struct prbtest_thread_data *thread_data;
 	struct prbtest_data *test_data;
 	struct task_struct *thread;
-	cpumask_t test_cpus;
+	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);
+	KUNIT_ASSERT_EQ(test, err, 0);
 
 	cpus_read_lock();
 	/*
@@ -250,15 +256,15 @@ static void test_readerwriter(struct kunit *test)
 	 * Instead use a snapshot of the online CPUs.
 	 * If they change during test execution it is unfortunate but not a grave error.
 	 */
-	cpumask_copy(&test_cpus, cpu_online_mask);
+	cpumask_copy(test_cpus, cpu_online_mask);
 	cpus_read_unlock();
 
 	/* One CPU is for the reader, all others are writers */
-	reader_cpu = cpumask_first(&test_cpus);
-	if (cpumask_weight(&test_cpus) == 1)
+	reader_cpu = cpumask_first(test_cpus);
+	if (cpumask_weight(test_cpus) == 1)
 		kunit_warn(test, "more than one CPU is recommended");
 	else
-		cpumask_clear_cpu(reader_cpu, &test_cpus);
+		cpumask_clear_cpu(reader_cpu, test_cpus);
 
 	/* KUnit test can get restarted more times. */
 	prbtest_prb_reinit(&test_rb);
@@ -271,7 +277,7 @@ static void test_readerwriter(struct kunit *test)
 
 	kunit_info(test, "running for %lu ms\n", runtime_ms);
 
-	for_each_cpu(cpu, &test_cpus) {
+	for_each_cpu(cpu, test_cpus) {
 		thread_data = kunit_kmalloc(test, sizeof(*thread_data), GFP_KERNEL);
 		KUNIT_ASSERT_NOT_NULL(test, thread_data);
 		thread_data->test_data = test_data;
-- 
2.50.0

Hi Petr,

kernel test robot noticed the following build errors:

[auto build test ERROR on linux-next/master]
[cannot apply to linus/master v6.16-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Petr-Mladek/printk-ringbuffer-Explain-why-the-KUnit-test-ignores-failed-writes/20250702-175422
base:   linux-next/master
patch link:    https://lore.kernel.org/r/20250702095157.110916-3-pmladek%40suse.com
patch subject: [PATCH 2/3] printk: kunit: support offstack cpumask
config: riscv-randconfig-001-20250703 (https://download.01.org/0day-ci/archive/20250703/202507032226.1sEv2EJM-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250703/202507032226.1sEv2EJM-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507032226.1sEv2EJM-lkp@intel.com/

All errors (new ones prefixed by >>):

   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:219:1: note: in expansion of macro 'KUNIT_DEFINE_ACTION_WRAPPER'
     219 | KUNIT_DEFINE_ACTION_WRAPPER(prbtest_cpumask_cleanup, free_cpumask_var, cpumask_var_t);
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
--
   In file included from printk_ringbuffer_kunit_test.c:14:
   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;                    \
         |                                ^
   printk_ringbuffer_kunit_test.c:219:1: note: in expansion of macro 'KUNIT_DEFINE_ACTION_WRAPPER'
     219 | KUNIT_DEFINE_ACTION_WRAPPER(prbtest_cpumask_cleanup, free_cpumask_var, cpumask_var_t);
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +409 include/kunit/resource.h

b9dce8a1ed3efe David Gow 2023-05-25  392  
56778b49c9a2cb David Gow 2023-11-28  393  /**
56778b49c9a2cb David Gow 2023-11-28  394   * KUNIT_DEFINE_ACTION_WRAPPER() - Wrap a function for use as a deferred action.
56778b49c9a2cb David Gow 2023-11-28  395   *
56778b49c9a2cb David Gow 2023-11-28  396   * @wrapper: The name of the new wrapper function define.
56778b49c9a2cb David Gow 2023-11-28  397   * @orig: The original function to wrap.
56778b49c9a2cb David Gow 2023-11-28  398   * @arg_type: The type of the argument accepted by @orig.
56778b49c9a2cb David Gow 2023-11-28  399   *
56778b49c9a2cb David Gow 2023-11-28  400   * Defines a wrapper for a function which accepts a single, pointer-sized
56778b49c9a2cb David Gow 2023-11-28  401   * argument. This wrapper can then be passed to kunit_add_action() and
56778b49c9a2cb David Gow 2023-11-28  402   * similar. This should be used in preference to casting a function
56778b49c9a2cb David Gow 2023-11-28  403   * directly to kunit_action_t, as casting function pointers will break
56778b49c9a2cb David Gow 2023-11-28  404   * control flow integrity (CFI), leading to crashes.
56778b49c9a2cb David Gow 2023-11-28  405   */
56778b49c9a2cb David Gow 2023-11-28  406  #define KUNIT_DEFINE_ACTION_WRAPPER(wrapper, orig, arg_type)	\
56778b49c9a2cb David Gow 2023-11-28  407  	static void wrapper(void *in)				\
56778b49c9a2cb David Gow 2023-11-28  408  	{							\
56778b49c9a2cb David Gow 2023-11-28 @409  		arg_type arg = (arg_type)in;			\
56778b49c9a2cb David Gow 2023-11-28  410  		orig(arg);					\
56778b49c9a2cb David Gow 2023-11-28  411  	}
56778b49c9a2cb David Gow 2023-11-28  412  
56778b49c9a2cb David Gow 2023-11-28  413  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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)
> @@ -240,8 +241,13 @@ static void test_readerwriter(struct kunit *test)
>  	struct prbtest_thread_data *thread_data;
>  	struct prbtest_data *test_data;
>  	struct task_struct *thread;
> -	cpumask_t test_cpus;
> +	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);
> +	KUNIT_ASSERT_EQ(test, err, 0);
>  
>  	cpus_read_lock();
>  	/*
> @@ -250,15 +256,15 @@ static void test_readerwriter(struct kunit *test)
>  	 * Instead use a snapshot of the online CPUs.
>  	 * If they change during test execution it is unfortunate but not a grave error.
>  	 */
> -	cpumask_copy(&test_cpus, cpu_online_mask);
> +	cpumask_copy(test_cpus, cpu_online_mask);
>  	cpus_read_unlock();
>  
>  	/* One CPU is for the reader, all others are writers */
> -	reader_cpu = cpumask_first(&test_cpus);
> -	if (cpumask_weight(&test_cpus) == 1)
> +	reader_cpu = cpumask_first(test_cpus);
> +	if (cpumask_weight(test_cpus) == 1)
>  		kunit_warn(test, "more than one CPU is recommended");
>  	else
> -		cpumask_clear_cpu(reader_cpu, &test_cpus);
> +		cpumask_clear_cpu(reader_cpu, test_cpus);
>  
>  	/* KUnit test can get restarted more times. */
>  	prbtest_prb_reinit(&test_rb);
> @@ -271,7 +277,7 @@ static void test_readerwriter(struct kunit *test)
>  
>  	kunit_info(test, "running for %lu ms\n", runtime_ms);
>  
> -	for_each_cpu(cpu, &test_cpus) {
> +	for_each_cpu(cpu, test_cpus) {
>  		thread_data = kunit_kmalloc(test, sizeof(*thread_data), GFP_KERNEL);
>  		KUNIT_ASSERT_NOT_NULL(test, thread_data);
>  		thread_data->test_data = test_data;
> -- 
> 2.50.0
>

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();

On Tue, Jul 8, 2025, at 16:24, Petr Mladek wrote:
> On Wed 2025-07-02 13:28:35, Nathan Chancellor wrote:
>> On Wed, Jul 02, 2025 at 11:51:56AM +0200, Petr Mladek wrote:
>
> 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.

I'm not familiar enough with the cleanup mechanism of kunit,
but can't you just move the mask allocation outside of
test_readerwriter()?

> + */
> +static void prbtest_free_cpumask_var(cpumask_var_t *mask)
> +{
> +	free_cpumask_var(*mask);
> +}

Or you could pass this as a cpumask_t pointer instead,
which should do the right thing without the indirection.
> 
>  	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);

In my original version, I did not have the
KUNIT_ASSERT_TRUE() here, which seems sufficient since this
is not what you are testing at all, and in normal systems
this would just be a stack variable.

    Arnd

On Tue 2025-07-08 16:48:47, Arnd Bergmann wrote:
> On Tue, Jul 8, 2025, at 16:24, Petr Mladek wrote:
> > On Wed 2025-07-02 13:28:35, Nathan Chancellor wrote:
> >> On Wed, Jul 02, 2025 at 11:51:56AM +0200, Petr Mladek wrote:
> >
> > 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.
> 
> I'm not familiar enough with the cleanup mechanism of kunit,
> but can't you just move the mask allocation outside of
> test_readerwriter()?
> 
> > + */
> > +static void prbtest_free_cpumask_var(cpumask_var_t *mask)
> > +{
> > +	free_cpumask_var(*mask);
> > +}
> 
> Or you could pass this as a cpumask_t pointer instead,
> which should do the right thing without the indirection.

It was actually enough to use this in the KUNIT_DEFINE_ACTION_WRAPPER()
definition. It removed the warning about passing an array type.
And I needed neither a wrapper not #ifdef.

I am going to send an updated version of the patch. I am sorry
that it took so long. This patch has fallen though cracks because
of vacation and other urgent tasks.

Best Regards,
Petr

On Tue 2025-07-08 16:48:47, Arnd Bergmann wrote:
> On Tue, Jul 8, 2025, at 16:24, Petr Mladek wrote:
> > On Wed 2025-07-02 13:28:35, Nathan Chancellor wrote:
> >> On Wed, Jul 02, 2025 at 11:51:56AM +0200, Petr Mladek wrote:
> >
> > 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.
> 
> I'm not familiar enough with the cleanup mechanism of kunit,
> but can't you just move the mask allocation outside of
> test_readerwriter()?

The only solution would be global variable.

test_readerwriter() is the top-level function passed
to KUnit framework via:

	KUNIT_CASE_SLOW(test_readerwriter),

And it seems that the clean is even done in a separate process.

I see the following:

   KUNIT_CASE_SLOW() sets .run_case()

The callback is called via via:

  + kunit_try_run_case()
   +  kunit_run_case_internal()
     + test_case->run_case()

And kunit_try_run_case() is called via:

    + kunit_run_case_catch_errors()
      + kunit_try_catch_run()
	+ kthread_create()
	     -> kunit_try_run_case() in the new thread

The clean up is called from the same kunit_run_case_catch_errors()
in another thread

      + kunit_try_catch_run()
	+ kthread_create()
	    -> kunit_try_run_case_cleanup() in another new thread

> > + */
> > +static void prbtest_free_cpumask_var(cpumask_var_t *mask)
> > +{
> > +	free_cpumask_var(*mask);
> > +}
> 
> Or you could pass this as a cpumask_t pointer instead,
> which should do the right thing without the indirection.

Nice trick. I am going to try it.

> >  	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);
> 
> In my original version, I did not have the
> KUNIT_ASSERT_TRUE() here, which seems sufficient since this
> is not what you are testing at all, and in normal systems
> this would just be a stack variable.

I think that KUNIT_ASSERT is standard handling of any problem in the
test. At least, I see KUNIT_ASSERT*() after any *malloc*() in
the code samples in Documentation/dev-tools/kunit/usage.rst

Best Regards,
Petr

On Wed, Jul 09, 2025 at 01:36:14PM +0200, Petr Mladek wrote:
> On Tue 2025-07-08 16:48:47, Arnd Bergmann wrote:
> > On Tue, Jul 8, 2025, at 16:24, Petr Mladek wrote:
> > > On Wed 2025-07-02 13:28:35, Nathan Chancellor wrote:
> > >> On Wed, Jul 02, 2025 at 11:51:56AM +0200, Petr Mladek wrote:
> > >
> > > 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.
> > 
> > I'm not familiar enough with the cleanup mechanism of kunit,
> > but can't you just move the mask allocation outside of
> > test_readerwriter()?
> 
> The only solution would be global variable.

When the cpumask is allocated on the stack, free_cpumask_var() is a no-op.
So while the stack address would be leaked to another thread,
it should be fine as nothing is ever done with it.
For more clarity it could also be gated explicitly:

if (IS_ENABLED(CONFIG_CPUMASK_OFFSTACK)) {
	err = kunit_add_action_or_reset(test, prbtest_cpumask_cleanup, test_cpus);
	KUNIT_ASSERT_EQ(test, err, 0);
}

<snip>

On Wed 2025-07-09 14:53:29, Thomas Weißschuh wrote:
> On Wed, Jul 09, 2025 at 01:36:14PM +0200, Petr Mladek wrote:
> > On Tue 2025-07-08 16:48:47, Arnd Bergmann wrote:
> > > On Tue, Jul 8, 2025, at 16:24, Petr Mladek wrote:
> > > > On Wed 2025-07-02 13:28:35, Nathan Chancellor wrote:
> > > >> On Wed, Jul 02, 2025 at 11:51:56AM +0200, Petr Mladek wrote:
> > > >
> > > > 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.
> > > 
> > > I'm not familiar enough with the cleanup mechanism of kunit,
> > > but can't you just move the mask allocation outside of
> > > test_readerwriter()?
> > 
> > The only solution would be global variable.
> 
> When the cpumask is allocated on the stack, free_cpumask_var() is a no-op.
> So while the stack address would be leaked to another thread,
> it should be fine as nothing is ever done with it.
> For more clarity it could also be gated explicitly:
> 
> if (IS_ENABLED(CONFIG_CPUMASK_OFFSTACK)) {
> 	err = kunit_add_action_or_reset(test, prbtest_cpumask_cleanup, test_cpus);
> 	KUNIT_ASSERT_EQ(test, err, 0);
> }

It is likely a matter of taste but I like this idea. It looks better
than passing an invalid pointer and hope nobody would do anything
with it.

The only problem is that

    if (IS_ENABLED(CONFIG_CPUMASK_OFFSTACK)) {

did not prevented the compiler warning. I guess that the code was still
compiled and later just optimized out.

So, I am going to use #ifdef instead. I create a function:

/*
 * A cast would be needed for the clean up action when the cpumask was on stack.
 * Also it would leak the stack address to the cleanup thread.
 * And alloc_cpu_mask() and free_cpumask_var() would do nothing anyway.
 */
#ifdef CONFIG_CPUMASK_OFFSTACK
KUNIT_DEFINE_ACTION_WRAPPER(prbtest_cpumask_cleanup, free_cpumask_var, cpumask_var_t);

static void prbtest_alloc_cpumask(struct kunit *test, cpumask_var_t *mask)
{
	int err;

	KUNIT_ASSERT_TRUE(test, alloc_cpumask_var(mask, GFP_KERNEL));
	err = kunit_add_action_or_reset(test, prbtest_cpumask_cleanup, *mask);
	KUNIT_ASSERT_EQ(test, err, 0);
}
#else
static inline
void prbtest_alloc_cpumask(struct kunit *test, cpumask_var_t *mask) {}
#endif

which will be called in test_readerwriter().

It seems to work, ..., sigh. I did not expect so many troubles with
a tiny detail.

Best Regards,
Petr

On Thu, Jul 10, 2025, at 15:51, Petr Mladek wrote:
> On Wed 2025-07-09 14:53:29, Thomas Weißschuh wrote:
>> On Wed, Jul 09, 2025 at 01:36:14PM +0200, Petr Mladek wrote:
>> if (IS_ENABLED(CONFIG_CPUMASK_OFFSTACK)) {
>> 	err = kunit_add_action_or_reset(test, prbtest_cpumask_cleanup, test_cpus);
>> 	KUNIT_ASSERT_EQ(test, err, 0);
>> }
>
> It is likely a matter of taste but I like this idea. It looks better
> than passing an invalid pointer and hope nobody would do anything
> with it.
>
> The only problem is that
>
>     if (IS_ENABLED(CONFIG_CPUMASK_OFFSTACK)) {
>
> did not prevented the compiler warning. I guess that the code was still
> compiled and later just optimized out.

Right, gcc does some of the warnings after dead code eliminations
and some before. clang tries to do all warnings before eliminating
dead code, so you still lose.

> /*
>  * A cast would be needed for the clean up action when the cpumask was 
> on stack.
>  * Also it would leak the stack address to the cleanup thread.
>  * And alloc_cpu_mask() and free_cpumask_var() would do nothing anyway.
>  */
> #ifdef CONFIG_CPUMASK_OFFSTACK
> KUNIT_DEFINE_ACTION_WRAPPER(prbtest_cpumask_cleanup, free_cpumask_var, 
> cpumask_var_t);
>
> static void prbtest_alloc_cpumask(struct kunit *test, cpumask_var_t *mask)
> {
> 	int err;
>
> 	KUNIT_ASSERT_TRUE(test, alloc_cpumask_var(mask, GFP_KERNEL));
> 	err = kunit_add_action_or_reset(test, prbtest_cpumask_cleanup, *mask);
> 	KUNIT_ASSERT_EQ(test, err, 0);
> }
> #else
> static inline
> void prbtest_alloc_cpumask(struct kunit *test, cpumask_var_t *mask) {}
> #endif
>
> which will be called in test_readerwriter().

Looks fine to me

> It seems to work, ..., sigh. I did not expect so many troubles with
> a tiny detail.

I wonder if just making the cpumask_t 'static' would still be
simpler, given that there are no concurrent callers.

    Arnd