* Re: [PATCH 5/5] md: clarify the resync ETA comment in status_resync()
From: sashiko-bot @ 2026-06-24 15:58 UTC (permalink / raw)
To: Hiroshi Nishida; +Cc: linux-raid, yukuai
In-Reply-To: <20260624155421.211626-6-nishidafmly@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The newly added comment incorrectly states that the divisor `db` is scaled up by 32, when the code actually scales it down.
--
commit 21672ecf7d2b823245c32b9d2699f7cf1a5c2e7c
Author: Hiroshi Nishida <nishidafmly@gmail.com>
md: clarify the resync ETA comment in status_resync()
This commit updates the block comment above the remaining-time computation
in status_resync() to clarify the calculation logic. It is a comment-only
change.
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 0cd85cc92ed31..a5c0da0d11334 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -8916,17 +8916,11 @@ static int status_resync(struct seq_file *seq, struct mddev *mddev)
> * db: blocks written from mark until now
> * rt: remaining time
> *
> - * rt is a sector_t, which is always 64bit now. We are keeping
> - * the original algorithm, but it is not really necessary.
> - *
> - * Original algorithm:
> - * So we divide before multiply in case it is 32bit and close
> - * to the limit.
> - * We scale the divisor (db) by 32 to avoid losing precision
> - * near the end of resync when the number of remaining sectors
> - * is close to 'db'.
> - * We then divide rt by 32 after multiplying by db to compensate.
> - * The '+1' avoids division by zero if db is very small.
> + * rt is computed as (remaining sectors) * dt / db. To keep precision
> + * near the end of resync, when the remaining count is close to db, the
> + * divisor db is scaled up by 32 before the divide and rt is scaled back
[Severity: Low]
Is this new comment accurate regarding the math applied to db?
The comment states that the divisor db is scaled up by 32, but looking at
the actual code:
rt = max_sectors - resync;
rt = div64_u64(rt, db/32+1);
This divides db by 32, effectively scaling it down rather than up (which in
turn scales the resulting quotient up). Does this comment need to be
adjusted to reflect the db/32 downscaling?
> + * down by 32 afterwards. The '+1' avoids division by zero when db is
> + * very small.
> */
> dt = ((jiffies - mddev->resync_mark) / HZ);
> if (!dt) dt++;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624155421.211626-1-nishidafmly@gmail.com?part=5
^ permalink raw reply
* Re: [PATCH 4/5] md: use sector_t for recovery_active in status_resync()
From: sashiko-bot @ 2026-06-24 16:03 UTC (permalink / raw)
To: Hiroshi Nishida; +Cc: yukuai, linux-raid
In-Reply-To: <20260624155421.211626-5-nishidafmly@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message falsely claims `mddev->recovery_active` is a 'sector-typed atomic' and that declaring a local variable as `int` 'needlessly narrows it', but the structure field remains a standard 32-bit `atomic_t`.
--
commit 7ec77a6b233ce4d151f7fc723e5f312b20b4ac92
Author: Hiroshi Nishida <nishidafmly@gmail.com>
md: use sector_t for recovery_active in status_resync()
This commit changes the local variable recovery_active in status_resync()
from int to sector_t. The commit message states this is to match the type
of mddev->recovery_active and avoid needlessly narrowing the value.
> recovery_active holds a sector count read from the sector-typed atomic
> mddev->recovery_active and is then combined with the sector_t values
> curr_mark_cnt and resync_mark_cnt. Declaring it as a plain int needlessly
> narrows it and mixes signedness into sector_t arithmetic; declare it
> sector_t to match.
[Severity: Medium]
Is this description accurate? Looking at struct mddev in drivers/md/md.h,
recovery_active is defined as a standard atomic_t:
drivers/md/md.h:struct mddev {
...
atomic_t recovery_active;
...
}
Since atomic_t is a 32-bit signed integer, and atomic_read() returns an int,
does changing this local variable to sector_t actually prevent narrowing? The
implicit cast from int to sector_t will now simply happen at assignment
rather than during the arithmetic operations.
Could the commit message be updated to reflect that mddev->recovery_active
is a standard atomic_t, rather than a "sector-typed atomic"?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624155421.211626-1-nishidafmly@gmail.com?part=4
^ permalink raw reply
* Re: [PATCH 6/8] md/raid5: allocate worker groups per NUMA node
From: sashiko-bot @ 2026-06-24 16:07 UTC (permalink / raw)
To: Hiroshi Nishida; +Cc: linux-raid, yukuai
In-Reply-To: <20260624155452.211646-7-nishidafmly@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] The patch incorrectly uses a sequential loop index `i` as the NUMA node ID parameter to `kzalloc_node()`.
Pre-existing issues:
- [High] The `worker_groups` array is sized using `num_possible_nodes()` but is indexed by the raw NUMA node ID, leading to out-of-bounds accesses on systems with sparse NUMA node IDs.
--
commit bce08bf6ce446b00da625ff4028f5782b756e17c
Author: Hiroshi Nishida <nishidafmly@gmail.com>
md/raid5: allocate worker groups per NUMA node
alloc_thread_groups() previously allocated all r5worker arrays in a
single kcalloc() block, assigning workers for NUMA node N from node 0
memory. On multi-socket systems this causes remote memory traffic on
every worker->work and worker->temp_inactive_list access.
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 8e9edaaca667b..c8787ab7b309c 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
[ ... ]
> @@ -7336,7 +7340,6 @@ static int alloc_thread_groups(struct r5conf *conf, int cnt, int *group_cnt,
[ ... ]
> *group_cnt = num_possible_nodes();
> - size = sizeof(struct r5worker) * cnt;
> - workers = kcalloc(size, *group_cnt, GFP_NOIO);
> *worker_groups = kzalloc_objs(struct r5worker_group, *group_cnt,
> GFP_NOIO);
[Severity: High]
This isn't a bug introduced by this patch, but could this array sizing cause
an out-of-bounds access on systems with sparse NUMA nodes?
We size the worker_groups array using num_possible_nodes(), which is a count.
On systems with sparse NUMA node IDs (for example, nodes 0 and 8), the array
would only have 2 elements. Later, in raid5_wakeup_stripe_thread(), we index
into this array using cpu_to_group(cpu) which expands to cpu_to_node(cpu).
If a CPU belongs to node 8, we would access conf->worker_groups[8], exceeding
the allocated size and corrupting memory.
> - if (!*worker_groups || !workers) {
> - kfree(workers);
> - kfree(*worker_groups);
> + if (!*worker_groups)
> return -ENOMEM;
> - }
>
> + size = sizeof(struct r5worker) * cnt;
> for (i = 0; i < *group_cnt; i++) {
> - struct r5worker_group *group;
> + struct r5worker_group *group = &(*worker_groups)[i];
> + struct r5worker *workers;
> +
> + workers = kzalloc_node(size, GFP_NOIO, i);
[Severity: High]
Could passing the sequential loop index i directly to kzalloc_node cause a
NULL pointer dereference on systems with sparse NUMA nodes?
Since num_possible_nodes returns a count rather than an upper bound on IDs,
on a system with nodes 0 and 8, i would be evaluated as 1. Passing an
unpopulated node ID to kzalloc_node without validation can lead to a crash
when accessing NODE_DATA(nid). Should this loop use
for_each_node_state(nid, N_MEMORY) or similar to get the actual node ID
instead of using the loop counter?
> + if (!workers)
> + goto out_free;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624155452.211646-1-nishidafmly@gmail.com?part=6
^ permalink raw reply
* Re: [PATCH 7/8] md/raid5: raise MAX_STRIPE_BATCH from 8 to 32
From: sashiko-bot @ 2026-06-24 16:09 UTC (permalink / raw)
To: Hiroshi Nishida; +Cc: yukuai, linux-raid
In-Reply-To: <20260624155452.211646-8-nishidafmly@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Increasing MAX_STRIPE_BATCH to 32 without proportionally increasing STRIPE_BATCH_WORKERS serializes stripe processing and causes worker starvation.
--
commit b1a0a169dabd8b46b6b9ca40375034cf879934c6
Author: Hiroshi Nishida <nishidafmly@gmail.com>
md/raid5: raise MAX_STRIPE_BATCH from 8 to 32
This commit increases the maximum stripe batch size from 8 to 32 to
amortize device_lock acquisition over more stripes. It also introduces
STRIPE_BATCH_WORKERS to decouple the worker spawning threshold from the
batch size.
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index c8787ab7b309c..ad6230415af33 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
[ ... ]
> @@ -210,7 +210,7 @@ static void raid5_wakeup_stripe_thread(struct stripe_head *sh)
> /* at least one worker should run to avoid race */
> queue_work_on(sh->cpu, raid5_wq, &group->workers[0].work);
>
> - thread_cnt = group->stripes_cnt / MAX_STRIPE_BATCH - 1;
> + thread_cnt = group->stripes_cnt / STRIPE_BATCH_WORKERS - 1;
> /* wakeup more workers */
> for (i = 1; i < conf->worker_cnt_per_group && thread_cnt > 0; i++) {
[Severity: High]
Will increasing the stripe batch size while keeping the worker spawn threshold
small serialize stripe processing and starve the worker threads?
With this change, raid5_wakeup_stripe_thread() still wakes up an additional
worker for every 8 pending stripes (e.g., waking 4 threads for 32 stripes).
However, the first awakened worker executing handle_active_stripes() will
greedily dequeue up to MAX_STRIPE_BATCH (32) stripes under the lock:
drivers/md/raid5.c:handle_active_stripes() {
...
while (batch_size < MAX_STRIPE_BATCH &&
(sh = __get_priority_stripe(conf, group)) != NULL)
batch[batch_size++] = sh;
...
}
It appears the first thread will steal all 32 pending stripes and process
them sequentially, while the other awakened threads acquire the lock,
find the lists empty, and exit without doing any work.
Could this cause a performance regression by serializing CPU-intensive parity
operations onto a single thread, while also creating unnecessary lock
contention from the starved threads?
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index 7aeba1fc7f09e..1f37dabd727be 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
[ ... ]
> @@ -496,7 +496,8 @@ struct disk_info {
> #define BYPASS_THRESHOLD 1
> #define NR_HASH (PAGE_SIZE / sizeof(struct hlist_head))
> #define HASH_MASK (NR_HASH - 1)
> -#define MAX_STRIPE_BATCH 8
> +#define MAX_STRIPE_BATCH 32 /* stripes per handle_active_stripes pass */
> +#define STRIPE_BATCH_WORKERS 8 /* stripes-per-worker threshold for spawning */
> #define RAID5_SYNC_WINDOW 32 /* stripes to pre-submit per sync_request call */
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624155452.211646-1-nishidafmly@gmail.com?part=7
^ permalink raw reply
* Re: [PATCH 5/8] md/raid5: submit a window of stripes during resync/recovery
From: sashiko-bot @ 2026-06-24 16:12 UTC (permalink / raw)
To: Hiroshi Nishida; +Cc: linux-raid, yukuai
In-Reply-To: <20260624155452.211646-6-nishidafmly@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The batch loop ignores the return value of md_bitmap_start_sync(), forcing clean stripes to be unnecessarily synchronized.
- [High] Time-of-check to time-of-use race on the array's degraded state, leading to silent data corruption via incorrect bitmap updates.
--
commit 01d1e80ee6aa253c48c233e7fc218fbdac9522fa
Author: Hiroshi Nishida <nishidafmly@gmail.com>
md/raid5: submit a window of stripes during resync/recovery
This commit alters raid5_sync_request() to dispatch up to 32 stripes per
call instead of one. It attempts to submit the remaining stripes in the
window with a non-blocking flag to avoid the per-stripe throttle sleep.
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 9cb4ed3bd85c5..8e9edaaca667b 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -6620,16 +6621,7 @@ static inline sector_t raid5_sync_request(...)
[ ... ]
> - sh = raid5_get_active_stripe(conf, NULL, sector_nr,
> - R5_GAS_NOBLOCK);
> - if (sh == NULL) {
> - sh = raid5_get_active_stripe(conf, NULL, sector_nr, 0);
> - /* make sure we don't swamp the stripe cache if someone else
> - * is trying to get access
> - */
> - schedule_timeout_uninterruptible(1);
> - }
> - /* Need to check if array will still be degraded after recovery/resync
> + /* Check once whether array will still be degraded after recovery/resync.
> * Note in case of > 1 drive failures it's possible we're rebuilding
> * one drive while leaving another faulty drive in array.
> */
> @@ -6640,13 +6632,42 @@ static inline sector_t raid5_sync_request(...)
> still_degraded = true;
> }
>
> + /* First stripe: block if stripe cache is full, then throttle. */
> + sh = raid5_get_active_stripe(conf, NULL, sector_nr, R5_GAS_NOBLOCK);
> + if (sh == NULL) {
> + sh = raid5_get_active_stripe(conf, NULL, sector_nr, 0);
> + /* make sure we don't swamp the stripe cache if someone else
> + * is trying to get access
> + */
> + schedule_timeout_uninterruptible(1);
> + }
[Severity: High]
Does moving the degraded check before the stripe allocation introduce a
time-of-check to time-of-use race?
If a drive fails during the potential sleep in raid5_get_active_stripe() or
schedule_timeout_uninterruptible(), the stale still_degraded value is used
below to update the bitmap. This could incorrectly clear the NEEDED bit,
potentially leaving parity inconsistent if the system crashes before the sync
completes.
> md_bitmap_start_sync(mddev, sector_nr, &sync_blocks, still_degraded);
> set_bit(STRIPE_SYNC_REQUESTED, &sh->state);
> set_bit(STRIPE_HANDLE, &sh->state);
[ ... ]
> + win_sector = sector_nr + RAID5_STRIPE_SECTORS(conf);
> + for (submitted = 1;
> + submitted < RAID5_SYNC_WINDOW && win_sector < max_sector &&
> + win_sector < mddev->resync_max;
> + submitted++, win_sector += RAID5_STRIPE_SECTORS(conf)) {
> + sh = raid5_get_active_stripe(conf, NULL, win_sector,
> + R5_GAS_NOBLOCK);
> + if (!sh)
> + break;
> + md_bitmap_start_sync(mddev, win_sector, &sync_blocks,
> + still_degraded);
> + set_bit(STRIPE_SYNC_REQUESTED, &sh->state);
> + set_bit(STRIPE_HANDLE, &sh->state);
[Severity: High]
Does this batch loop unnecessarily synchronize clean stripes?
The boolean return value of md_bitmap_start_sync() is ignored here. If the
first stripe needs sync but subsequent stripes in the window are clean, this
loop unconditionally queues every stripe for synchronization regardless of
its clean/dirty state, forcing read-rebuild-write cycles on clean stripes.
> + raid5_release_stripe(sh);
> + }
> +
> + return submitted * RAID5_STRIPE_SECTORS(conf);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624155452.211646-1-nishidafmly@gmail.com?part=5
^ permalink raw reply related
* Re: [PATCH 8/8] md/raid5: reserve stripe cache for user I/O during rebuild
From: sashiko-bot @ 2026-06-24 16:12 UTC (permalink / raw)
To: Hiroshi Nishida; +Cc: linux-raid, yukuai
In-Reply-To: <20260624155452.211646-9-nishidafmly@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch fails to bound the rebuild's share of the stripe cache to 50%. Breaking the read-ahead batch loop just causes the outer md_do_sync() loop to allocate stripes one-by-one, relentlessly filling the cache to 100% anyway.
--
commit 29ed9bbd7953c5d6ceeb0158cc53c81fd4d3aaf9
Author: Hiroshi Nishida <nishidafmly@gmail.com>
md/raid5: reserve stripe cache for user I/O during rebuild
This commit attempts to prevent rebuild operations from monopolizing the
stripe cache and starving concurrent user I/O. It adds yield points in the
resync read-ahead loop to stop submitting stripes if there are active
waiters or if the cache is more than half full.
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -6656,6 +6656,27 @@ static inline sector_t raid5_sync_request(struct mddev *mddev, sector_t sector_n
> submitted < RAID5_SYNC_WINDOW && win_sector < max_sector &&
> win_sector < mddev->resync_max;
> submitted++, win_sector += RAID5_STRIPE_SECTORS(conf)) {
[ ... ]
> + /*
> + * Reserve cache for user I/O only when it is actually competing.
> + * preread_active_stripes counts stripes queued for write I/O
> + * (including the read phase of RMW); sync stripes never set
> + * STRIPE_PREREAD_ACTIVE, so during a pure rebuild it stays zero
> + * and the window fills freely. Competing user reads do not bump
> + * the counter but are caught by the waitqueue_active() check
> + * above.
> + */
> + if (atomic_read(&conf->preread_active_stripes) > 0 &&
> + atomic_read(&conf->active_stripes) >=
> + conf->max_nr_stripes / RAID5_SYNC_HWMARK)
> + break;
[Severity: High]
Does breaking out of the read-ahead loop here actually limit the cache
usage to 50%?
When this breaks, raid5_sync_request() returns control to md_do_sync(),
which operates in a loop and will immediately call raid5_sync_request()
again for the next sector.
Because raid5_sync_request() unconditionally allocates its first stripe
earlier in the function before entering this batch loop, won't the outer
loop just continue allocating stripes one-by-one until the cache reaches
100% capacity and triggers the fallback blocking path anyway?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624155452.211646-1-nishidafmly@gmail.com?part=8
^ permalink raw reply
* Re: [PATCH 1/8] md: change chunk_sectors and stripe cache counts to unsigned int
From: sashiko-bot @ 2026-06-24 16:16 UTC (permalink / raw)
To: Hiroshi Nishida; +Cc: linux-raid, yukuai
In-Reply-To: <20260624155452.211646-2-nishidafmly@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [Medium] The commit's core justification is false. Changing the types to unsigned does not enable compiler right-shift optimizations for variable divisions.
- [Low] Unupdated printf-style format specifiers passing unsigned fields to '%d' cause compiler warnings (-Wformat) and break builds configured with -Werror.
Pre-existing issues:
- [Critical] The macro `round_down` is incorrectly used with a non-power-of-two divisor, causing silent data corruption in bitmap sector calculations.
--
commit 86528a8b22367a5ee16a7cb19a1d56bf58f6aa67
Author: Hiroshi Nishida <nishidafmly@gmail.com>
md: change chunk_sectors and stripe cache counts to unsigned int
This commit changes several structural fields and local variables like
chunk_sectors and max_nr_stripes from signed to unsigned int. The stated
rationale is to prevent negative values and enable right-shift compiler
optimizations for power-of-two division in the hot I/O path.
> chunk_sectors, new_chunk_sectors, prev_chunk_sectors, max_nr_stripes,
> and min_nr_stripes are never negative. Using signed int is semantically
> wrong and prevents the compiler from optimizing division/modulo by
> power-of-two chunk sizes to right shifts in the hot I/O path.
[Severity: Medium]
Is this optimization rationale accurate? The hot I/O path uses sector_div(),
which expands to do_div(). The do_div() macro explicitly casts the divisor
to a 32-bit unsigned integer, so the division was already operating on an
unsigned value.
Additionally, because sectors_per_chunk is a runtime variable rather than a
compile-time constant, would the compiler be able to optimize it into a
right shift without a conditional branch?
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index d8daf0f75cbbe..b9ad26844799d 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
[ ... ]
> @@ -437,7 +437,7 @@ struct mddev {
> int external; /* metadata is
> * managed externally */
> char metadata_type[17]; /* externally set*/
> - int chunk_sectors;
> + unsigned int chunk_sectors;
[Severity: Low]
Does changing this field to unsigned int cause compiler warnings with
existing format strings?
For example, chunk_size_show() in drivers/md/md.c appears to use %d to
format mddev->chunk_sectors:
chunk_size_show() {
...
return sprintf(page, "%d (%d)\n",
mddev->new_chunk_sectors << 9,
mddev->chunk_sectors << 9);
}
Will this break builds configured with -Werror?
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index cb5feae04db27..5cd9d0f36b6e0 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
[ ... ]
> @@ -572,12 +572,12 @@ struct r5conf {
> /* only protect corresponding hash list and inactive_list */
> spinlock_t hash_locks[NR_STRIPE_HASH_LOCKS];
> struct mddev *mddev;
> - int chunk_sectors;
> + unsigned int chunk_sectors;
> int level, algorithm, rmw_level;
> int max_degraded;
> int raid_disks;
> - int max_nr_stripes;
> - int min_nr_stripes;
> + unsigned int max_nr_stripes;
> + unsigned int min_nr_stripes;
[Severity: Low]
Will the changes to min_nr_stripes and max_nr_stripes also trigger format
warnings?
Places like raid5_end_read_request() and raid5_set_cache_size() still use
%d to print these fields. For instance, in raid5_set_cache_size():
pr_err("md/raid:%s: Cannot increase cache size, ret=%d, new_size=%d, min_nr_stripes=%d, max_nr_stripes=%d\n",
...
conf->min_nr_stripes,
conf->max_nr_stripes);
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 0c5c9fb0606ee..28828e083c2b3 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
[ ... ]
> @@ -2970,7 +2970,7 @@ sector_t raid5_compute_sector(struct r5conf *conf, sector_t r_sector,
> sector_t new_sector;
> int algorithm = previous ? conf->prev_algo
> : conf->algorithm;
> - int sectors_per_chunk = previous ? conf->prev_chunk_sectors
> + unsigned int sectors_per_chunk = previous ? conf->prev_chunk_sectors
> : conf->chunk_sectors;
[Severity: Critical]
This is a pre-existing issue, but could the use of sectors_per_chunk
cause problems when passed to rounding macros?
Looking at raid5_bitmap_sector():
raid5_bitmap_sector() {
...
sectors_per_chunk = conf->chunk_sectors *
(conf->raid_disks - conf->max_degraded);
start = round_down(start, sectors_per_chunk);
end = round_up(end, sectors_per_chunk);
...
}
The round_down() and round_up() macros are designed specifically for
power-of-two divisors. If the number of data disks is not a power of two,
sectors_per_chunk will not be a power of two, which would yield incorrect
sector boundaries.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624155452.211646-1-nishidafmly@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH 4/5] md: use sector_t for recovery_active in status_resync()
From: Hiroshi Nishida @ 2026-06-24 16:48 UTC (permalink / raw)
To: sashiko-reviews; +Cc: yukuai, linux-raid
In-Reply-To: <20260624160331.9C5F51F000E9@smtp.kernel.org>
> [Medium] mddev->recovery_active is a standard atomic_t, not "sector-typed";
> changing the local to sector_t doesn't prevent narrowing.
You're right -- mddev->recovery_active is an atomic_t and
atomic_read() returns int, so there's nothing wider to narrow; the
int->sector_t conversion just moves to the assignment.
The change is only a type-consistency tidy (the local is used solely
with the sector_t curr_mark_cnt/resync_mark_cnt), not a narrowing fix.
I'll reword the commit message in v2 accordingly and drop the
"sector-typed atomic"/"narrows" wording.
2026年6月24日(水) 9:03 <sashiko-bot@kernel.org>:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The commit message falsely claims `mddev->recovery_active` is a 'sector-typed atomic' and that declaring a local variable as `int` 'needlessly narrows it', but the structure field remains a standard 32-bit `atomic_t`.
> --
>
> commit 7ec77a6b233ce4d151f7fc723e5f312b20b4ac92
> Author: Hiroshi Nishida <nishidafmly@gmail.com>
>
> md: use sector_t for recovery_active in status_resync()
>
> This commit changes the local variable recovery_active in status_resync()
> from int to sector_t. The commit message states this is to match the type
> of mddev->recovery_active and avoid needlessly narrowing the value.
>
> > recovery_active holds a sector count read from the sector-typed atomic
> > mddev->recovery_active and is then combined with the sector_t values
> > curr_mark_cnt and resync_mark_cnt. Declaring it as a plain int needlessly
> > narrows it and mixes signedness into sector_t arithmetic; declare it
> > sector_t to match.
>
> [Severity: Medium]
> Is this description accurate? Looking at struct mddev in drivers/md/md.h,
> recovery_active is defined as a standard atomic_t:
>
> drivers/md/md.h:struct mddev {
> ...
> atomic_t recovery_active;
> ...
> }
>
> Since atomic_t is a 32-bit signed integer, and atomic_read() returns an int,
> does changing this local variable to sector_t actually prevent narrowing? The
> implicit cast from int to sector_t will now simply happen at assignment
> rather than during the arithmetic operations.
>
> Could the commit message be updated to reflect that mddev->recovery_active
> is a standard atomic_t, rather than a "sector-typed atomic"?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260624155421.211626-1-nishidafmly@gmail.com?part=4
--
Hiroshi Nishida
nishidafmly@gmail.com
^ permalink raw reply
* Re: [PATCH 5/5] md: clarify the resync ETA comment in status_resync()
From: Hiroshi Nishida @ 2026-06-24 16:50 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-raid, yukuai
In-Reply-To: <20260624155828.3C6F91F000E9@smtp.kernel.org>
> The comment states that the divisor db is scaled up by 32, but the code
> rt = div64_u64(rt, db/32+1); divides db by 32 -- scaling it down.
You're right -- the code divides by db/32, which scales the divisor
*down* by 32 (so the intermediate quotient is 32x larger and keeps
low-order bits a plain divide by db would lose), and rt is then
rescaled by the >> 5.
My rewritten comment had the direction backwards. I'll fix it in v2:
rt is, in exact arithmetic, (remaining sectors) * dt / db. To keep
precision near the end of resync, when the remaining count is close to db,
the dividend is divided by db/32 rather than db -- i.e. the divisor is
scaled down by 32, so the intermediate quotient is 32x larger -- and rt is
scaled back down by 32 (>> 5) afterwards.
Thanks,
2026年6月24日(水) 8:58 <sashiko-bot@kernel.org>:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] The newly added comment incorrectly states that the divisor `db` is scaled up by 32, when the code actually scales it down.
> --
>
> commit 21672ecf7d2b823245c32b9d2699f7cf1a5c2e7c
> Author: Hiroshi Nishida <nishidafmly@gmail.com>
>
> md: clarify the resync ETA comment in status_resync()
>
> This commit updates the block comment above the remaining-time computation
> in status_resync() to clarify the calculation logic. It is a comment-only
> change.
>
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 0cd85cc92ed31..a5c0da0d11334 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -8916,17 +8916,11 @@ static int status_resync(struct seq_file *seq, struct mddev *mddev)
> > * db: blocks written from mark until now
> > * rt: remaining time
> > *
> > - * rt is a sector_t, which is always 64bit now. We are keeping
> > - * the original algorithm, but it is not really necessary.
> > - *
> > - * Original algorithm:
> > - * So we divide before multiply in case it is 32bit and close
> > - * to the limit.
> > - * We scale the divisor (db) by 32 to avoid losing precision
> > - * near the end of resync when the number of remaining sectors
> > - * is close to 'db'.
> > - * We then divide rt by 32 after multiplying by db to compensate.
> > - * The '+1' avoids division by zero if db is very small.
> > + * rt is computed as (remaining sectors) * dt / db. To keep precision
> > + * near the end of resync, when the remaining count is close to db, the
> > + * divisor db is scaled up by 32 before the divide and rt is scaled back
>
> [Severity: Low]
> Is this new comment accurate regarding the math applied to db?
>
> The comment states that the divisor db is scaled up by 32, but looking at
> the actual code:
>
> rt = max_sectors - resync;
> rt = div64_u64(rt, db/32+1);
>
> This divides db by 32, effectively scaling it down rather than up (which in
> turn scales the resulting quotient up). Does this comment need to be
> adjusted to reflect the db/32 downscaling?
>
> > + * down by 32 afterwards. The '+1' avoids division by zero when db is
> > + * very small.
> > */
> > dt = ((jiffies - mddev->resync_mark) / HZ);
> > if (!dt) dt++;
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260624155421.211626-1-nishidafmly@gmail.com?part=5
--
Hiroshi Nishida
nishidafmly@gmail.com
^ permalink raw reply
* Re: [PATCH 6/8] md/raid5: allocate worker groups per NUMA node
From: Hiroshi Nishida @ 2026-06-24 16:53 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-raid, yukuai
In-Reply-To: <20260624160726.2B9161F000E9@smtp.kernel.org>
> [High] kzalloc_node(size, GFP_NOIO, i) -- i is a sequential index, not a node ID
> [High, pre-existing] worker_groups sized by num_possible_nodes() but indexed
> by node ID -> OOB on sparse NUMA
Both are valid.
The kzalloc_node(..., i) is wrong on sparse NUMA node IDs -- i runs
0..num_possible_nodes()-1, which only equals the node ID when nodes
are dense -- and it rides on the pre-existing assumption that the
worker_groups array (sized by the node *count*) can be indexed by
cpu_to_node().
The NUMA-locality win here is multi-socket-only and I don't have a
measurement to justify it, so rather than grow this into a sparse-NUMA
correctness fix I'll
drop this patch from v2.
The pre-existing worker_groups sizing/indexing issue is real and
separate; thanks for catching it.
Thanks,
2026年6月24日(水) 9:07 <sashiko-bot@kernel.org>:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] The patch incorrectly uses a sequential loop index `i` as the NUMA node ID parameter to `kzalloc_node()`.
>
> Pre-existing issues:
> - [High] The `worker_groups` array is sized using `num_possible_nodes()` but is indexed by the raw NUMA node ID, leading to out-of-bounds accesses on systems with sparse NUMA node IDs.
> --
>
> commit bce08bf6ce446b00da625ff4028f5782b756e17c
> Author: Hiroshi Nishida <nishidafmly@gmail.com>
>
> md/raid5: allocate worker groups per NUMA node
>
> alloc_thread_groups() previously allocated all r5worker arrays in a
> single kcalloc() block, assigning workers for NUMA node N from node 0
> memory. On multi-socket systems this causes remote memory traffic on
> every worker->work and worker->temp_inactive_list access.
>
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index 8e9edaaca667b..c8787ab7b309c 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> [ ... ]
> > @@ -7336,7 +7340,6 @@ static int alloc_thread_groups(struct r5conf *conf, int cnt, int *group_cnt,
> [ ... ]
> > *group_cnt = num_possible_nodes();
> > - size = sizeof(struct r5worker) * cnt;
> > - workers = kcalloc(size, *group_cnt, GFP_NOIO);
> > *worker_groups = kzalloc_objs(struct r5worker_group, *group_cnt,
> > GFP_NOIO);
>
> [Severity: High]
> This isn't a bug introduced by this patch, but could this array sizing cause
> an out-of-bounds access on systems with sparse NUMA nodes?
>
> We size the worker_groups array using num_possible_nodes(), which is a count.
> On systems with sparse NUMA node IDs (for example, nodes 0 and 8), the array
> would only have 2 elements. Later, in raid5_wakeup_stripe_thread(), we index
> into this array using cpu_to_group(cpu) which expands to cpu_to_node(cpu).
> If a CPU belongs to node 8, we would access conf->worker_groups[8], exceeding
> the allocated size and corrupting memory.
>
> > - if (!*worker_groups || !workers) {
> > - kfree(workers);
> > - kfree(*worker_groups);
> > + if (!*worker_groups)
> > return -ENOMEM;
> > - }
> >
> > + size = sizeof(struct r5worker) * cnt;
> > for (i = 0; i < *group_cnt; i++) {
> > - struct r5worker_group *group;
> > + struct r5worker_group *group = &(*worker_groups)[i];
> > + struct r5worker *workers;
> > +
> > + workers = kzalloc_node(size, GFP_NOIO, i);
>
> [Severity: High]
> Could passing the sequential loop index i directly to kzalloc_node cause a
> NULL pointer dereference on systems with sparse NUMA nodes?
>
> Since num_possible_nodes returns a count rather than an upper bound on IDs,
> on a system with nodes 0 and 8, i would be evaluated as 1. Passing an
> unpopulated node ID to kzalloc_node without validation can lead to a crash
> when accessing NODE_DATA(nid). Should this loop use
> for_each_node_state(nid, N_MEMORY) or similar to get the actual node ID
> instead of using the loop counter?
>
> > + if (!workers)
> > + goto out_free;
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260624155452.211646-1-nishidafmly@gmail.com?part=6
--
Hiroshi Nishida
nishidafmly@gmail.com
^ permalink raw reply
* Re: [PATCH 7/8] md/raid5: raise MAX_STRIPE_BATCH from 8 to 32
From: Hiroshi Nishida @ 2026-06-24 17:01 UTC (permalink / raw)
To: sashiko-reviews; +Cc: yukuai, linux-raid
In-Reply-To: <20260624160917.1508E1F000E9@smtp.kernel.org>
> [High] Raising MAX_STRIPE_BATCH to 32 while keeping the spawn threshold at 8
> lets the first worker grab all 32 stripes and starves the others.
You're right. In stock, MAX_STRIPE_BATCH does double duty -- it caps
the greedy dequeue
in handle_active_stripes() AND is the spawn divisor in
raid5_wakeup_stripe_thread() --
and keeping those equal is what balances the workers woken against the
work each one takes.
My patch grew the dequeue cap to 32 but left the spawn divisor at 8,
so for ~32 pending stripes it wakes 4 workerswhile the first greedily
takes all 32 and
the rest find empty lists -- serializing the batch and adding
pointless lock contention, exactly as you describe.
The lock-amortization benefit was never isolated, and re-coupling the two (spawn
divisor = batch) just trades it for coarser worker granularity, so
I'll drop this
patch from v2 rather than guess at the tuning.
Thanks,
2026年6月24日(水) 9:09 <sashiko-bot@kernel.org>:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Increasing MAX_STRIPE_BATCH to 32 without proportionally increasing STRIPE_BATCH_WORKERS serializes stripe processing and causes worker starvation.
> --
>
> commit b1a0a169dabd8b46b6b9ca40375034cf879934c6
> Author: Hiroshi Nishida <nishidafmly@gmail.com>
>
> md/raid5: raise MAX_STRIPE_BATCH from 8 to 32
>
> This commit increases the maximum stripe batch size from 8 to 32 to
> amortize device_lock acquisition over more stripes. It also introduces
> STRIPE_BATCH_WORKERS to decouple the worker spawning threshold from the
> batch size.
>
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index c8787ab7b309c..ad6230415af33 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> [ ... ]
> > @@ -210,7 +210,7 @@ static void raid5_wakeup_stripe_thread(struct stripe_head *sh)
> > /* at least one worker should run to avoid race */
> > queue_work_on(sh->cpu, raid5_wq, &group->workers[0].work);
> >
> > - thread_cnt = group->stripes_cnt / MAX_STRIPE_BATCH - 1;
> > + thread_cnt = group->stripes_cnt / STRIPE_BATCH_WORKERS - 1;
> > /* wakeup more workers */
> > for (i = 1; i < conf->worker_cnt_per_group && thread_cnt > 0; i++) {
>
> [Severity: High]
> Will increasing the stripe batch size while keeping the worker spawn threshold
> small serialize stripe processing and starve the worker threads?
>
> With this change, raid5_wakeup_stripe_thread() still wakes up an additional
> worker for every 8 pending stripes (e.g., waking 4 threads for 32 stripes).
>
> However, the first awakened worker executing handle_active_stripes() will
> greedily dequeue up to MAX_STRIPE_BATCH (32) stripes under the lock:
>
> drivers/md/raid5.c:handle_active_stripes() {
> ...
> while (batch_size < MAX_STRIPE_BATCH &&
> (sh = __get_priority_stripe(conf, group)) != NULL)
> batch[batch_size++] = sh;
> ...
> }
>
> It appears the first thread will steal all 32 pending stripes and process
> them sequentially, while the other awakened threads acquire the lock,
> find the lists empty, and exit without doing any work.
>
> Could this cause a performance regression by serializing CPU-intensive parity
> operations onto a single thread, while also creating unnecessary lock
> contention from the starved threads?
>
> > diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> > index 7aeba1fc7f09e..1f37dabd727be 100644
> > --- a/drivers/md/raid5.h
> > +++ b/drivers/md/raid5.h
> [ ... ]
> > @@ -496,7 +496,8 @@ struct disk_info {
> > #define BYPASS_THRESHOLD 1
> > #define NR_HASH (PAGE_SIZE / sizeof(struct hlist_head))
> > #define HASH_MASK (NR_HASH - 1)
> > -#define MAX_STRIPE_BATCH 8
> > +#define MAX_STRIPE_BATCH 32 /* stripes per handle_active_stripes pass */
> > +#define STRIPE_BATCH_WORKERS 8 /* stripes-per-worker threshold for spawning */
> > #define RAID5_SYNC_WINDOW 32 /* stripes to pre-submit per sync_request call */
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260624155452.211646-1-nishidafmly@gmail.com?part=7
--
Hiroshi Nishida
nishidafmly@gmail.com
^ permalink raw reply
* Re: [PATCH 5/8] md/raid5: submit a window of stripes during resync/recovery
From: Hiroshi Nishida @ 2026-06-24 17:13 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-raid, yukuai
In-Reply-To: <20260624161238.6813B1F000E9@smtp.kernel.org>
> [High] TOCTOU on still_degraded: the degraded check was moved before the
> blocking stripe get + schedule_timeout, so a drive failing during the sleep
> leaves a stale still_degraded that can wrongly clear the NEEDED bit.
Agreed -- this is a real regression I introduced.
I hoisted the still_degraded computation so the window could reuse it,
but that placed it before the blocking get and the
schedule_timeout_uninterruptible(1),
whereas stock computes it after.
In v2 I'll move the still_degraded computation back below the blocking
get of the first stripe (restoring stock's ordering).
The remaining window stripes are fetched with R5_GAS_NOBLOCK and don't
sleep, so they can reuse that
value without reopening the race across a sleep.
> [High] The batch loop ignores md_bitmap_start_sync()'s return and
force-syncs
> clean stripes in the window.
Also correct.
Stock makes the skip decision once at the top
(!md_bitmap_start_sync(..., true) && sync_blocks >= ...),
but the window loop doesn't, so a clean bitmap region the window
crosses gets synced unnecessarily.
In v2 I'll have the window honor the bitmap and stop at the boundary of the
dirty region being resynced, rather than extending into already-in-sync stripes.
Both are correctness details the per-stripe path got right and my
batching dropped;
I'll fold the fixes into v2 and re-test the bitmap resync path.
Thanks,
2026年6月24日(水) 9:12 <sashiko-bot@kernel.org>:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The batch loop ignores the return value of md_bitmap_start_sync(), forcing clean stripes to be unnecessarily synchronized.
> - [High] Time-of-check to time-of-use race on the array's degraded state, leading to silent data corruption via incorrect bitmap updates.
> --
>
> commit 01d1e80ee6aa253c48c233e7fc218fbdac9522fa
> Author: Hiroshi Nishida <nishidafmly@gmail.com>
>
> md/raid5: submit a window of stripes during resync/recovery
>
> This commit alters raid5_sync_request() to dispatch up to 32 stripes per
> call instead of one. It attempts to submit the remaining stripes in the
> window with a non-blocking flag to avoid the per-stripe throttle sleep.
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 9cb4ed3bd85c5..8e9edaaca667b 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -6620,16 +6621,7 @@ static inline sector_t raid5_sync_request(...)
> [ ... ]
> > - sh = raid5_get_active_stripe(conf, NULL, sector_nr,
> > - R5_GAS_NOBLOCK);
> > - if (sh == NULL) {
> > - sh = raid5_get_active_stripe(conf, NULL, sector_nr, 0);
> > - /* make sure we don't swamp the stripe cache if someone else
> > - * is trying to get access
> > - */
> > - schedule_timeout_uninterruptible(1);
> > - }
> > - /* Need to check if array will still be degraded after recovery/resync
> > + /* Check once whether array will still be degraded after recovery/resync.
> > * Note in case of > 1 drive failures it's possible we're rebuilding
> > * one drive while leaving another faulty drive in array.
> > */
> > @@ -6640,13 +6632,42 @@ static inline sector_t raid5_sync_request(...)
> > still_degraded = true;
> > }
> >
> > + /* First stripe: block if stripe cache is full, then throttle. */
> > + sh = raid5_get_active_stripe(conf, NULL, sector_nr, R5_GAS_NOBLOCK);
> > + if (sh == NULL) {
> > + sh = raid5_get_active_stripe(conf, NULL, sector_nr, 0);
> > + /* make sure we don't swamp the stripe cache if someone else
> > + * is trying to get access
> > + */
> > + schedule_timeout_uninterruptible(1);
> > + }
>
> [Severity: High]
> Does moving the degraded check before the stripe allocation introduce a
> time-of-check to time-of-use race?
>
> If a drive fails during the potential sleep in raid5_get_active_stripe() or
> schedule_timeout_uninterruptible(), the stale still_degraded value is used
> below to update the bitmap. This could incorrectly clear the NEEDED bit,
> potentially leaving parity inconsistent if the system crashes before the sync
> completes.
>
> > md_bitmap_start_sync(mddev, sector_nr, &sync_blocks, still_degraded);
> > set_bit(STRIPE_SYNC_REQUESTED, &sh->state);
> > set_bit(STRIPE_HANDLE, &sh->state);
>
> [ ... ]
>
> > + win_sector = sector_nr + RAID5_STRIPE_SECTORS(conf);
> > + for (submitted = 1;
> > + submitted < RAID5_SYNC_WINDOW && win_sector < max_sector &&
> > + win_sector < mddev->resync_max;
> > + submitted++, win_sector += RAID5_STRIPE_SECTORS(conf)) {
> > + sh = raid5_get_active_stripe(conf, NULL, win_sector,
> > + R5_GAS_NOBLOCK);
> > + if (!sh)
> > + break;
> > + md_bitmap_start_sync(mddev, win_sector, &sync_blocks,
> > + still_degraded);
> > + set_bit(STRIPE_SYNC_REQUESTED, &sh->state);
> > + set_bit(STRIPE_HANDLE, &sh->state);
>
> [Severity: High]
> Does this batch loop unnecessarily synchronize clean stripes?
>
> The boolean return value of md_bitmap_start_sync() is ignored here. If the
> first stripe needs sync but subsequent stripes in the window are clean, this
> loop unconditionally queues every stripe for synchronization regardless of
> its clean/dirty state, forcing read-rebuild-write cycles on clean stripes.
>
> > + raid5_release_stripe(sh);
> > + }
> > +
> > + return submitted * RAID5_STRIPE_SECTORS(conf);
> > }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260624155452.211646-1-nishidafmly@gmail.com?part=5
--
Hiroshi Nishida
nishidafmly@gmail.com
^ permalink raw reply
* Re: [PATCH 1/8] md: change chunk_sectors and stripe cache counts to unsigned int
From: Hiroshi Nishida @ 2026-06-24 17:25 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-raid, yukuai
In-Reply-To: <20260624161624.12CC81F000E9@smtp.kernel.org>
> [Medium] unsigned types don't enable right-shift optimization for variable divisions.
You're right -- the hot-path divisions go through
sector_div()/do_div() (already u32),
and sectors_per_chunk is a runtime value, so signedness changes no codegen.
The rationale is wrong and the change has no perf benefit, so I'll
drop this patch
from v2 rather than reword a claim that doesn't hold.
> [Low] unupdated %d specifiers for the now-unsigned fields.
Another reason to drop it -- avoids the %d/%u churn. (They don't warn
in a default
build, only under -Wformat-signedness, but they are inconsistent.)
> [Critical, pre-existing] round_down/round_up with a non-power-of-two divisor.
Good catch, and independent of this series.
sectors_per_chunk = chunk_sectors * (raid_disks - max_degraded)
isn't a power of two whenever the data-disk count isn't (raid5 4-disk
-> 3 data),
and round_down/round_up are mask-based.
I'll send a separate fix switching to rounddown()/roundup() with a Fixes: tag.
Thanks,
2026年6月24日(水) 9:16 <sashiko-bot@kernel.org>:
>
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [Medium] The commit's core justification is false. Changing the types to unsigned does not enable compiler right-shift optimizations for variable divisions.
> - [Low] Unupdated printf-style format specifiers passing unsigned fields to '%d' cause compiler warnings (-Wformat) and break builds configured with -Werror.
>
> Pre-existing issues:
> - [Critical] The macro `round_down` is incorrectly used with a non-power-of-two divisor, causing silent data corruption in bitmap sector calculations.
> --
>
> commit 86528a8b22367a5ee16a7cb19a1d56bf58f6aa67
> Author: Hiroshi Nishida <nishidafmly@gmail.com>
>
> md: change chunk_sectors and stripe cache counts to unsigned int
>
> This commit changes several structural fields and local variables like
> chunk_sectors and max_nr_stripes from signed to unsigned int. The stated
> rationale is to prevent negative values and enable right-shift compiler
> optimizations for power-of-two division in the hot I/O path.
>
> > chunk_sectors, new_chunk_sectors, prev_chunk_sectors, max_nr_stripes,
> > and min_nr_stripes are never negative. Using signed int is semantically
> > wrong and prevents the compiler from optimizing division/modulo by
> > power-of-two chunk sizes to right shifts in the hot I/O path.
>
> [Severity: Medium]
> Is this optimization rationale accurate? The hot I/O path uses sector_div(),
> which expands to do_div(). The do_div() macro explicitly casts the divisor
> to a 32-bit unsigned integer, so the division was already operating on an
> unsigned value.
>
> Additionally, because sectors_per_chunk is a runtime variable rather than a
> compile-time constant, would the compiler be able to optimize it into a
> right shift without a conditional branch?
>
> > diff --git a/drivers/md/md.h b/drivers/md/md.h
> > index d8daf0f75cbbe..b9ad26844799d 100644
> > --- a/drivers/md/md.h
> > +++ b/drivers/md/md.h
> [ ... ]
> > @@ -437,7 +437,7 @@ struct mddev {
> > int external; /* metadata is
> > * managed externally */
> > char metadata_type[17]; /* externally set*/
> > - int chunk_sectors;
> > + unsigned int chunk_sectors;
>
> [Severity: Low]
> Does changing this field to unsigned int cause compiler warnings with
> existing format strings?
>
> For example, chunk_size_show() in drivers/md/md.c appears to use %d to
> format mddev->chunk_sectors:
>
> chunk_size_show() {
> ...
> return sprintf(page, "%d (%d)\n",
> mddev->new_chunk_sectors << 9,
> mddev->chunk_sectors << 9);
> }
>
> Will this break builds configured with -Werror?
>
> > diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> > index cb5feae04db27..5cd9d0f36b6e0 100644
> > --- a/drivers/md/raid5.h
> > +++ b/drivers/md/raid5.h
> [ ... ]
> > @@ -572,12 +572,12 @@ struct r5conf {
> > /* only protect corresponding hash list and inactive_list */
> > spinlock_t hash_locks[NR_STRIPE_HASH_LOCKS];
> > struct mddev *mddev;
> > - int chunk_sectors;
> > + unsigned int chunk_sectors;
> > int level, algorithm, rmw_level;
> > int max_degraded;
> > int raid_disks;
> > - int max_nr_stripes;
> > - int min_nr_stripes;
> > + unsigned int max_nr_stripes;
> > + unsigned int min_nr_stripes;
>
> [Severity: Low]
> Will the changes to min_nr_stripes and max_nr_stripes also trigger format
> warnings?
>
> Places like raid5_end_read_request() and raid5_set_cache_size() still use
> %d to print these fields. For instance, in raid5_set_cache_size():
>
> pr_err("md/raid:%s: Cannot increase cache size, ret=%d, new_size=%d, min_nr_stripes=%d, max_nr_stripes=%d\n",
> ...
> conf->min_nr_stripes,
> conf->max_nr_stripes);
>
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index 0c5c9fb0606ee..28828e083c2b3 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> [ ... ]
> > @@ -2970,7 +2970,7 @@ sector_t raid5_compute_sector(struct r5conf *conf, sector_t r_sector,
> > sector_t new_sector;
> > int algorithm = previous ? conf->prev_algo
> > : conf->algorithm;
> > - int sectors_per_chunk = previous ? conf->prev_chunk_sectors
> > + unsigned int sectors_per_chunk = previous ? conf->prev_chunk_sectors
> > : conf->chunk_sectors;
>
> [Severity: Critical]
> This is a pre-existing issue, but could the use of sectors_per_chunk
> cause problems when passed to rounding macros?
>
> Looking at raid5_bitmap_sector():
>
> raid5_bitmap_sector() {
> ...
> sectors_per_chunk = conf->chunk_sectors *
> (conf->raid_disks - conf->max_degraded);
> start = round_down(start, sectors_per_chunk);
> end = round_up(end, sectors_per_chunk);
> ...
> }
>
> The round_down() and round_up() macros are designed specifically for
> power-of-two divisors. If the number of data disks is not a power of two,
> sectors_per_chunk will not be a power of two, which would yield incorrect
> sector boundaries.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260624155452.211646-1-nishidafmly@gmail.com?part=1
--
Hiroshi Nishida
nishidafmly@gmail.com
^ permalink raw reply
* Re: [PATCH 8/8] md/raid5: reserve stripe cache for user I/O during rebuild
From: Hiroshi Nishida @ 2026-06-24 17:25 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-raid, yukuai
In-Reply-To: <20260624161245.292821F000E9@smtp.kernel.org>
> [High] Breaking the read-ahead loop doesn't bound the rebuild to 50% -- the
> outer md_do_sync() loop just calls raid5_sync_request() again and the
> unconditional first-stripe allocation keeps filling the cache to 100%.
You're right, and it defeats the patch's purpose.
The 50% check only gates the window batch; the first stripe of each
raid5_sync_request() call is allocated
unconditionally before the loop, so md_do_sync() keeps driving one
stripe per call past 50%
until the cache is full and the existing blocking get throttles at ~100% anyway.
The "reserve half the cache for user I/O" claim doesn't hold.
Making it actually bound the rebuild would mean gating the
first-stripe allocation and
having raid5_sync_request() yield without making progress --
effectively reworking the sync throttle, which is more than this
change should take on,
and the user-latency benefit was never something I could measure
cleanly.
So I'll drop this patch from v2. (It's coupled to the read-ahead
window patch, which I'll keep and fix per the other thread.)
Thanks,
2026年6月24日(水) 9:12 <sashiko-bot@kernel.org>:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The patch fails to bound the rebuild's share of the stripe cache to 50%. Breaking the read-ahead batch loop just causes the outer md_do_sync() loop to allocate stripes one-by-one, relentlessly filling the cache to 100% anyway.
> --
>
> commit 29ed9bbd7953c5d6ceeb0158cc53c81fd4d3aaf9
> Author: Hiroshi Nishida <nishidafmly@gmail.com>
>
> md/raid5: reserve stripe cache for user I/O during rebuild
>
> This commit attempts to prevent rebuild operations from monopolizing the
> stripe cache and starving concurrent user I/O. It adds yield points in the
> resync read-ahead loop to stop submitting stripes if there are active
> waiters or if the cache is more than half full.
>
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> > @@ -6656,6 +6656,27 @@ static inline sector_t raid5_sync_request(struct mddev *mddev, sector_t sector_n
> > submitted < RAID5_SYNC_WINDOW && win_sector < max_sector &&
> > win_sector < mddev->resync_max;
> > submitted++, win_sector += RAID5_STRIPE_SECTORS(conf)) {
>
> [ ... ]
>
> > + /*
> > + * Reserve cache for user I/O only when it is actually competing.
> > + * preread_active_stripes counts stripes queued for write I/O
> > + * (including the read phase of RMW); sync stripes never set
> > + * STRIPE_PREREAD_ACTIVE, so during a pure rebuild it stays zero
> > + * and the window fills freely. Competing user reads do not bump
> > + * the counter but are caught by the waitqueue_active() check
> > + * above.
> > + */
> > + if (atomic_read(&conf->preread_active_stripes) > 0 &&
> > + atomic_read(&conf->active_stripes) >=
> > + conf->max_nr_stripes / RAID5_SYNC_HWMARK)
> > + break;
>
> [Severity: High]
> Does breaking out of the read-ahead loop here actually limit the cache
> usage to 50%?
>
> When this breaks, raid5_sync_request() returns control to md_do_sync(),
> which operates in a loop and will immediately call raid5_sync_request()
> again for the next sector.
>
> Because raid5_sync_request() unconditionally allocates its first stripe
> earlier in the function before entering this batch loop, won't the outer
> loop just continue allocating stripes one-by-one until the cache reaches
> 100% capacity and triggers the fallback blocking path anyway?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260624155452.211646-1-nishidafmly@gmail.com?part=8
--
Hiroshi Nishida
nishidafmly@gmail.com
^ permalink raw reply
* Re: [PATCH] raid6: fix riscv symbol undeclared warnigns
From: Paul Walmsley @ 2026-06-25 17:50 UTC (permalink / raw)
To: Ben Dooks
Cc: linux-raid, Song Liu, Yu Kuai, linux-riscv, linux-kernel,
Paul Walmsley, Palmer Dabbelt
In-Reply-To: <20260622135207.480540-1-ben.dooks@codethink.co.uk>
On Mon, 22 Jun 2026, Ben Dooks wrote:
> The riscv rvv.c file is missing the include of pq_arch.h
> which defines all the exported functions. Include this
> to remove the following sparse warnings:
>
> lib/raid/raid6/riscv/rvv.c:1225:1: warning: symbol 'raid6_rvvx1' was not declared. Should it be static?
> lib/raid/raid6/riscv/rvv.c:1226:1: warning: symbol 'raid6_rvvx2' was not declared. Should it be static?
> lib/raid/raid6/riscv/rvv.c:1227:1: warning: symbol 'raid6_rvvx4' was not declared. Should it be static?
> lib/raid/raid6/riscv/rvv.c:1228:1: warning: symbol 'raid6_rvvx8' was not declared. Should it be static?
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Thanks, queued for v7.2-rc.
- Paul
^ permalink raw reply
* Re: [PATCH] raid6: fix raid6_recov_rvv symbol undeclared warning
From: Paul Walmsley @ 2026-06-25 17:52 UTC (permalink / raw)
To: Ben Dooks
Cc: linux-raid, Song Liu, Yu Kuai, linux-riscv, linux-kernel,
Paul Walmsley, Palmer Dabbelt
In-Reply-To: <20260622135535.481534-1-ben.dooks@codethink.co.uk>
On Mon, 22 Jun 2026, Ben Dooks wrote:
> The riscv recov_rvv.c should have included pq_arch.h for
> the definition of raid6_recov_rvv. Add the include to
> fix the following sparse warning:
>
> lib/raid/raid6/riscv/recov_rvv.c:218:32: warning: symbol 'raid6_recov_rvv' was not declared. Should it be static?
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Thanks, queued for v7.2-rc.
- Paul
^ permalink raw reply
* [PATCH 0/8] x86: Remove cpu_has_xfeatures() and add AVX-512 xor_gen()
From: Eric Biggers @ 2026-06-26 4:37 UTC (permalink / raw)
To: x86
Cc: linux-um, linux-raid, linux-crypto, linux-kernel,
Christoph Hellwig, Andrew Morton, Eric Biggers
My patch "lib/raid/xor: x86: Add AVX-512 optimized xor_gen()"
(https://lore.kernel.org/r/20260615190338.26581-1-ebiggers@kernel.org/)
still seems to be blocked on a Sashiko comment about cpu_has_xfeatures()
not being called.
However, the x86-optimized RAID library code supports UML, and currently
UML doesn't implement cpu_has_xfeatures(). That's perhaps why the
existing AVX-512 optimized RAID6 code doesn't check it either.
In fact, it seems to have been getting by fine without it, which
suggests that it's not truly needed.
But to eliminate any doubts, I've had a go at fully resolving the
situation by making both native x86 and UML explicitly clear any
X86_FEATURE_* flags at boot time whose xfeatures are missing.
Then, cpu_has_xfeatures() is entirely removed from the kernel.
The last patch adds the AVX-512 optimized xor_gen(). I do still think
it would be fine to proceed with it without the rest. But if there are
any doubts, we can take this more comprehensive cleanup route.
Eric Biggers (8):
x86/fpu: Check for missing AVX and AVX-512 xstate bits
um: Check for missing AVX and AVX-512 xstate bits
crypto: x86 - Stop using cpu_has_xfeatures()
lib/crypto: x86: Stop using cpu_has_xfeatures()
lib/crc: x86: Stop using cpu_has_xfeatures()
x86/fpu: Remove cpu_has_xfeatures()
lib/raid/xor: x86: Remove redundant X86_FEATURE_OSXSAVE check
lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
arch/um/kernel/um_arch.c | 78 ++++++++++++-
arch/x86/crypto/aegis128-aesni-glue.c | 3 +-
arch/x86/crypto/aesni-intel_glue.c | 7 +-
arch/x86/crypto/aria_aesni_avx2_glue.c | 11 +-
arch/x86/crypto/aria_aesni_avx_glue.c | 11 +-
arch/x86/crypto/aria_gfni_avx512_glue.c | 11 +-
arch/x86/crypto/camellia_aesni_avx2_glue.c | 11 +-
arch/x86/crypto/camellia_aesni_avx_glue.c | 11 +-
arch/x86/crypto/cast5_avx_glue.c | 7 +-
arch/x86/crypto/cast6_avx_glue.c | 7 +-
arch/x86/crypto/serpent_avx2_glue.c | 9 +-
arch/x86/crypto/serpent_avx_glue.c | 7 +-
arch/x86/crypto/sm4_aesni_avx2_glue.c | 11 +-
arch/x86/crypto/sm4_aesni_avx_glue.c | 11 +-
arch/x86/crypto/twofish_avx_glue.c | 6 +-
arch/x86/include/asm/fpu/api.h | 9 --
arch/x86/kernel/fpu/xstate.c | 63 ++++-------
lib/crc/x86/crc-pclmul-template.h | 6 +-
lib/crypto/x86/blake2s.h | 4 +-
lib/crypto/x86/chacha.h | 3 +-
lib/crypto/x86/nh.h | 4 +-
lib/crypto/x86/poly1305.h | 7 +-
lib/crypto/x86/sha1.h | 4 +-
lib/crypto/x86/sha256.h | 4 +-
lib/crypto/x86/sha512.h | 3 +-
lib/crypto/x86/sm3.h | 3 +-
lib/raid/xor/Makefile | 2 +-
lib/raid/xor/x86/xor-avx512.c | 121 +++++++++++++++++++++
lib/raid/xor/x86/xor_arch.h | 24 ++--
29 files changed, 264 insertions(+), 194 deletions(-)
create mode 100644 lib/raid/xor/x86/xor-avx512.c
base-commit: 4edcdefd4083ae04b1a5656f4be6cd83ae919ef4
--
2.54.0
^ permalink raw reply
* [PATCH 2/8] um: Check for missing AVX and AVX-512 xstate bits
From: Eric Biggers @ 2026-06-26 4:37 UTC (permalink / raw)
To: x86
Cc: linux-um, linux-raid, linux-crypto, linux-kernel,
Christoph Hellwig, Andrew Morton, Eric Biggers
In-Reply-To: <20260626043731.319287-1-ebiggers@kernel.org>
If the CPU declares AVX or AVX-512 support, verify that all the
corresponding xstate bits are also set. If any are missing, warn and
don't set the corresponding X86_FEATURE_* flags.
This eliminates the perceived need for UML-supporting AVX and AVX-512
optimized code in the kernel (that is, lib/raid/ currently) to start
checking the xstate bits in addition to X86_FEATURE_AVX*.
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
arch/um/kernel/um_arch.c | 78 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 77 insertions(+), 1 deletion(-)
diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c
index 2141f5f1f5a2..aafbaef2ae82 100644
--- a/arch/um/kernel/um_arch.c
+++ b/arch/um/kernel/um_arch.c
@@ -262,16 +262,92 @@ EXPORT_SYMBOL(task_size);
unsigned long brk_start;
#define MIN_VMALLOC (32 * 1024 * 1024)
+static u64 __init read_xcr0(void)
+{
+ u32 a, b, c, d;
+
+ asm volatile("cpuid"
+ : "=a"(a), "=b"(b), "=c"(c), "=d"(d)
+ : "a"(0), "c"(0));
+ if (a >= 1) { /* max_leaf >= 1 */
+ asm volatile("cpuid"
+ : "=a"(a), "=b"(b), "=c"(c), "=d"(d)
+ : "a"(1), "c"(0));
+ if (c & (1 << 27)) { /* XSAVE enabled by OS */
+ asm volatile("xgetbv" : "=d"(d), "=a"(a) : "c"(0));
+ return ((u64)d << 32) | a;
+ }
+ }
+ return 0;
+}
+
+static void __init validate_and_set_cpu_cap(int cap, u64 xcr0)
+{
+ /*
+ * Check for missing xstate features right away, so that there's no
+ * perceived need for all optimized code in the kernel to do so.
+ */
+ switch (cap) {
+ case X86_FEATURE_AVX:
+ case X86_FEATURE_AVX2:
+ case X86_FEATURE_AVX_VNNI:
+ case X86_FEATURE_FMA:
+ case X86_FEATURE_VAES:
+ case X86_FEATURE_VPCLMULQDQ:
+ if ((xcr0 & 0x7) != 0x7) {
+ static bool warned;
+
+ if (!warned) {
+ os_warn("Disabling AVX support due to missing xstate features\n");
+ warned = true;
+ }
+ return;
+ }
+ break;
+ case X86_FEATURE_AVX512F:
+ case X86_FEATURE_AVX512BW:
+ case X86_FEATURE_AVX512CD:
+ case X86_FEATURE_AVX512DQ:
+ case X86_FEATURE_AVX512ER:
+ case X86_FEATURE_AVX512IFMA:
+ case X86_FEATURE_AVX512PF:
+ case X86_FEATURE_AVX512VBMI:
+ case X86_FEATURE_AVX512VL:
+ case X86_FEATURE_AVX512_4FMAPS:
+ case X86_FEATURE_AVX512_4VNNIW:
+ case X86_FEATURE_AVX512_BF16:
+ case X86_FEATURE_AVX512_BITALG:
+ case X86_FEATURE_AVX512_FP16:
+ case X86_FEATURE_AVX512_VBMI2:
+ case X86_FEATURE_AVX512_VNNI:
+ case X86_FEATURE_AVX512_VP2INTERSECT:
+ case X86_FEATURE_AVX512_VPOPCNTDQ:
+ if ((xcr0 & 0xe7) != 0xe7) {
+ static bool warned;
+
+ if (!warned) {
+ os_warn("Disabling AVX-512 support due to missing xstate features\n");
+ warned = true;
+ }
+ return;
+ }
+ break;
+ }
+ set_cpu_cap(&boot_cpu_data, cap);
+}
+
static void __init parse_host_cpu_flags(char *line)
{
+ u64 xcr0 = read_xcr0();
int i;
+
for (i = 0; i < 32*NCAPINTS; i++) {
if ((x86_cap_flags[i] != NULL) && strstr(line, x86_cap_flags[i]))
- set_cpu_cap(&boot_cpu_data, i);
+ validate_and_set_cpu_cap(i, xcr0);
}
}
static void __init parse_cache_line(char *line)
{
--
2.54.0
^ permalink raw reply related
* [PATCH 1/8] x86/fpu: Check for missing AVX and AVX-512 xstate bits
From: Eric Biggers @ 2026-06-26 4:37 UTC (permalink / raw)
To: x86
Cc: linux-um, linux-raid, linux-crypto, linux-kernel,
Christoph Hellwig, Andrew Morton, Eric Biggers
In-Reply-To: <20260626043731.319287-1-ebiggers@kernel.org>
If the CPU declares AVX or AVX-512 support, verify that the
corresponding xstate bits are also set. If not, warn and clear them.
This eliminates the perceived need for AVX and AVX-512 optimized code in
the kernel to call cpu_has_xfeatures(). That has never been universally
done, which strongly suggests that it has never really been needed in
practice, but this should remove any remaining doubt.
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
arch/x86/kernel/fpu/xstate.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index a7b6524a9dea..7f7e62e4ebc5 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -797,10 +797,27 @@ static u64 __init guest_default_mask(void)
* for KVM guests.
*/
return ~(u64)XFEATURE_MASK_USER_DYNAMIC;
}
+/* Clear any X86_FEATURE_* used by the kernel whose xfeatures are missing. */
+static void __init clear_cpu_caps_with_missing_xfeatures(u64 xfeatures)
+{
+ u64 mask;
+
+ mask = XFEATURE_MASK_FPSSE | XFEATURE_MASK_YMM;
+ if (boot_cpu_has(X86_FEATURE_AVX) && (xfeatures & mask) != mask) {
+ pr_err("x86/fpu: Disabling AVX support due to missing xstate features\n");
+ setup_clear_cpu_cap(X86_FEATURE_AVX);
+ }
+ mask = XFEATURE_MASK_FPSSE | XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512;
+ if (boot_cpu_has(X86_FEATURE_AVX512F) && (xfeatures & mask) != mask) {
+ pr_err("x86/fpu: Disabling AVX-512 support due to missing xstate features\n");
+ setup_clear_cpu_cap(X86_FEATURE_AVX512F);
+ }
+}
+
/*
* Enable and initialize the xsave feature.
* Called once per system bootup.
*/
void __init fpu__init_system_xstate(unsigned int legacy_size)
@@ -853,10 +870,12 @@ void __init fpu__init_system_xstate(unsigned int legacy_size)
pr_err("x86/fpu: Both APX/MPX present in the CPU's xstate features: 0x%llx.\n",
fpu_kernel_cfg.max_features);
goto out_disable;
}
+ clear_cpu_caps_with_missing_xfeatures(fpu_kernel_cfg.max_features);
+
fpu_kernel_cfg.independent_features = fpu_kernel_cfg.max_features &
XFEATURE_MASK_INDEPENDENT;
/*
* Clear XSAVE features that are disabled in the normal CPUID.
--
2.54.0
^ permalink raw reply related
* [PATCH 3/8] crypto: x86 - Stop using cpu_has_xfeatures()
From: Eric Biggers @ 2026-06-26 4:37 UTC (permalink / raw)
To: x86
Cc: linux-um, linux-raid, linux-crypto, linux-kernel,
Christoph Hellwig, Andrew Morton, Eric Biggers
In-Reply-To: <20260626043731.319287-1-ebiggers@kernel.org>
Checking both boot_cpu_has(X86_FEATURE_AVX*) and cpu_has_xfeatures() has
never really been needed in practice, and it's never been universally
done (e.g., lib/raid/ omits cpu_has_xfeatures()). Nevertheless, both
x86 and UML now explicitly clear the AVX and AVX-512 flags if their
xfeatures are missing, which should remove any remaining doubts.
Thus, remove all the calls to cpu_has_xfeatures(), as well as the
related checks of boot_cpu_has(X86_FEATURE_OSXSAVE).
In a few cases there was no corresponding boot_cpu_has(X86_FEATURE_AVX*)
check, so add the missing ones.
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
arch/x86/crypto/aegis128-aesni-glue.c | 3 +--
arch/x86/crypto/aesni-intel_glue.c | 7 ++-----
arch/x86/crypto/aria_aesni_avx2_glue.c | 11 +----------
arch/x86/crypto/aria_aesni_avx_glue.c | 11 +----------
arch/x86/crypto/aria_gfni_avx512_glue.c | 11 +----------
arch/x86/crypto/camellia_aesni_avx2_glue.c | 11 +----------
arch/x86/crypto/camellia_aesni_avx_glue.c | 11 +----------
arch/x86/crypto/cast5_avx_glue.c | 7 ++-----
arch/x86/crypto/cast6_avx_glue.c | 7 ++-----
arch/x86/crypto/serpent_avx2_glue.c | 9 +--------
arch/x86/crypto/serpent_avx_glue.c | 7 ++-----
arch/x86/crypto/sm4_aesni_avx2_glue.c | 11 +----------
arch/x86/crypto/sm4_aesni_avx_glue.c | 11 +----------
arch/x86/crypto/twofish_avx_glue.c | 6 ++----
14 files changed, 19 insertions(+), 104 deletions(-)
diff --git a/arch/x86/crypto/aegis128-aesni-glue.c b/arch/x86/crypto/aegis128-aesni-glue.c
index f1adfba1a76e..09fc0b15b0e9 100644
--- a/arch/x86/crypto/aegis128-aesni-glue.c
+++ b/arch/x86/crypto/aegis128-aesni-glue.c
@@ -263,12 +263,11 @@ static struct aead_alg crypto_aegis128_aesni_alg = {
};
static int __init crypto_aegis128_aesni_module_init(void)
{
if (!boot_cpu_has(X86_FEATURE_XMM4_1) ||
- !boot_cpu_has(X86_FEATURE_AES) ||
- !cpu_has_xfeatures(XFEATURE_MASK_SSE, NULL))
+ !boot_cpu_has(X86_FEATURE_AES))
return -ENODEV;
return crypto_register_aead(&crypto_aegis128_aesni_alg);
}
diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index f522fff9231e..f6f899db7482 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -1546,12 +1546,11 @@ static int __init register_avx_algs(void)
* For simplicity, just always check for VAES and VPCLMULQDQ together.
*/
if (!boot_cpu_has(X86_FEATURE_AVX2) ||
!boot_cpu_has(X86_FEATURE_VAES) ||
!boot_cpu_has(X86_FEATURE_VPCLMULQDQ) ||
- !boot_cpu_has(X86_FEATURE_PCLMULQDQ) ||
- !cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM, NULL))
+ !boot_cpu_has(X86_FEATURE_PCLMULQDQ))
return 0;
err = crypto_register_skciphers(skcipher_algs_vaes_avx2,
ARRAY_SIZE(skcipher_algs_vaes_avx2));
if (err)
return err;
@@ -1560,13 +1559,11 @@ static int __init register_avx_algs(void)
if (err)
return err;
if (!boot_cpu_has(X86_FEATURE_AVX512BW) ||
!boot_cpu_has(X86_FEATURE_AVX512VL) ||
- !boot_cpu_has(X86_FEATURE_BMI2) ||
- !cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM |
- XFEATURE_MASK_AVX512, NULL))
+ !boot_cpu_has(X86_FEATURE_BMI2))
return 0;
if (boot_cpu_has(X86_FEATURE_PREFER_YMM)) {
int i;
diff --git a/arch/x86/crypto/aria_aesni_avx2_glue.c b/arch/x86/crypto/aria_aesni_avx2_glue.c
index 1487a49bfbac..371be2fb6469 100644
--- a/arch/x86/crypto/aria_aesni_avx2_glue.c
+++ b/arch/x86/crypto/aria_aesni_avx2_glue.c
@@ -193,26 +193,17 @@ static struct skcipher_alg aria_algs[] = {
}
};
static int __init aria_avx2_init(void)
{
- const char *feature_name;
-
if (!boot_cpu_has(X86_FEATURE_AVX) ||
!boot_cpu_has(X86_FEATURE_AVX2) ||
- !boot_cpu_has(X86_FEATURE_AES) ||
- !boot_cpu_has(X86_FEATURE_OSXSAVE)) {
+ !boot_cpu_has(X86_FEATURE_AES)) {
pr_info("AVX2 or AES-NI instructions are not detected.\n");
return -ENODEV;
}
- if (!cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM,
- &feature_name)) {
- pr_info("CPU feature '%s' is not supported.\n", feature_name);
- return -ENODEV;
- }
-
if (boot_cpu_has(X86_FEATURE_GFNI)) {
aria_ops.aria_encrypt_16way = aria_aesni_avx_gfni_encrypt_16way;
aria_ops.aria_decrypt_16way = aria_aesni_avx_gfni_decrypt_16way;
aria_ops.aria_ctr_crypt_16way = aria_aesni_avx_gfni_ctr_crypt_16way;
aria_ops.aria_encrypt_32way = aria_aesni_avx2_gfni_encrypt_32way;
diff --git a/arch/x86/crypto/aria_aesni_avx_glue.c b/arch/x86/crypto/aria_aesni_avx_glue.c
index e4e3d78915a5..d23fc91c0ebd 100644
--- a/arch/x86/crypto/aria_aesni_avx_glue.c
+++ b/arch/x86/crypto/aria_aesni_avx_glue.c
@@ -180,25 +180,16 @@ static struct skcipher_alg aria_algs[] = {
}
};
static int __init aria_avx_init(void)
{
- const char *feature_name;
-
if (!boot_cpu_has(X86_FEATURE_AVX) ||
- !boot_cpu_has(X86_FEATURE_AES) ||
- !boot_cpu_has(X86_FEATURE_OSXSAVE)) {
+ !boot_cpu_has(X86_FEATURE_AES)) {
pr_info("AVX or AES-NI instructions are not detected.\n");
return -ENODEV;
}
- if (!cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM,
- &feature_name)) {
- pr_info("CPU feature '%s' is not supported.\n", feature_name);
- return -ENODEV;
- }
-
if (boot_cpu_has(X86_FEATURE_GFNI)) {
aria_ops.aria_encrypt_16way = aria_aesni_avx_gfni_encrypt_16way;
aria_ops.aria_decrypt_16way = aria_aesni_avx_gfni_decrypt_16way;
aria_ops.aria_ctr_crypt_16way = aria_aesni_avx_gfni_ctr_crypt_16way;
} else {
diff --git a/arch/x86/crypto/aria_gfni_avx512_glue.c b/arch/x86/crypto/aria_gfni_avx512_glue.c
index 363cbf4399cc..e05bbeb22d4a 100644
--- a/arch/x86/crypto/aria_gfni_avx512_glue.c
+++ b/arch/x86/crypto/aria_gfni_avx512_glue.c
@@ -194,28 +194,19 @@ static struct skcipher_alg aria_algs[] = {
}
};
static int __init aria_avx512_init(void)
{
- const char *feature_name;
-
if (!boot_cpu_has(X86_FEATURE_AVX) ||
!boot_cpu_has(X86_FEATURE_AVX2) ||
!boot_cpu_has(X86_FEATURE_AVX512F) ||
!boot_cpu_has(X86_FEATURE_AVX512VL) ||
- !boot_cpu_has(X86_FEATURE_GFNI) ||
- !boot_cpu_has(X86_FEATURE_OSXSAVE)) {
+ !boot_cpu_has(X86_FEATURE_GFNI)) {
pr_info("AVX512/GFNI instructions are not detected.\n");
return -ENODEV;
}
- if (!cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM |
- XFEATURE_MASK_AVX512, &feature_name)) {
- pr_info("CPU feature '%s' is not supported.\n", feature_name);
- return -ENODEV;
- }
-
aria_ops.aria_encrypt_16way = aria_aesni_avx_gfni_encrypt_16way;
aria_ops.aria_decrypt_16way = aria_aesni_avx_gfni_decrypt_16way;
aria_ops.aria_ctr_crypt_16way = aria_aesni_avx_gfni_ctr_crypt_16way;
aria_ops.aria_encrypt_32way = aria_aesni_avx2_gfni_encrypt_32way;
aria_ops.aria_decrypt_32way = aria_aesni_avx2_gfni_decrypt_32way;
diff --git a/arch/x86/crypto/camellia_aesni_avx2_glue.c b/arch/x86/crypto/camellia_aesni_avx2_glue.c
index 2d2f4e16537c..073fa3bb8388 100644
--- a/arch/x86/crypto/camellia_aesni_avx2_glue.c
+++ b/arch/x86/crypto/camellia_aesni_avx2_glue.c
@@ -95,26 +95,17 @@ static struct skcipher_alg camellia_algs[] = {
},
};
static int __init camellia_aesni_init(void)
{
- const char *feature_name;
-
if (!boot_cpu_has(X86_FEATURE_AVX) ||
!boot_cpu_has(X86_FEATURE_AVX2) ||
- !boot_cpu_has(X86_FEATURE_AES) ||
- !boot_cpu_has(X86_FEATURE_OSXSAVE)) {
+ !boot_cpu_has(X86_FEATURE_AES)) {
pr_info("AVX2 or AES-NI instructions are not detected.\n");
return -ENODEV;
}
- if (!cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM,
- &feature_name)) {
- pr_info("CPU feature '%s' is not supported.\n", feature_name);
- return -ENODEV;
- }
-
return crypto_register_skciphers(camellia_algs,
ARRAY_SIZE(camellia_algs));
}
static void __exit camellia_aesni_fini(void)
diff --git a/arch/x86/crypto/camellia_aesni_avx_glue.c b/arch/x86/crypto/camellia_aesni_avx_glue.c
index 5c321f255eb7..872e5e07220f 100644
--- a/arch/x86/crypto/camellia_aesni_avx_glue.c
+++ b/arch/x86/crypto/camellia_aesni_avx_glue.c
@@ -96,25 +96,16 @@ static struct skcipher_alg camellia_algs[] = {
}
};
static int __init camellia_aesni_init(void)
{
- const char *feature_name;
-
if (!boot_cpu_has(X86_FEATURE_AVX) ||
- !boot_cpu_has(X86_FEATURE_AES) ||
- !boot_cpu_has(X86_FEATURE_OSXSAVE)) {
+ !boot_cpu_has(X86_FEATURE_AES)) {
pr_info("AVX or AES-NI instructions are not detected.\n");
return -ENODEV;
}
- if (!cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM,
- &feature_name)) {
- pr_info("CPU feature '%s' is not supported.\n", feature_name);
- return -ENODEV;
- }
-
return crypto_register_skciphers(camellia_algs,
ARRAY_SIZE(camellia_algs));
}
static void __exit camellia_aesni_fini(void)
diff --git a/arch/x86/crypto/cast5_avx_glue.c b/arch/x86/crypto/cast5_avx_glue.c
index 3aca04d43b34..5de35e863370 100644
--- a/arch/x86/crypto/cast5_avx_glue.c
+++ b/arch/x86/crypto/cast5_avx_glue.c
@@ -90,15 +90,12 @@ static struct skcipher_alg cast5_algs[] = {
}
};
static int __init cast5_init(void)
{
- const char *feature_name;
-
- if (!cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM,
- &feature_name)) {
- pr_info("CPU feature '%s' is not supported.\n", feature_name);
+ if (!boot_cpu_has(X86_FEATURE_AVX)) {
+ pr_info("AVX instructions are not detected.\n");
return -ENODEV;
}
return crypto_register_skciphers(cast5_algs,
ARRAY_SIZE(cast5_algs));
diff --git a/arch/x86/crypto/cast6_avx_glue.c b/arch/x86/crypto/cast6_avx_glue.c
index c4dd28c30303..3d7ea48007bc 100644
--- a/arch/x86/crypto/cast6_avx_glue.c
+++ b/arch/x86/crypto/cast6_avx_glue.c
@@ -90,15 +90,12 @@ static struct skcipher_alg cast6_algs[] = {
},
};
static int __init cast6_init(void)
{
- const char *feature_name;
-
- if (!cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM,
- &feature_name)) {
- pr_info("CPU feature '%s' is not supported.\n", feature_name);
+ if (!boot_cpu_has(X86_FEATURE_AVX)) {
+ pr_info("AVX instructions are not detected.\n");
return -ENODEV;
}
return crypto_register_skciphers(cast6_algs, ARRAY_SIZE(cast6_algs));
}
diff --git a/arch/x86/crypto/serpent_avx2_glue.c b/arch/x86/crypto/serpent_avx2_glue.c
index f5f2121b7956..72a9e2b306d6 100644
--- a/arch/x86/crypto/serpent_avx2_glue.c
+++ b/arch/x86/crypto/serpent_avx2_glue.c
@@ -91,21 +91,14 @@ static struct skcipher_alg serpent_algs[] = {
},
};
static int __init serpent_avx2_init(void)
{
- const char *feature_name;
-
- if (!boot_cpu_has(X86_FEATURE_AVX2) || !boot_cpu_has(X86_FEATURE_OSXSAVE)) {
+ if (!boot_cpu_has(X86_FEATURE_AVX2)) {
pr_info("AVX2 instructions are not detected.\n");
return -ENODEV;
}
- if (!cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM,
- &feature_name)) {
- pr_info("CPU feature '%s' is not supported.\n", feature_name);
- return -ENODEV;
- }
return crypto_register_skciphers(serpent_algs,
ARRAY_SIZE(serpent_algs));
}
diff --git a/arch/x86/crypto/serpent_avx_glue.c b/arch/x86/crypto/serpent_avx_glue.c
index 9c8b3a335d5c..42c4e1569674 100644
--- a/arch/x86/crypto/serpent_avx_glue.c
+++ b/arch/x86/crypto/serpent_avx_glue.c
@@ -98,15 +98,12 @@ static struct skcipher_alg serpent_algs[] = {
},
};
static int __init serpent_init(void)
{
- const char *feature_name;
-
- if (!cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM,
- &feature_name)) {
- pr_info("CPU feature '%s' is not supported.\n", feature_name);
+ if (!boot_cpu_has(X86_FEATURE_AVX)) {
+ pr_info("AVX instructions are not detected.\n");
return -ENODEV;
}
return crypto_register_skciphers(serpent_algs,
ARRAY_SIZE(serpent_algs));
diff --git a/arch/x86/crypto/sm4_aesni_avx2_glue.c b/arch/x86/crypto/sm4_aesni_avx2_glue.c
index fec0ab7a63dd..eef73894e777 100644
--- a/arch/x86/crypto/sm4_aesni_avx2_glue.c
+++ b/arch/x86/crypto/sm4_aesni_avx2_glue.c
@@ -96,26 +96,17 @@ static struct skcipher_alg sm4_aesni_avx2_skciphers[] = {
}
};
static int __init sm4_init(void)
{
- const char *feature_name;
-
if (!boot_cpu_has(X86_FEATURE_AVX) ||
!boot_cpu_has(X86_FEATURE_AVX2) ||
- !boot_cpu_has(X86_FEATURE_AES) ||
- !boot_cpu_has(X86_FEATURE_OSXSAVE)) {
+ !boot_cpu_has(X86_FEATURE_AES)) {
pr_info("AVX2 or AES-NI instructions are not detected.\n");
return -ENODEV;
}
- if (!cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM,
- &feature_name)) {
- pr_info("CPU feature '%s' is not supported.\n", feature_name);
- return -ENODEV;
- }
-
return crypto_register_skciphers(sm4_aesni_avx2_skciphers,
ARRAY_SIZE(sm4_aesni_avx2_skciphers));
}
static void __exit sm4_exit(void)
diff --git a/arch/x86/crypto/sm4_aesni_avx_glue.c b/arch/x86/crypto/sm4_aesni_avx_glue.c
index 88caf418a06f..ed383da5ff46 100644
--- a/arch/x86/crypto/sm4_aesni_avx_glue.c
+++ b/arch/x86/crypto/sm4_aesni_avx_glue.c
@@ -312,25 +312,16 @@ static struct skcipher_alg sm4_aesni_avx_skciphers[] = {
}
};
static int __init sm4_init(void)
{
- const char *feature_name;
-
if (!boot_cpu_has(X86_FEATURE_AVX) ||
- !boot_cpu_has(X86_FEATURE_AES) ||
- !boot_cpu_has(X86_FEATURE_OSXSAVE)) {
+ !boot_cpu_has(X86_FEATURE_AES)) {
pr_info("AVX or AES-NI instructions are not detected.\n");
return -ENODEV;
}
- if (!cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM,
- &feature_name)) {
- pr_info("CPU feature '%s' is not supported.\n", feature_name);
- return -ENODEV;
- }
-
return crypto_register_skciphers(sm4_aesni_avx_skciphers,
ARRAY_SIZE(sm4_aesni_avx_skciphers));
}
static void __exit sm4_exit(void)
diff --git a/arch/x86/crypto/twofish_avx_glue.c b/arch/x86/crypto/twofish_avx_glue.c
index 9e20db013750..985bc54a2340 100644
--- a/arch/x86/crypto/twofish_avx_glue.c
+++ b/arch/x86/crypto/twofish_avx_glue.c
@@ -100,14 +100,12 @@ static struct skcipher_alg twofish_algs[] = {
},
};
static int __init twofish_init(void)
{
- const char *feature_name;
-
- if (!cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM, &feature_name)) {
- pr_info("CPU feature '%s' is not supported.\n", feature_name);
+ if (!boot_cpu_has(X86_FEATURE_AVX)) {
+ pr_info("AVX instructions are not detected.\n");
return -ENODEV;
}
return crypto_register_skciphers(twofish_algs,
ARRAY_SIZE(twofish_algs));
--
2.54.0
^ permalink raw reply related
* [PATCH 4/8] lib/crypto: x86: Stop using cpu_has_xfeatures()
From: Eric Biggers @ 2026-06-26 4:37 UTC (permalink / raw)
To: x86
Cc: linux-um, linux-raid, linux-crypto, linux-kernel,
Christoph Hellwig, Andrew Morton, Eric Biggers
In-Reply-To: <20260626043731.319287-1-ebiggers@kernel.org>
Checking both boot_cpu_has() and cpu_has_xfeatures() has never really
been needed in practice, and it's never been universally done (e.g.,
lib/raid/ omits cpu_has_xfeatures()). Nevertheless, both x86 and UML
now explicitly clear the AVX and AVX-512 flags if their xfeatures are
missing, which should remove any remaining doubts.
Thus, remove all the calls to cpu_has_xfeatures().
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
lib/crypto/x86/blake2s.h | 4 +---
lib/crypto/x86/chacha.h | 3 +--
lib/crypto/x86/nh.h | 4 +---
lib/crypto/x86/poly1305.h | 7 ++-----
lib/crypto/x86/sha1.h | 4 +---
lib/crypto/x86/sha256.h | 4 +---
lib/crypto/x86/sha512.h | 3 +--
lib/crypto/x86/sm3.h | 3 +--
8 files changed, 9 insertions(+), 23 deletions(-)
diff --git a/lib/crypto/x86/blake2s.h b/lib/crypto/x86/blake2s.h
index f8eed6cb042e..0f7c51f055c8 100644
--- a/lib/crypto/x86/blake2s.h
+++ b/lib/crypto/x86/blake2s.h
@@ -53,10 +53,8 @@ static void blake2s_mod_init_arch(void)
static_branch_enable(&blake2s_use_ssse3);
if (boot_cpu_has(X86_FEATURE_AVX) &&
boot_cpu_has(X86_FEATURE_AVX2) &&
boot_cpu_has(X86_FEATURE_AVX512F) &&
- boot_cpu_has(X86_FEATURE_AVX512VL) &&
- cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM |
- XFEATURE_MASK_AVX512, NULL))
+ boot_cpu_has(X86_FEATURE_AVX512VL))
static_branch_enable(&blake2s_use_avx512);
}
diff --git a/lib/crypto/x86/chacha.h b/lib/crypto/x86/chacha.h
index 10cf8f1c569d..c79562aac56b 100644
--- a/lib/crypto/x86/chacha.h
+++ b/lib/crypto/x86/chacha.h
@@ -163,12 +163,11 @@ static void chacha_mod_init_arch(void)
return;
static_branch_enable(&chacha_use_simd);
if (boot_cpu_has(X86_FEATURE_AVX) &&
- boot_cpu_has(X86_FEATURE_AVX2) &&
- cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM, NULL)) {
+ boot_cpu_has(X86_FEATURE_AVX2)) {
static_branch_enable(&chacha_use_avx2);
if (boot_cpu_has(X86_FEATURE_AVX512VL) &&
boot_cpu_has(X86_FEATURE_AVX512BW)) /* kmovq */
static_branch_enable(&chacha_use_avx512vl);
diff --git a/lib/crypto/x86/nh.h b/lib/crypto/x86/nh.h
index 83361c2e9783..342636dcb750 100644
--- a/lib/crypto/x86/nh.h
+++ b/lib/crypto/x86/nh.h
@@ -35,11 +35,9 @@ static bool nh_arch(const u32 *key, const u8 *message, size_t message_len,
#define nh_mod_init_arch nh_mod_init_arch
static void nh_mod_init_arch(void)
{
if (boot_cpu_has(X86_FEATURE_XMM2)) {
static_branch_enable(&have_sse2);
- if (boot_cpu_has(X86_FEATURE_AVX2) &&
- cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM,
- NULL))
+ if (boot_cpu_has(X86_FEATURE_AVX2))
static_branch_enable(&have_avx2);
}
}
diff --git a/lib/crypto/x86/poly1305.h b/lib/crypto/x86/poly1305.h
index ee92e3740a78..b061b9926fa5 100644
--- a/lib/crypto/x86/poly1305.h
+++ b/lib/crypto/x86/poly1305.h
@@ -141,18 +141,15 @@ static void poly1305_emit(const struct poly1305_state *ctx,
}
#define poly1305_mod_init_arch poly1305_mod_init_arch
static void poly1305_mod_init_arch(void)
{
- if (boot_cpu_has(X86_FEATURE_AVX) &&
- cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM, NULL))
+ if (boot_cpu_has(X86_FEATURE_AVX))
static_branch_enable(&poly1305_use_avx);
- if (boot_cpu_has(X86_FEATURE_AVX) && boot_cpu_has(X86_FEATURE_AVX2) &&
- cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM, NULL))
+ if (boot_cpu_has(X86_FEATURE_AVX) && boot_cpu_has(X86_FEATURE_AVX2))
static_branch_enable(&poly1305_use_avx2);
if (boot_cpu_has(X86_FEATURE_AVX) && boot_cpu_has(X86_FEATURE_AVX2) &&
boot_cpu_has(X86_FEATURE_AVX512F) &&
- cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512, NULL) &&
/* Skylake downclocks unacceptably much when using zmm, but later generations are fast. */
boot_cpu_data.x86_vfm != INTEL_SKYLAKE_X)
static_branch_enable(&poly1305_use_avx512);
}
diff --git a/lib/crypto/x86/sha1.h b/lib/crypto/x86/sha1.h
index c48a0131fd12..6aff433466e7 100644
--- a/lib/crypto/x86/sha1.h
+++ b/lib/crypto/x86/sha1.h
@@ -57,13 +57,11 @@ static void sha1_blocks(struct sha1_block_state *state,
#define sha1_mod_init_arch sha1_mod_init_arch
static void sha1_mod_init_arch(void)
{
if (boot_cpu_has(X86_FEATURE_SHA_NI)) {
static_call_update(sha1_blocks_x86, sha1_blocks_ni);
- } else if (cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM,
- NULL) &&
- boot_cpu_has(X86_FEATURE_AVX)) {
+ } else if (boot_cpu_has(X86_FEATURE_AVX)) {
if (boot_cpu_has(X86_FEATURE_AVX2) &&
boot_cpu_has(X86_FEATURE_BMI1) &&
boot_cpu_has(X86_FEATURE_BMI2))
static_call_update(sha1_blocks_x86, sha1_blocks_avx2);
else
diff --git a/lib/crypto/x86/sha256.h b/lib/crypto/x86/sha256.h
index 0ee69d8e39fe..e98ffdaf4b14 100644
--- a/lib/crypto/x86/sha256.h
+++ b/lib/crypto/x86/sha256.h
@@ -102,13 +102,11 @@ static void sha256_mod_init_arch(void)
static_branch_enable(&have_sha_ni);
} else if (IS_ENABLED(CONFIG_CPU_SUP_ZHAOXIN) &&
boot_cpu_has(X86_FEATURE_PHE_EN) &&
boot_cpu_data.x86 >= 0x07) {
static_call_update(sha256_blocks_x86, sha256_blocks_phe);
- } else if (cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM,
- NULL) &&
- boot_cpu_has(X86_FEATURE_AVX)) {
+ } else if (boot_cpu_has(X86_FEATURE_AVX)) {
if (boot_cpu_has(X86_FEATURE_AVX2) &&
boot_cpu_has(X86_FEATURE_BMI2))
static_call_update(sha256_blocks_x86,
sha256_blocks_avx2);
else
diff --git a/lib/crypto/x86/sha512.h b/lib/crypto/x86/sha512.h
index 0213c70cedd0..4e177b4606bd 100644
--- a/lib/crypto/x86/sha512.h
+++ b/lib/crypto/x86/sha512.h
@@ -35,12 +35,11 @@ static void sha512_blocks(struct sha512_block_state *state,
}
#define sha512_mod_init_arch sha512_mod_init_arch
static void sha512_mod_init_arch(void)
{
- if (cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM, NULL) &&
- boot_cpu_has(X86_FEATURE_AVX)) {
+ if (boot_cpu_has(X86_FEATURE_AVX)) {
if (boot_cpu_has(X86_FEATURE_AVX2) &&
boot_cpu_has(X86_FEATURE_BMI2))
static_call_update(sha512_blocks_x86,
sha512_blocks_avx2);
else
diff --git a/lib/crypto/x86/sm3.h b/lib/crypto/x86/sm3.h
index 3834780f2f6a..e06d4a22e4fa 100644
--- a/lib/crypto/x86/sm3.h
+++ b/lib/crypto/x86/sm3.h
@@ -31,9 +31,8 @@ static void sm3_blocks(struct sm3_block_state *state,
}
#define sm3_mod_init_arch sm3_mod_init_arch
static void sm3_mod_init_arch(void)
{
- if (boot_cpu_has(X86_FEATURE_AVX) && boot_cpu_has(X86_FEATURE_BMI2) &&
- cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM, NULL))
+ if (boot_cpu_has(X86_FEATURE_AVX) && boot_cpu_has(X86_FEATURE_BMI2))
static_call_update(sm3_blocks_x86, sm3_blocks_avx);
}
--
2.54.0
^ permalink raw reply related
* [PATCH 5/8] lib/crc: x86: Stop using cpu_has_xfeatures()
From: Eric Biggers @ 2026-06-26 4:37 UTC (permalink / raw)
To: x86
Cc: linux-um, linux-raid, linux-crypto, linux-kernel,
Christoph Hellwig, Andrew Morton, Eric Biggers
In-Reply-To: <20260626043731.319287-1-ebiggers@kernel.org>
Checking both boot_cpu_has() and cpu_has_xfeatures() has never really
been needed in practice, and it's never been universally done (e.g.,
lib/raid/ omits cpu_has_xfeatures()). Nevertheless, both x86 and UML
now explicitly clear the AVX and AVX-512 flags if their xfeatures are
missing, which should remove any remaining doubts.
Thus, remove all the calls to cpu_has_xfeatures().
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
lib/crc/x86/crc-pclmul-template.h | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/lib/crc/x86/crc-pclmul-template.h b/lib/crc/x86/crc-pclmul-template.h
index 02744831c6fa..893119bb7c07 100644
--- a/lib/crc/x86/crc-pclmul-template.h
+++ b/lib/crc/x86/crc-pclmul-template.h
@@ -25,20 +25,18 @@ crc_t prefix##_vpclmul_avx512(crc_t crc, const u8 *p, size_t len, \
DEFINE_STATIC_CALL(prefix##_pclmul, prefix##_pclmul_sse)
static inline bool have_vpclmul(void)
{
return boot_cpu_has(X86_FEATURE_VPCLMULQDQ) &&
- boot_cpu_has(X86_FEATURE_AVX2) &&
- cpu_has_xfeatures(XFEATURE_MASK_YMM, NULL);
+ boot_cpu_has(X86_FEATURE_AVX2);
}
static inline bool have_avx512(void)
{
return boot_cpu_has(X86_FEATURE_AVX512BW) &&
boot_cpu_has(X86_FEATURE_AVX512VL) &&
- !boot_cpu_has(X86_FEATURE_PREFER_YMM) &&
- cpu_has_xfeatures(XFEATURE_MASK_AVX512, NULL);
+ !boot_cpu_has(X86_FEATURE_PREFER_YMM);
}
/*
* Call a [V]PCLMULQDQ optimized CRC function if the data length is at least 16
* bytes, the CPU has PCLMULQDQ support, and the current context may use SIMD.
--
2.54.0
^ permalink raw reply related
* [PATCH 6/8] x86/fpu: Remove cpu_has_xfeatures()
From: Eric Biggers @ 2026-06-26 4:37 UTC (permalink / raw)
To: x86
Cc: linux-um, linux-raid, linux-crypto, linux-kernel,
Christoph Hellwig, Andrew Morton, Eric Biggers
In-Reply-To: <20260626043731.319287-1-ebiggers@kernel.org>
The only remaining caller of cpu_has_xfeatures() is
print_xstate_features(), which uses it only to check and get the name of
a single feature.
Remove it and just inline the needed code into print_xstate_features().
This also makes the "unknown xstate feature" entry at index XFEATURE_MAX
of xfeature_names[] unnecessary, so remove that too.
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
arch/x86/include/asm/fpu/api.h | 9 -------
arch/x86/kernel/fpu/xstate.c | 44 +++-------------------------------
2 files changed, 3 insertions(+), 50 deletions(-)
diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
index 90c63fe19c0f..cfed8b24d64f 100644
--- a/arch/x86/include/asm/fpu/api.h
+++ b/arch/x86/include/asm/fpu/api.h
@@ -97,19 +97,10 @@ static inline void fpregs_assert_state_consistent(void) { }
/*
* Load the task FPU state before returning to userspace.
*/
extern void switch_fpu_return(void);
-/*
- * Query the presence of one or more xfeatures. Works on any legacy CPU as well.
- *
- * If 'feature_name' is set then put a human-readable description of
- * the feature there as well - this can be used to print error (or success)
- * messages.
- */
-extern int cpu_has_xfeatures(u64 xfeatures_mask, const char **feature_name);
-
/* Trap handling */
extern int fpu__exception_code(struct fpu *fpu, int trap_nr);
extern void fpu_sync_fpstate(struct fpu *fpu);
extern void fpu_reset_from_exception_fixup(void);
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 7f7e62e4ebc5..c6f0264f957c 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -64,12 +64,12 @@ static const char *xfeature_names[] =
"unknown xstate feature",
"unknown xstate feature",
"AMX Tile config",
"AMX Tile data",
"APX registers",
- "unknown xstate feature",
};
+static_assert(ARRAY_SIZE(xfeature_names) == XFEATURE_MAX);
static unsigned short xsave_cpuid_features[] __initdata = {
[XFEATURE_FP] = X86_FEATURE_FPU,
[XFEATURE_SSE] = X86_FEATURE_XMM,
[XFEATURE_YMM] = X86_FEATURE_AVX,
@@ -120,48 +120,10 @@ static inline unsigned int next_xfeature_order(unsigned int i, u64 mask)
i++)
#define XSTATE_FLAG_SUPERVISOR BIT(0)
#define XSTATE_FLAG_ALIGNED64 BIT(1)
-/*
- * Return whether the system supports a given xfeature.
- *
- * Also return the name of the (most advanced) feature that the caller requested:
- */
-int cpu_has_xfeatures(u64 xfeatures_needed, const char **feature_name)
-{
- u64 xfeatures_missing = xfeatures_needed & ~fpu_kernel_cfg.max_features;
-
- if (unlikely(feature_name)) {
- long xfeature_idx, max_idx;
- u64 xfeatures_print;
- /*
- * So we use FLS here to be able to print the most advanced
- * feature that was requested but is missing. So if a driver
- * asks about "XFEATURE_MASK_SSE | XFEATURE_MASK_YMM" we'll print the
- * missing AVX feature - this is the most informative message
- * to users:
- */
- if (xfeatures_missing)
- xfeatures_print = xfeatures_missing;
- else
- xfeatures_print = xfeatures_needed;
-
- xfeature_idx = fls64(xfeatures_print)-1;
- max_idx = ARRAY_SIZE(xfeature_names)-1;
- xfeature_idx = min(xfeature_idx, max_idx);
-
- *feature_name = xfeature_names[xfeature_idx];
- }
-
- if (xfeatures_missing)
- return 0;
-
- return 1;
-}
-EXPORT_SYMBOL_GPL(cpu_has_xfeatures);
-
static bool xfeature_is_aligned64(int xfeature_nr)
{
return xstate_flags[xfeature_nr] & XSTATE_FLAG_ALIGNED64;
}
@@ -300,13 +262,13 @@ static void __init print_xstate_features(void)
{
int i;
for (i = 0; i < XFEATURE_MAX; i++) {
u64 mask = BIT_ULL(i);
- const char *name;
+ const char *name = xfeature_names[i];
- if (cpu_has_xfeatures(mask, &name))
+ if (fpu_kernel_cfg.max_features & mask)
pr_info("x86/fpu: Supporting XSAVE feature 0x%03Lx: '%s'\n", mask, name);
}
}
/*
--
2.54.0
^ permalink raw reply related
* [PATCH 7/8] lib/raid/xor: x86: Remove redundant X86_FEATURE_OSXSAVE check
From: Eric Biggers @ 2026-06-26 4:37 UTC (permalink / raw)
To: x86
Cc: linux-um, linux-raid, linux-crypto, linux-kernel,
Christoph Hellwig, Andrew Morton, Eric Biggers
In-Reply-To: <20260626043731.319287-1-ebiggers@kernel.org>
X86_FEATURE_AVX implies X86_FEATURE_OSXSAVE already.
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
lib/raid/xor/x86/xor_arch.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/lib/raid/xor/x86/xor_arch.h b/lib/raid/xor/x86/xor_arch.h
index 99fe85a213c6..991abe3f4bbd 100644
--- a/lib/raid/xor/x86/xor_arch.h
+++ b/lib/raid/xor/x86/xor_arch.h
@@ -16,12 +16,11 @@ extern struct xor_block_template xor_block_avx;
*
* 32-bit without MMX can fall back to the generic routines.
*/
static __always_inline void __init arch_xor_init(void)
{
- if (boot_cpu_has(X86_FEATURE_AVX) &&
- boot_cpu_has(X86_FEATURE_OSXSAVE)) {
+ if (boot_cpu_has(X86_FEATURE_AVX)) {
xor_force(&xor_block_avx);
} else if (IS_ENABLED(CONFIG_X86_64) || boot_cpu_has(X86_FEATURE_XMM)) {
xor_register(&xor_block_sse);
xor_register(&xor_block_sse_pf64);
} else if (boot_cpu_has(X86_FEATURE_MMX)) {
--
2.54.0
^ permalink raw reply related
* [PATCH 8/8] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Eric Biggers @ 2026-06-26 4:37 UTC (permalink / raw)
To: x86
Cc: linux-um, linux-raid, linux-crypto, linux-kernel,
Christoph Hellwig, Andrew Morton, Eric Biggers, David Laight
In-Reply-To: <20260626043731.319287-1-ebiggers@kernel.org>
Add an implementation of xor_gen() using AVX-512.
It uses 512-bit vectors, i.e. ZMM registers. It also uses the
vpternlogq instruction to do three-input XORs when applicable.
It's enabled on x86_64 CPUs that have AVX512F && !PREFER_YMM. In
practice that means:
- AMD Zen 4 and later (client and server)
- Intel Sapphire Rapids and later (server)
- Intel Rocket Lake (client)
- Intel Nova Lake and later (client)
The !PREFER_YMM condition excludes the older AVX-512 implementations in
Intel Skylake Server and Intel Ice Lake. They could run this code, but
they're known to have overly-eager downclocking when ZMM registers are
used. This is the same policy that the crypto and CRC code uses.
Benchmark on AMD Ryzen 9 9950X (Zen 5):
src_cnt avx avx512 Improvement
======= ========== ========== ===========
1 56353 MB/s 75388 MB/s 33%
2 54274 MB/s 68409 MB/s 26%
3 44649 MB/s 64042 MB/s 43%
4 41315 MB/s 55002 MB/s 33%
Reviewed-by: David Laight <david.laight.linux@gmail.com>
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
lib/raid/xor/Makefile | 2 +-
lib/raid/xor/x86/xor-avx512.c | 121 ++++++++++++++++++++++++++++++++++
lib/raid/xor/x86/xor_arch.h | 23 ++++---
3 files changed, 135 insertions(+), 11 deletions(-)
create mode 100644 lib/raid/xor/x86/xor-avx512.c
diff --git a/lib/raid/xor/Makefile b/lib/raid/xor/Makefile
index e8ecec3c09f9..4a0e5c6d8298 100644
--- a/lib/raid/xor/Makefile
+++ b/lib/raid/xor/Makefile
@@ -27,11 +27,11 @@ xor-$(CONFIG_ALTIVEC) += powerpc/xor_vmx.o powerpc/xor_vmx_glue.o
xor-$(CONFIG_RISCV_ISA_V) += riscv/xor.o riscv/xor-glue.o
xor-$(CONFIG_SPARC32) += sparc/xor-sparc32.o
xor-$(CONFIG_SPARC64) += sparc/xor-sparc64.o sparc/xor-sparc64-glue.o
xor-$(CONFIG_S390) += s390/xor.o
xor-$(CONFIG_X86_32) += x86/xor-avx.o x86/xor-sse.o x86/xor-mmx.o
-xor-$(CONFIG_X86_64) += x86/xor-avx.o x86/xor-sse.o
+xor-$(CONFIG_X86_64) += x86/xor-avx.o x86/xor-sse.o x86/xor-avx512.o
obj-y += tests/
CFLAGS_xor-neon.o += $(CC_FLAGS_FPU) -I$(src)/$(SRCARCH)
CFLAGS_REMOVE_xor-neon.o += $(CC_FLAGS_NO_FPU)
diff --git a/lib/raid/xor/x86/xor-avx512.c b/lib/raid/xor/x86/xor-avx512.c
new file mode 100644
index 000000000000..17f57900d827
--- /dev/null
+++ b/lib/raid/xor/x86/xor-avx512.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * AVX-512 optimized implementation of xor_gen()
+ *
+ * Copyright 2026 Google LLC
+ */
+
+#include <linux/types.h>
+#include <asm/fpu/api.h>
+#include "xor_impl.h"
+#include "xor_arch.h"
+
+/*
+ * Implementation notes:
+ *
+ * Unrolling by the number of buffers (2-5) is very important.
+ *
+ * Unrolling by length is less important, especially when using register-indexed
+ * addressing with negative indices from the end of the buffers. That approach
+ * results in just two loop control instructions being needed per iteration,
+ * regardless of the number of buffers.
+ *
+ * In fact, benchmarks showed that the 2 and 3 buffer cases require only 2x
+ * unrolling by length, while the 4 and 5 buffer cases don't require any
+ * unrolling by length. Benchmarks also showed that the register-indexed
+ * addressing isn't a bottleneck either; i.e., we can't do any better by
+ * incrementing the pointers as we go along, even with more unrolling.
+ */
+
+static void xor_avx512_2(long bytes, u8 *p1, const u8 *p2)
+{
+ long i = -bytes;
+
+ asm volatile("1: vmovdqa64 (%1,%0), %%zmm0\n"
+ "vmovdqa64 64(%1,%0), %%zmm1\n"
+ "vpxorq (%2,%0), %%zmm0, %%zmm0\n"
+ "vpxorq 64(%2,%0), %%zmm1, %%zmm1\n"
+ "vmovdqa64 %%zmm0, (%1,%0)\n"
+ "vmovdqa64 %%zmm1, 64(%1,%0)\n"
+ "add $128, %0\n"
+ "jnz 1b\n"
+ : "+&r"(i)
+ : "r"(p1 + bytes), "r"(p2 + bytes)
+ : "memory", "cc");
+}
+
+static void xor_avx512_3(long bytes, u8 *p1, const u8 *p2, const u8 *p3)
+{
+ long i = -bytes;
+
+ asm volatile("1: vmovdqa64 (%1,%0), %%zmm0\n"
+ "vmovdqa64 64(%1,%0), %%zmm1\n"
+ "vmovdqa64 (%2,%0), %%zmm2\n"
+ "vmovdqa64 64(%2,%0), %%zmm3\n"
+ "vpternlogq $0x96, (%3,%0), %%zmm2, %%zmm0\n"
+ "vpternlogq $0x96, 64(%3,%0), %%zmm3, %%zmm1\n"
+ "vmovdqa64 %%zmm0, (%1,%0)\n"
+ "vmovdqa64 %%zmm1, 64(%1,%0)\n"
+ "add $128, %0\n"
+ "jnz 1b\n"
+ : "+&r"(i)
+ : "r"(p1 + bytes), "r"(p2 + bytes), "r"(p3 + bytes)
+ : "memory", "cc");
+}
+
+static void xor_avx512_4(long bytes, u8 *p1, const u8 *p2, const u8 *p3,
+ const u8 *p4)
+{
+ long i = -bytes;
+
+ asm volatile("1: vmovdqa64 (%1,%0), %%zmm0\n"
+ "vmovdqa64 (%2,%0), %%zmm1\n"
+ "vpxorq (%3,%0), %%zmm0, %%zmm0\n"
+ "vpternlogq $0x96, (%4,%0), %%zmm1, %%zmm0\n"
+ "vmovdqa64 %%zmm0, (%1,%0)\n"
+ "add $64, %0\n"
+ "jnz 1b\n"
+ : "+&r"(i)
+ : "r"(p1 + bytes), "r"(p2 + bytes), "r"(p3 + bytes),
+ "r"(p4 + bytes)
+ : "memory", "cc");
+}
+
+static void xor_avx512_5(long bytes, u8 *p1, const u8 *p2, const u8 *p3,
+ const u8 *p4, const u8 *p5)
+{
+ long i = -bytes;
+
+ asm volatile("1: vmovdqa64 (%1,%0), %%zmm0\n"
+ "vmovdqa64 (%2,%0), %%zmm1\n"
+ "vpternlogq $0x96, (%3,%0), %%zmm1, %%zmm0\n"
+ "vmovdqa64 (%4,%0), %%zmm1\n"
+ "vpternlogq $0x96, (%5,%0), %%zmm1, %%zmm0\n"
+ "vmovdqa64 %%zmm0, (%1,%0)\n"
+ "add $64, %0\n"
+ "jnz 1b\n"
+ : "+&r"(i)
+ : "r"(p1 + bytes), "r"(p2 + bytes), "r"(p3 + bytes),
+ "r"(p4 + bytes), "r"(p5 + bytes)
+ : "memory", "cc");
+}
+
+DO_XOR_BLOCKS(avx512_inner, xor_avx512_2, xor_avx512_3, xor_avx512_4,
+ xor_avx512_5);
+
+/*
+ * Preconditions: bytes is a nonzero multiple of 512, and all buffers are
+ * 64-byte aligned.
+ */
+static void xor_gen_avx512(void *dest, void **srcs, unsigned int src_cnt,
+ unsigned int bytes)
+{
+ kernel_fpu_begin();
+ xor_gen_avx512_inner(dest, srcs, src_cnt, bytes);
+ kernel_fpu_end();
+}
+
+struct xor_block_template xor_block_avx512 = {
+ .name = "avx512",
+ .xor_gen = xor_gen_avx512,
+};
diff --git a/lib/raid/xor/x86/xor_arch.h b/lib/raid/xor/x86/xor_arch.h
index 991abe3f4bbd..d5e192b8793f 100644
--- a/lib/raid/xor/x86/xor_arch.h
+++ b/lib/raid/xor/x86/xor_arch.h
@@ -4,25 +4,28 @@
extern struct xor_block_template xor_block_pII_mmx;
extern struct xor_block_template xor_block_p5_mmx;
extern struct xor_block_template xor_block_sse;
extern struct xor_block_template xor_block_sse_pf64;
extern struct xor_block_template xor_block_avx;
+extern struct xor_block_template xor_block_avx512;
-/*
- * When SSE is available, use it as it can write around L2. We may also be able
- * to load into the L1 only depending on how the cpu deals with a load to a line
- * that is being prefetched.
- *
- * When AVX2 is available, force using it as it is better by all measures.
- *
- * 32-bit without MMX can fall back to the generic routines.
- */
static __always_inline void __init arch_xor_init(void)
{
- if (boot_cpu_has(X86_FEATURE_AVX)) {
+ if (IS_ENABLED(CONFIG_X86_64) && boot_cpu_has(X86_FEATURE_AVX512F) &&
+ !boot_cpu_has(X86_FEATURE_PREFER_YMM)) {
+ /* AVX-512 will be the best; no need to try others. */
+ /* !PREFER_YMM excludes CPUs with overly-eager downclocking. */
+ xor_force(&xor_block_avx512);
+ } else if (boot_cpu_has(X86_FEATURE_AVX)) {
+ /* AVX will be the best; no need to try others. */
xor_force(&xor_block_avx);
} else if (IS_ENABLED(CONFIG_X86_64) || boot_cpu_has(X86_FEATURE_XMM)) {
+ /*
+ * When SSE is available, use it as it can write around L2. We
+ * may also be able to load into the L1 only depending on how
+ * the cpu deals with a load to a line that is being prefetched.
+ */
xor_register(&xor_block_sse);
xor_register(&xor_block_sse_pf64);
} else if (boot_cpu_has(X86_FEATURE_MMX)) {
xor_register(&xor_block_pII_mmx);
xor_register(&xor_block_p5_mmx);
--
2.54.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox