Re: [PATCH 6/6] md/raid5: derive the default group_thread_cnt from the hardware

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Double-read of the sysfs-modifiable `group_thread_cnt_default` module parameter can cause an array initialization failure due to a large negative worker count.
- [Medium] Massive overallocation of worker threads and worker groups on systems with sparse NUMA node maps.

Pre-existing issues:
- [High] A failure to register the CPU hotplug instance in `raid5_alloc_percpu()` causes a NULL pointer dereference (Oops) during error cleanup.
--

--- Patch [6]: [PATCH 6/6] md/raid5: derive the default group_thread_cnt from the hardware ---
commit 9c6dbf46ca41e580ada80950d4adaba43e7b0100
Author: Hiroshi Nishida <[email protected]>

md/raid5: derive the default group_thread_cnt from the hardware

This commit updates the RAID5 subsystem to automatically configure the number of
worker threads per NUMA node based on the hardware CPU count, rather than
defaulting to a single raid5d thread. It calculates a default value bounded by
a cap, and allows overriding this via a new module parameter or the existing
sysfs attribute.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 8e1c2eba42417..6e91eb0ad5759 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c

[ ... ]

> @@ -7584,6 +7589,31 @@ static unsigned long raid5_cache_count(struct shrinker *shrink,
>  #define RAID5_CACHE_DEFAULT_RAM_SHIFT	9	/* above it: ~1/512 of the extra RAM */
>  #define RAID5_CACHE_DEFAULT_MAX		4096
>  
> +/*
> + * Default group_thread_cnt (worker_cnt_per_group) for a new array when the
> + * group_thread_cnt_default module parameter is left at -1.  The historical
> + * default is 0 -- a single raid5d thread -- which cannot keep a fast, wide
> + * array busy on a many-core host.  Derive a starting point from the CPU
> + * count: half the online CPUs divided across the NUMA nodes (this is a
> + * per-node count -- see alloc_thread_groups() -- so the total lands near
> + * half the online CPUs regardless of socket count), capped at
> + * RAID5_AUTO_GROUP_THREAD_MAX.  This is only a ceiling:
> + * raid5_wakeup_stripe_thread() wakes workers in proportion to the queued
> + * stripe count, so a lightly loaded array uses far fewer.  A lone worker is
> + * not worth its overhead, so 1 collapses back to 0.  The group_thread_cnt
> + * sysfs attribute overrides this per array.
> + */
> +#define RAID5_AUTO_GROUP_THREAD_MAX	256
> +
> +static int raid5_default_group_thread_cnt(void)
> +{
> +	unsigned int gtc = num_online_cpus() / (2 * num_possible_nodes());

[Severity: Medium]
Could this lead to massive overallocation of worker threads on systems with
sparse NUMA node maps?

Here in raid5_default_group_thread_cnt() in drivers/md/raid5.c, gtc is
calculated using num_possible_nodes(), which returns the actual count of
possible nodes. However, alloc_thread_groups() allocates threads based on
nr_node_ids:

drivers/md/raid5.c:alloc_thread_groups() {
	...
	*group_cnt = nr_node_ids;
	size = sizeof(struct r5worker) * cnt;
	workers = kcalloc(size, *group_cnt, GFP_NOIO);
	...
}

If a system has sparse NUMA node IDs (e.g., nodes 0 and 64, making
num_possible_nodes() == 2 but nr_node_ids == 65), would multiplying the
large gtc value by the highest node ID cause hundreds of unused struct
r5worker and struct r5worker_group instances to be allocated?

> +
> +	if (gtc > RAID5_AUTO_GROUP_THREAD_MAX)
> +		gtc = RAID5_AUTO_GROUP_THREAD_MAX;
> +	return gtc == 1 ? 0 : gtc;
> +}
> +
>  static struct r5conf *setup_conf(struct mddev *mddev)
>  {
>  	struct r5conf *conf;

[ ... ]

> @@ -7677,10 +7708,18 @@ static struct r5conf *setup_conf(struct mddev *mddev)
>  		goto abort;
>  	for (i = 0; i < PENDING_IO_MAX; i++)
>  		list_add(&conf->pending_data[i].sibling, &conf->free_list);
> -	/* Don't enable multi-threading by default*/
> -	if (!alloc_thread_groups(conf, 0, &group_cnt, &new_group)) {
> +	/*
> +	 * Multi-threading defaults to a hardware-derived worker count (see
> +	 * raid5_default_group_thread_cnt()); group_thread_cnt_default overrides
> +	 * the choice, and the group_thread_cnt sysfs attribute overrides it per
> +	 * array.
> +	 */
> +	def_threads = group_thread_cnt_default < 0 ?
> +		raid5_default_group_thread_cnt() :
> +		min(group_thread_cnt_default, RAID5_AUTO_GROUP_THREAD_MAX);

[Severity: Medium]
Is it possible for a concurrent sysfs write to cause an array initialization
failure due to a large negative worker count?

The module parameter group_thread_cnt_default is evaluated twice without
READ_ONCE() (once in the ternary condition, and once in the min() macro).

Could an administrator writing to the sysfs parameter concurrently with array
assembly cause the ternary condition to read a positive value, and the min()
macro to read a negative value, resulting in def_threads becoming -1?

> +	if (!alloc_thread_groups(conf, def_threads, &group_cnt, &new_group)) {
>  		conf->group_cnt = group_cnt;
> -		conf->worker_cnt_per_group = 0;
> +		conf->worker_cnt_per_group = def_threads;
>  		conf->worker_groups = new_group;
>  	} else
>  		goto abort;

[Severity: High]
This isn't a bug introduced by this patch, but does the cleanup path for
setup failures lead to a NULL pointer dereference?

If cpuhp_state_add_instance() in raid5_alloc_percpu() previously returned
an error (like -ENOMEM) and failed to register the hotplug node, the error
bubbles up and causes a jump to the abort label.

In the cleanup path, raid5_free_percpu() unconditionally calls
cpuhp_state_remove_instance():

drivers/md/raid5.c:raid5_free_percpu() {
	if (!conf->percpu)
		return;
	cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
	...
}

Because conf->node is completely zero-initialized and was never successfully
hashed, its pprev pointer is NULL. Won't the subsequent hlist_del() in the
teardown path dereference this NULL pointer and crash the kernel? Should we
check hlist_unhashed(&conf->node) before removing the instance?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6