Re: [PATCH v12 26/40] dyndbg-test: add do_bulk testpoint, rename do_prints to do_classes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] test_dynamic_debug: classmap offset mismatch for submodule classes
- [Low] locking: param_set_do_repeats blocks param_lock for up to 60s
--

commit adf504f365302c7db4e37850cebef69f82ed353f
Author: Jim Cromie <jim.cromie@gmail.com>

dyndbg-test: add do_bulk testpoint, rename do_prints to do_classes

This patch renames do_prints to do_classes to better align with its purpose
and introduces do_bulk to generate high-volume workloads. It adds support for
executing these debug operations multiple times or over a timed duration using
shared module parameter callbacks.

> diff --git a/lib/test_dynamic_debug.c b/lib/test_dynamic_debug.c
> index 01ce07001d4c7..19e27b5747ac2 100644
> --- a/lib/test_dynamic_debug.c
> +++ b/lib/test_dynamic_debug.c

[ ... ]

> @@ -28,25 +28,57 @@
>  #endif
>  
>  #include <linux/module.h>
> +#include <linux/sched.h>
> +#include <linux/sched/signal.h>
> +#include <linux/jiffies.h>
>  
> -/* re-gen output by reading or writing sysfs node: do_prints */
> +#define TEST_MAX_REPEATS 10000
> +#define TEST_MAX_SECONDS 60
>  
> -static void do_prints(void); /* device under test */
> -static int param_set_do_prints(const char *instr, const struct kernel_param *kp)
> +/* re-trigger debug output by reading or writing sysfs nodes: do_classes or do_bulk */
> +static void do_classes(int); /* device under test */
> +static void do_bulk(int);    /* device under test */
> +
> +static int param_set_do_repeats(const char *instr, const struct kernel_param *kp)
>  {
> -	do_prints();
> +	int rc, ct;
> +	void (*repeat_fn)(int) = kp->arg;
> +
> +	rc = kstrtoint(instr, 0, &ct);
> +	if (rc) {
> +		pr_err("expecting numeric input, using 1 instead\n");
> +		ct = 1;
> +	}
> +	if (ct > TEST_MAX_REPEATS) {
> +		pr_warn("%s: clamping %d to %d\n", kp->name, ct, TEST_MAX_REPEATS);
> +		ct = TEST_MAX_REPEATS;
> +	} else if (ct < -TEST_MAX_SECONDS) {
> +		pr_warn("%s: clamping %d seconds to -%d\n",
> +			kp->name, ct, TEST_MAX_SECONDS);
> +		ct = -TEST_MAX_SECONDS;
> +	}
> +
> +	repeat_fn(ct);

[Severity: Low]
Does this synchronous loop execution block param_lock system-wide?

When test_dynamic_debug is built-in, writing to its module parameters via
sysfs triggers param_attr_store() in kernel/params.c, which acquires the
global param_lock mutex around the .set callback:

kernel/params.c:param_attr_store() {
    ...
    kernel_param_lock(mk->mod);
    if (param_check_unsafe(attribute->param))
        err = attribute->param->ops->set(buf, attribute->param);
    ...
}

Because param_set_do_repeats() invokes the workload synchronously (which
can run for up to 60 seconds as shown in do_classes() below), this starves
param_lock, preventing other processes from reading or writing any
built-in module parameters for the duration.

> +
>  	return 0;
>  }

[ ... ]

> @@ -103,7 +135,10 @@ enum cat_disjoint_bits {
>  	D2_DRMRES };
>  
>  /* numeric verbosity, V2 > V1 related.  V1 is > D2_DRMRES */
> -enum cat_level_num { V1 = 16, V2, V3, V4, V5, V6, V7 };
> +enum cat_level_num { V1 = 16, V2, V3, V4, V5, V6, V7, V8 };
> +
> +/* test _USE_ w offset */
> +enum cat_level_offset { Vu1 = V1 + 8, Vu2, Vu3, Vu4, Vu5, Vu6, Vu7, Vu8 };
>  
>  /* recapitulate DRM's multi-classmap setup */
>  #if !defined(TEST_DYNAMIC_DEBUG_SUBMOD)

[ ... ]

> @@ -160,6 +183,19 @@ DYNAMIC_DEBUG_CLASSMAP_DEFINE(fail_emptyclass, 0, 0 /* ,empty */);
>  DYNAMIC_DEBUG_CLASSMAP_DEFINE(fail_maptype, 3, 10, "no such type");
>  DYNAMIC_DEBUG_CLASSMAP_DEFINE(fail_base_len, 0, 60,
>  			      "base", "plus", "classes", "length", "too-big");
> +#endif
> +
> +#else /* TEST_DYNAMIC_DEBUG_SUBMOD */
> +
> +/*
> + * in submod/drm-drivers, use the classmaps defined in top/parent
> + * module above.
> + */
> +
> +DYNAMIC_DEBUG_CLASSMAP_USE(map_disjoint_bits);
> +DYNAMIC_DEBUG_CLASSMAP_USE_(map_level_num, 7);

[Severity: Low]
Is this registration macro passing an outdated offset?

The patch adds V8 to map_level_num, increasing its length to 8. While
cat_level_offset correctly shifts the submodule's class definitions by 8
(Vu1 = V1 + 8) above, this registration macro still tells dyndbg the
classes start at offset 7.

This mismatch causes submodule debug callsites to map to the wrong class
IDs, leaving the last class (Vu8) completely out of bounds and
uncontrollable.

> +
> +#if defined(DD_MACRO_ARGCHECK)
>  DYNAMIC_DEBUG_CLASSMAP_USE_(fail_offset_big, 100);
>  #endif /* DD_MACRO_ARGCHECK */

[ ... ]

> @@ -210,20 +247,85 @@ static void do_levels(void)

[ ... ]

> -static void do_prints(void)
> +static void do_classes(int ct)
> +{
> +	if (ct < 0) {
> +		unsigned long timeout = jiffies + (-ct) * HZ;
> +
> +		pr_debug("%s for %d seconds:\n", __func__, -ct);
> +		while (time_before(jiffies, timeout)) {
> +			if (signal_pending(current))
> +				break;
> +			do_cats();
> +			do_levels();
> +			cond_resched();
> +		}
> +		return;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-dd-cmap-part2-clean-v12-0-3c8cc5725def@gmail.com?part=26