* Re: [PATCH 2/2] xor/kunit: add a benchmark
From: Eric Biggers @ 2026-06-17 17:14 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Andrew Morton, linux-kernel, linux-raid
In-Reply-To: <20260617054416.3841334-3-hch@lst.de>
On Wed, Jun 17, 2026 at 07:44:04AM +0200, Christoph Hellwig wrote:
> diff --git a/lib/raid/xor/tests/xor_kunit.c b/lib/raid/xor/tests/xor_kunit.c
> index 659ae3edbc25..5939b78b3944 100644
> --- a/lib/raid/xor/tests/xor_kunit.c
> +++ b/lib/raid/xor/tests/xor_kunit.c
> @@ -125,8 +125,71 @@ static void xor_test(struct kunit *test)
> }
> }
>
> +#ifdef CONFIG_XOR_BENCHMARK
> +static void xor_benchmark(struct kunit *test)
The #ifdef can be avoided using kunit_skip(), as the crypto and CRC
tests do:
if (!IS_ENABLED(CONFIG_XOR_BENCHMARK))
kunit_skip(test, "not enabled");
> + for (j = 0; j < ARRAY_SIZE(len_to_test); j++) {
> + unsigned int len = len_to_test[j];
> + const unsigned long num_iters = 1000;
> +
> + KUNIT_ASSERT_GT(test, len, 0);
> + KUNIT_ASSERT_LE(test, len, XOR_KUNIT_MAX_BYTES);
> +
> + preempt_disable();
> + t = ktime_get();
> + for (l = 0; l < num_iters; l++)
> + xor_gen(test_dest, test_buffers, nr, len);
> + t = ktime_get_ns() - t;
> + preempt_enable();
First one should be ktime_get_ns(), not ktime_get().
> +
> + speed[j] = div_u64((u64)len * num_iters * nr, t);
> + }
> +
> + static_assert(ARRAY_SIZE(len_to_test) == 2);
> + kunit_info(test, "%3u disks:\t%5llu GB/s\t%5llu GB/s\n",
> + nr, speed[0], speed[1]);
As mentioned in the other thread, this measures the speed at which the
source data is consumed, which differs from the code in
lib/raid/xor/xor-core.c that measures the speed at which the destination
data is produced. Probably best to make them consistent.
- Eric
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Eric Biggers @ 2026-06-17 15:44 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Andrew Morton, linux-kernel, linux-crypto, x86, David Laight,
linux-raid
In-Reply-To: <20260617055653.GB19218@lst.de>
On Wed, Jun 17, 2026 at 07:56:53AM +0200, Christoph Hellwig wrote:
> Can use the xor: prefix used for all other commits to lib/raid/xor?
>
> > 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%
>
> On my Zen 5 mobile (AMD Ryzen AI 7 PRO 350) both the existing
> AVX2 and this AVX512 code give numbers in the 200+ GB/s range. Not
> sure if is just the different benchmarking or something else going on.
I used lib/raid/xor/xor-core.c which measures the throughput of parity
data generated, whereas your proposed xor_benchmark() in xor_kunit
measures the throughput of source data consumed. I don't know which
makes more sense, but we should make them consistent with each other.
> FYI, one or 2 sources are basically useless as they RAID5 configs
> that have no benefits over simple mirroring and thus the numbers
> aren't too interesting.
>
> > +DO_XOR_BLOCKS(avx512_inner, xor_avx512_2, xor_avx512_3, xor_avx512_4,
> > + xor_avx512_5);
>
> Is there really much of a benefit of doing the historic DO_XOR_BLOCKS
> vs doing the loop manually? Especially as the common cases for a
> modern RAID will usually loop over more disks than this was built
> for. I.e., in practice one or two source buffers only happen at the
> end of a loop over more disks.
There's not really a way out of unrolling by source buffer count, as
otherwise the pointers would continuously have to be reloaded into
registers. That's why your proposal was so slow (see the numbers I gave
in https://lore.kernel.org/linux-crypto/20260612055933.GA6675@sol/ ).
It could be something different from 2-5 specifically, or open-coded
instead of using the macro if that's all you're asking for, but at a
high level the unrolling by source buffer count does seem to be needed.
- Eric
^ permalink raw reply
* Re: [PATCH v2] md/raid5: protect bitmap batch counters aka seq_flush/seq_write consistency
From: sashiko-bot @ 2026-06-17 14:43 UTC (permalink / raw)
To: Chen Cheng; +Cc: yukuai, linux-raid
In-Reply-To: <20260617142839.882378-1-chencheng@fnnas.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Direct integer comparison of sequence numbers `bm_seq` in `stripe_add_to_batch_list` fails to handle integer wrap-around.
--
commit d138461da7f53106a83faa762ae81dbb822cbfca
Author: Chen Cheng <chencheng@fnnas.com>
md/raid5: protect bitmap batch counters aka seq_flush/seq_write consistency
This patch protects seq_flush and seq_write consistency across multiple CPUs.
It uses READ_ONCE() and WRITE_ONCE() to prevent data races detected by KCSAN
when assigning sh->bm_seq without holding device_lock.
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index d0d7f96fd7cd9..891c2cb605d30 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -3538,7 +3538,7 @@ static void __add_stripe_bio(struct stripe_head *sh, struct bio *bi,
> sh->dev[dd_idx].sector);
>
> if (conf->mddev->bitmap && firstwrite && !sh->batch_head) {
> - sh->bm_seq = conf->seq_flush+1;
> + sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
> set_bit(STRIPE_BIT_DELAY, &sh->state);
> }
> }
[Severity: Critical]
This is a pre-existing issue, but does stripe_add_to_batch_list() handle
32-bit integer wrap-around correctly when comparing sequence numbers?
Since seq_flush, seq_write, and sh->bm_seq increment for each batch, they
will eventually overflow and wrap around to negative values. In other parts
of the code, this is handled using signed subtraction (e.g.,
sh->bm_seq - conf->seq_write > 0).
However, in stripe_add_to_batch_list(), there is a direct comparison:
drivers/md/raid5.c:stripe_add_to_batch_list() {
...
if (test_and_clear_bit(STRIPE_BIT_DELAY, &sh->state)) {
int seq = sh->bm_seq;
if (test_bit(STRIPE_BIT_DELAY, &sh->batch_head->state) &&
sh->batch_head->bm_seq > seq)
seq = sh->batch_head->bm_seq;
...
}
At the wrap-around boundary, a newer sequence number is numerically less than
an older sequence number. Does this direct comparison incorrectly evaluate to
false, assigning the batch head an older sequence number?
Could this cause the batch to be released early when conf->seq_write catches
up, completing before its newer bitmap updates are fully flushed? Should this
comparison use subtraction instead, such as sh->batch_head->bm_seq - seq > 0?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617142839.882378-1-chencheng@fnnas.com?part=1
^ permalink raw reply
* [PATCH v2] md/raid5: protect bitmap batch counters aka seq_flush/seq_write consistency
From: Chen Cheng @ 2026-06-17 14:28 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, chenchneg33, linux-kernel
From: Chen Cheng <chencheng@fnnas.com>
kcsan detect race :
- raid5d() closes the current bitmap batch by updating
conf->seq_flush under conf->device_lock.
- __add_stripe_bio() read conf->seq_flush without that
lock when assigning sh->bm_seq.
so, protect seq_flush/seq_write consistency for multiple CPUs by
READ_ONCE()/WRITE_ONCE() under the path without held device_lock.
re-explain the stripe batch sequence number update flow:
1. sh->bm_seq declare which batch number the stripe belongs to
when perform bitmap-related write.
==> bm_seq = seq_flush+1
2. stripe be handled,
* if sh->bm_seq - conf->seq_write > 0, means the
batch stripes **newer than** the last written
batch, it cannot proceed yet, queued on bitmap_list.
* otherwise , has already proceed.
3. raid5d() `++seq_flush` to closes the current batch, means
* no more stripes join that old batch
* just-closed batch ready to write-out to disk
4. raid5d() calls bitmap hooks unplug() or writeout, then,
`++seq_write` to the same as bm_seq.
- seq_flush - for producer, to close batches.
- seq_write - for consumer, the checkpoint number.
the report:
====================================
BUG: KCSAN: data-race in __add_stripe_bio / raid5d
write to 0xffff88ba5625d470 of 4 bytes by task 82401 on cpu 0:
raid5d+0x1d9/0xba0
[.....]
read to 0xffff88ba5625d470 of 4 bytes by task 82421 on cpu 8:
__add_stripe_bio+0x332/0x400
raid5_make_request+0x6ac/0x2930
md_handle_request+0x4a2/0xa40
md_submit_bio+0x109/0x1a0
__submit_bio+0x2ec/0x390
[.....]
v1 -> v2:
- remove WRITE_ONCE(conf->seq_write) in held device_lock path.
- remove READ_ONCE(conf->seq_flush) in held device_lock path.
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/raid5.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index a320b71d7117..b2c5a1930841 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -3536,11 +3536,11 @@ static void __add_stripe_bio(struct stripe_head *sh, struct bio *bi,
pr_debug("added bi b#%llu to stripe s#%llu, disk %d, logical %llu\n",
(*bip)->bi_iter.bi_sector, sh->sector, dd_idx,
sh->dev[dd_idx].sector);
if (conf->mddev->bitmap && firstwrite && !sh->batch_head) {
- sh->bm_seq = conf->seq_flush+1;
+ sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
set_bit(STRIPE_BIT_DELAY, &sh->state);
}
}
/*
@@ -5827,11 +5827,11 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
md_write_inc(mddev, bi);
sh->overwrite_disks++;
}
spin_unlock_irq(&sh->stripe_lock);
if (conf->mddev->bitmap) {
- sh->bm_seq = conf->seq_flush + 1;
+ sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
set_bit(STRIPE_BIT_DELAY, &sh->state);
}
set_bit(STRIPE_HANDLE, &sh->state);
clear_bit(STRIPE_DELAYED, &sh->state);
@@ -6877,16 +6877,17 @@ static void raid5d(struct md_thread *thread)
clear_bit(R5_DID_ALLOC, &conf->cache_state);
if (
!list_empty(&conf->bitmap_list)) {
/* Now is a good time to flush some bitmap updates */
- conf->seq_flush++;
+ int seq = conf->seq_flush + 1;
+ WRITE_ONCE(conf->seq_flush, seq);
spin_unlock_irq(&conf->device_lock);
if (md_bitmap_enabled(mddev, true))
mddev->bitmap_ops->unplug(mddev, true);
spin_lock_irq(&conf->device_lock);
- conf->seq_write = conf->seq_flush;
+ conf->seq_write = seq;
activate_bit_delay(conf, conf->temp_inactive_list);
}
raid5_activate_delayed(conf);
while ((bio = remove_bio_from_retry(conf, &offset))) {
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] md/raid5: protect bitmap batch counters aka seq_flush/seq_write
From: Chen Cheng @ 2026-06-17 13:55 UTC (permalink / raw)
To: sashiko-reviews, Chen Cheng; +Cc: yukuai, linux-raid
In-Reply-To: <20260616133034.015501F000E9@smtp.kernel.org>
在 2026/6/16 21:30, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Missing full memory barriers between the bitmap update/scan and `seq_flush` access allows torn writes.
> --
>
> commit 45ef15a300eccd14518bc90e254146a73a49bf51
> Author: Chen Cheng <chencheng@fnnas.com>
>
> md/raid5: protect bitmap batch counters aka seq_flush/seq_write
>
> This commit attempts to fix a KCSAN data-race report on conf->seq_flush
> in the raid5 code by introducing READ_ONCE() and WRITE_ONCE().
>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index d0d7f96fd7cd9..ffc89ebbc4857 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -3538,7 +3538,7 @@ static void __add_stripe_bio(struct stripe_head *sh, struct bio *bi,
>> sh->dev[dd_idx].sector);
>>
>> if (conf->mddev->bitmap && firstwrite && !sh->batch_head) {
>> - sh->bm_seq = conf->seq_flush+1;
>> + sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
> [Severity: Critical]
> This is a pre-existing issue, but does this lockless synchronization require
> a full memory barrier (smp_mb()) before reading seq_flush?
>
> The producer sequence updates the in-memory bitmap (which drops a lock,
> providing only a Store-Release barrier) and then reads seq_flush. Because
> there is no full memory barrier between the store to the bitmap and the load
> of seq_flush, weakly-ordered CPUs (and even x86, which allows Store-Load
> reordering) can reorder the read of seq_flush before the store to the
> bitmap becomes globally visible.
>
> Could this lead to silent data corruption on a crash if the consumer sees
> the old bitmap state but the producer assigns the stripe to the current
> batch?
barrier provide a tool to promise "the things before this statement all
done", and
I don't think I need such strong promise.
What I need is, "all cpu cache coherence for accesses variable
seq_flush", so
READ_ONCE/WRITE_ONCE is suitable for this scenario.
>> set_bit(STRIPE_BIT_DELAY, &sh->state);
>> }
>> }
> [ ... ]
>
>> @@ -5755,7 +5755,7 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
>> }
>> spin_unlock_irq(&sh->stripe_lock);
>> if (conf->mddev->bitmap) {
>> - sh->bm_seq = conf->seq_flush + 1;
>> + sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
> [Severity: Critical]
> This is a pre-existing issue, but the same missing full memory barrier
> pattern appears here in the discard path as well.
>
>> set_bit(STRIPE_BIT_DELAY, &sh->state);
>> }
>>
> [ ... ]
>
>> @@ -6805,12 +6805,13 @@ static void raid5d(struct md_thread *thread)
>> if (
>> !list_empty(&conf->bitmap_list)) {
>> /* Now is a good time to flush some bitmap updates */
>> - conf->seq_flush++;
>> + int seq = READ_ONCE(conf->seq_flush) + 1;
>> + WRITE_ONCE(conf->seq_flush, seq);
> [Severity: Critical]
> This isn't a bug introduced by this patch, but does the consumer side
> also rely dangerously on implicit barriers?
>
> The consumer (raid5d) writes seq_flush and relies on an implicit full
> barrier deep inside unplug() (via test_and_clear_bit) to order the
> subsequent bitmap scan.
>
> If the producer's missing barrier allows the read of seq_flush to be
> reordered, the consumer might flush the old batch and advance seq_write,
> unblocking the producer's stripe to be written to disk without its
> write-intent bitmap bit being flushed.
>
>> spin_unlock_irq(&conf->device_lock);
>> if (md_bitmap_enabled(mddev, true))
>> mddev->bitmap_ops->unplug(mddev, true);
>> spin_lock_irq(&conf->device_lock);
>> - conf->seq_write = conf->seq_flush;
>> + WRITE_ONCE(conf->seq_write, seq);
>> activate_bit_delay(conf, conf->temp_inactive_list);
>> }
>> raid5_activate_delayed(conf);
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: David Laight @ 2026-06-17 10:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Eric Biggers, Andrew Morton, linux-kernel, linux-crypto, x86,
linux-raid
In-Reply-To: <20260617055653.GB19218@lst.de>
On Wed, 17 Jun 2026 07:56:53 +0200
Christoph Hellwig <hch@lst.de> wrote:
> Can use the xor: prefix used for all other commits to lib/raid/xor?
>
> > 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%
>
> On my Zen 5 mobile (AMD Ryzen AI 7 PRO 350) both the existing
> AVX2 and this AVX512 code give numbers in the 200+ GB/s range. Not
> sure if is just the different benchmarking or something else going on.
I'd expect benchmarking of the xor loop to show that it is doing
2 memory reads every clock.
At 5GHz (I'm sure my zen5 will reach that single threaded) that
is 320 GB/s for src_cnt == 1 and 160 GB/s for src_cnt == 3.
200 GB/s with 32 byte cache reads would need a 200/32 = 6GHz cpu
just for the reads.
But I expect Eric is benchmarking more code and may be limited
by data cache refills.
>
> FYI, one or 2 sources are basically useless as they RAID5 configs
> that have no benefits over simple mirroring and thus the numbers
> aren't too interesting.
With three disks you xor two buffers (src_count == 1) to get the parity
to write to the third - so that is a valid RAID5 config.
>
> > +DO_XOR_BLOCKS(avx512_inner, xor_avx512_2, xor_avx512_3, xor_avx512_4,
> > + xor_avx512_5);
>
> Is there really much of a benefit of doing the historic DO_XOR_BLOCKS
> vs doing the loop manually? Especially as the common cases for a
> modern RAID will usually loop over more disks than this was built
> for. I.e., in practice one or two source buffers only happen at the
> end of a loop over more disks.
I stopped looking at what was being tested at that point :-)
David
^ permalink raw reply
* Re: [bug report] raid5-ppl: PPL support for disks with write-back cache enabled
From: Dan Carpenter @ 2026-06-17 7:04 UTC (permalink / raw)
To: Tomasz Majchrzak; +Cc: linux-raid
In-Reply-To: <ajJF2wKYWRk4GGCK@stanley.mountain>
On Wed, Jun 17, 2026 at 09:59:39AM +0300, Dan Carpenter wrote:
> This code is nine years old, so what I like to do is add it to the KTODO
> in case anyone wants to fix it.
>
> KTODO: Fix use after free in ppl_do_flush()
>
> Hello Tomasz Majchrzak,
>
> Commit 1532d9e87e8b ("raid5-ppl: PPL support for disks with
> write-back cache enabled") from Dec 27, 2017 (linux-next), leads to
> the following Smatch static checker warning:
>
> drivers/md/raid5-ppl.c:646 ppl_do_flush()
> warn: 'io' was already freed. (line 647)
>
> drivers/md/raid5-ppl.c
> 608 static void ppl_do_flush(struct ppl_io_unit *io)
> 609 {
> 610 struct ppl_log *log = io->log;
> 611 struct ppl_conf *ppl_conf = log->ppl_conf;
> 612 struct r5conf *conf = ppl_conf->mddev->private;
> 613 int raid_disks = conf->raid_disks;
> 614 int flushed_disks = 0;
> 615 int i;
> 616
> 617 atomic_set(&io->pending_flushes, raid_disks);
> 618
> 619 for_each_set_bit(i, &log->disk_flush_bitmap, raid_disks) {
> 620 struct md_rdev *rdev;
> 621 struct block_device *bdev = NULL;
> 622
> 623 rdev = conf->disks[i].rdev;
> 624 if (rdev && !test_bit(Faulty, &rdev->flags))
> 625 bdev = rdev->bdev;
> 626
> 627 if (bdev) {
> 628 struct bio *bio;
> 629
> 630 bio = bio_alloc_bioset(bdev, 0,
> 631 REQ_OP_WRITE | REQ_PREFLUSH,
> 632 GFP_NOIO, &ppl_conf->flush_bs);
> 633 bio->bi_private = io;
> 634 bio->bi_end_io = ppl_flush_endio;
> 635
> 636 pr_debug("%s: dev: %ps\n", __func__, bio->bi_bdev);
> 637
> 638 submit_bio(bio);
> 639 flushed_disks++;
> 640 }
> 641 }
> 642
> 643 log->disk_flush_bitmap = 0;
> 644
> 645 for (i = flushed_disks ; i < raid_disks; i++) {
> --> 646 if (atomic_dec_and_test(&io->pending_flushes))
> 647 ppl_io_unit_finished(io);
>
> The ppl_io_unit_finished() function frees "io" so probably there is
> supposed to be a statement after it.
This sentence a word missing. Probably there is supposed to be a *break*
statement.
regards,
dan carpenter
^ permalink raw reply
* [bug report] raid5-ppl: PPL support for disks with write-back cache enabled
From: Dan Carpenter @ 2026-06-17 6:59 UTC (permalink / raw)
To: Tomasz Majchrzak; +Cc: linux-raid
This code is nine years old, so what I like to do is add it to the KTODO
in case anyone wants to fix it.
KTODO: Fix use after free in ppl_do_flush()
Hello Tomasz Majchrzak,
Commit 1532d9e87e8b ("raid5-ppl: PPL support for disks with
write-back cache enabled") from Dec 27, 2017 (linux-next), leads to
the following Smatch static checker warning:
drivers/md/raid5-ppl.c:646 ppl_do_flush()
warn: 'io' was already freed. (line 647)
drivers/md/raid5-ppl.c
608 static void ppl_do_flush(struct ppl_io_unit *io)
609 {
610 struct ppl_log *log = io->log;
611 struct ppl_conf *ppl_conf = log->ppl_conf;
612 struct r5conf *conf = ppl_conf->mddev->private;
613 int raid_disks = conf->raid_disks;
614 int flushed_disks = 0;
615 int i;
616
617 atomic_set(&io->pending_flushes, raid_disks);
618
619 for_each_set_bit(i, &log->disk_flush_bitmap, raid_disks) {
620 struct md_rdev *rdev;
621 struct block_device *bdev = NULL;
622
623 rdev = conf->disks[i].rdev;
624 if (rdev && !test_bit(Faulty, &rdev->flags))
625 bdev = rdev->bdev;
626
627 if (bdev) {
628 struct bio *bio;
629
630 bio = bio_alloc_bioset(bdev, 0,
631 REQ_OP_WRITE | REQ_PREFLUSH,
632 GFP_NOIO, &ppl_conf->flush_bs);
633 bio->bi_private = io;
634 bio->bi_end_io = ppl_flush_endio;
635
636 pr_debug("%s: dev: %ps\n", __func__, bio->bi_bdev);
637
638 submit_bio(bio);
639 flushed_disks++;
640 }
641 }
642
643 log->disk_flush_bitmap = 0;
644
645 for (i = flushed_disks ; i < raid_disks; i++) {
--> 646 if (atomic_dec_and_test(&io->pending_flushes))
647 ppl_io_unit_finished(io);
The ppl_io_unit_finished() function frees "io" so probably there is
supposed to be a statement after it. The atomic_dec_and_test() will
underflow on subsequent iterations through the loop which is normally
harmless. We may want to convert this to refcount_t so that any
underflows cause a WARN().
648 }
649 }
This email is a free service from the Smatch-CI project [smatch.sf.net].
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH 2/2] xor/kunit: add a benchmark
From: sashiko-bot @ 2026-06-17 6:00 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-raid, yukuai
In-Reply-To: <20260617054416.3841334-3-hch@lst.de>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The elapsed benchmark time `t` (a 64-bit nanosecond value) is passed as a 32-bit divisor to `div_u64()`, leading to implicit truncation for runs over 4.29 seconds, resulting in totally incorrect throughput metrics or a potential divide-by-zero kernel panic.
- [Medium] Executing a massive 1,000-iteration XOR processing loop synchronously with preemption disabled can cause RCU CPU stall warnings and softlockups on slower test environments.
- [Medium] The throughput computation calculates Gigabytes per second (GB/s) using integer division, which unconditionally truncates any throughput slower than 1 GB/s down to 0 GB/s.
--
commit facfbbbac101882a52a1c7a2e6d86cb1173448ed
Author: Christoph Hellwig <hch@lst.de>
xor/kunit: add a benchmark
This commit adds a benchmark to test the XOR functions for more representative
block sizes and numbers of disks. It includes 4k and 16k sizes, as increasing
the test buffer size further would increase the functional kunit test runtime
too much.
> diff --git a/lib/raid/xor/tests/xor_kunit.c b/lib/raid/xor/tests/xor_kunit.c
> index 659ae3edbc25b..5939b78b39446 100644
> --- a/lib/raid/xor/tests/xor_kunit.c
> +++ b/lib/raid/xor/tests/xor_kunit.c
[ ... ]
> @@ -125,8 +125,71 @@ static void xor_test(struct kunit *test)
> + for (j = 0; j < ARRAY_SIZE(len_to_test); j++) {
> + unsigned int len = len_to_test[j];
> + const unsigned long num_iters = 1000;
> +
> + KUNIT_ASSERT_GT(test, len, 0);
> + KUNIT_ASSERT_LE(test, len, XOR_KUNIT_MAX_BYTES);
> +
> + preempt_disable();
> + t = ktime_get();
> + for (l = 0; l < num_iters; l++)
> + xor_gen(test_dest, test_buffers, nr, len);
> + t = ktime_get_ns() - t;
> + preempt_enable();
[Severity: Medium]
Will this massive 1,000-iteration XOR loop cause RCU CPU stall warnings or
trigger the softlockup watchdog on slower environments?
In the worst-case configuration, this processes 512MB of memory synchronously
with preemption completely disabled. On unaccelerated CI environments or
low-power CPUs, processing this amount of data can take tens of seconds, which
violates latency guarantees.
Should preemption be enabled periodically, or the number of iterations scaled
dynamically?
> +
> + speed[j] = div_u64((u64)len * num_iters * nr, t);
[Severity: High]
Does this code silently truncate the 64-bit elapsed time t when dividing?
div_u64() expects a 32-bit unsigned integer as its divisor. The elapsed time
t is a 64-bit nanosecond value. If the execution takes longer than 4.29
seconds (U32_MAX nanoseconds), the upper 32 bits of t will be implicitly
truncated, corrupting the calculation.
Worse, if the lower 32 bits happen to truncate to exactly 0, this will trigger
a division by zero kernel panic.
Should this use div64_u64() instead?
[Severity: Medium]
Will this integer division unconditionally truncate any throughput slower than
1 GB/s down to 0 GB/s?
Mathematically, computing bytes per nanosecond is directly equivalent to
computing GB/s. Because this uses integer division without scaling the
numerator, any throughput below 1 GB/s will evaluate to exactly 0. On
processors without SIMD acceleration or in VMs, this might falsely log 0 GB/s.
Could the numerator be multiplied by 1,000 to output MB/s and preserve useful
precision?
> + }
> +
> + static_assert(ARRAY_SIZE(len_to_test) == 2);
> + kunit_info(test, "%3u disks:\t%5llu GB/s\t%5llu GB/s\n",
> + nr, speed[0], speed[1]);
> + }
> +}
> +#endif /* CONFIG_XOR_BENCHMARK */
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617054416.3841334-1-hch@lst.de?part=2
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Christoph Hellwig @ 2026-06-17 5:56 UTC (permalink / raw)
To: Eric Biggers
Cc: Andrew Morton, linux-kernel, Christoph Hellwig, linux-crypto, x86,
David Laight, linux-raid
In-Reply-To: <20260615190338.26581-1-ebiggers@kernel.org>
Can use the xor: prefix used for all other commits to lib/raid/xor?
> 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%
On my Zen 5 mobile (AMD Ryzen AI 7 PRO 350) both the existing
AVX2 and this AVX512 code give numbers in the 200+ GB/s range. Not
sure if is just the different benchmarking or something else going on.
FYI, one or 2 sources are basically useless as they RAID5 configs
that have no benefits over simple mirroring and thus the numbers
aren't too interesting.
> +DO_XOR_BLOCKS(avx512_inner, xor_avx512_2, xor_avx512_3, xor_avx512_4,
> + xor_avx512_5);
Is there really much of a benefit of doing the historic DO_XOR_BLOCKS
vs doing the loop manually? Especially as the common cases for a
modern RAID will usually loop over more disks than this was built
for. I.e., in practice one or two source buffers only happen at the
end of a loop over more disks.
^ permalink raw reply
* Re: [PATCH v2] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Christoph Hellwig @ 2026-06-17 5:52 UTC (permalink / raw)
To: Eric Biggers
Cc: David Laight, Andrew Morton, linux-kernel, Christoph Hellwig,
linux-crypto, x86, linux-raid
In-Reply-To: <20260615184435.GA17731@quark>
On Mon, Jun 15, 2026 at 11:44:35AM -0700, Eric Biggers wrote:
> > Doesn't zen4 only have a 256bit bus between the cpu and cache?
> > So avx512 reads take two clocks.
> > Since this is memory limited it is unlikely to run faster than the
> > avx256 version.
>
> On AMD Genoa (Zen 4 server processor), the AVX-512 code added by this
> patch is indeed about the same speed as the existing AVX-2 code.
The same is true for Zen 5 mobile which has the same AVX-512 limitations.
I don't think it's the bus width, but I'll leave the details to the
experts.
>
> > OTOH if it doesn't cause down-clocking as well then it won't be slower.
>
> Yes, as far as I know that's not an issue on AMD processors, even Zen 4.
> The "avoid AVX-512 due to downclocking" rule is historical guidance for
> Intel processors that had a bad implementation of AVX-512. There's no
> reason to exclude Zen 4 from executing AVX-512 optimized code. At worst
> it will just be the same, as we're seeing here.
It does not cause down clocking. But for some of the more complicated
code I've seen AVX512 being significantly slower than AVX2 on these.
So we need to watch out and not automatically assume AVX512 is faster.
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Christoph Hellwig @ 2026-06-17 5:44 UTC (permalink / raw)
To: Dave Hansen
Cc: Borislav Petkov, Eric Biggers, Richard Weinberger, x86,
Christoph Hellwig, linux-crypto, David Laight, linux-raid,
Andrew Morton, linux-kernel, linux-um
In-Reply-To: <a832eee3-55ec-4cf4-907f-346ff98870ca@intel.com>
On Mon, Jun 15, 2026 at 05:29:58PM -0700, Dave Hansen wrote:
> On 6/15/26 16:53, Borislav Petkov wrote:
> >
> >> In any case, I'd like these to go away:
> >>
> >> $ git grep cpu_has_xfeatures | wc -l
> >> 31
> > Yeah, all in crypto. I can certainly see why.
> >
> > @dhansen, any other thoughts?
>
> If we can get rid of cpu_has_xfeatures(), I'm all for it. I'm not quite
> sure how the code would look so I'm reserving judgement until I see the
> patches. But it's worth a try.
I think the most important part is to be consistent. Either use it
everywhere or not at all.
^ permalink raw reply
* [PATCH 2/2] xor/kunit: add a benchmark
From: Christoph Hellwig @ 2026-06-17 5:44 UTC (permalink / raw)
To: Andrew Morton; +Cc: Eric Biggers, linux-kernel, linux-raid
In-Reply-To: <20260617054416.3841334-1-hch@lst.de>
Add a benchmark to test the XOR functions for more representative block
sizes and numbers of disks. Including 64k would be useful here, but
increasing the test buffer size increases the runtime of the functional
kunit test too much unfortunately.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
lib/raid/Kconfig | 6 ++++
lib/raid/xor/tests/xor_kunit.c | 63 ++++++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+)
diff --git a/lib/raid/Kconfig b/lib/raid/Kconfig
index 5ab2b0a7be4c..034969d240c6 100644
--- a/lib/raid/Kconfig
+++ b/lib/raid/Kconfig
@@ -28,3 +28,9 @@ config XOR_KUNIT_TEST
This is intended to help people writing architecture-specific
optimized versions. If unsure, say N.
+
+config XOR_BENCHMARK
+ bool "Benchmark for xor_gen"
+ depends on XOR_KUNIT_TEST
+ help
+ Include benchmarks in the KUnit test suite for xor_gen.
diff --git a/lib/raid/xor/tests/xor_kunit.c b/lib/raid/xor/tests/xor_kunit.c
index 659ae3edbc25..5939b78b3944 100644
--- a/lib/raid/xor/tests/xor_kunit.c
+++ b/lib/raid/xor/tests/xor_kunit.c
@@ -125,8 +125,71 @@ static void xor_test(struct kunit *test)
}
}
+#ifdef CONFIG_XOR_BENCHMARK
+static void xor_benchmark(struct kunit *test)
+{
+ static const unsigned int nr_to_test[] = {
+ 4, 5, 6, 7, 8, 10, 12, 15, 16, 32,
+ };
+ static const unsigned int len_to_test[] = {
+ SZ_4K, SZ_16K,
+ };
+ unsigned int i, j, l;
+ u64 t;
+
+ /* warm-up */
+ for (i = 0; i < ARRAY_SIZE(nr_to_test); i++) {
+ for (j = 0; j < ARRAY_SIZE(len_to_test); j++) {
+ for (l = 0; l < 10; l++) {
+ xor_gen(test_dest, test_buffers, nr_to_test[i],
+ len_to_test[j]);
+ }
+ }
+ }
+
+ /*
+ * Preferably this would be a loop over len_to_test, but the kunit
+ * logging always adds a newline to each logged format string.
+ */
+ static_assert(ARRAY_SIZE(len_to_test) == 2);
+ kunit_info(test, " \t%5u bytes\t%5u bytes\n",
+ len_to_test[0], len_to_test[1]);
+
+ for (i = 0; i < ARRAY_SIZE(nr_to_test); i++) {
+ unsigned int nr = nr_to_test[i];
+ u64 speed[ARRAY_SIZE(len_to_test)];
+
+ KUNIT_ASSERT_LE(test, nr, XOR_KUNIT_MAX_BUFFERS);
+
+ for (j = 0; j < ARRAY_SIZE(len_to_test); j++) {
+ unsigned int len = len_to_test[j];
+ const unsigned long num_iters = 1000;
+
+ KUNIT_ASSERT_GT(test, len, 0);
+ KUNIT_ASSERT_LE(test, len, XOR_KUNIT_MAX_BYTES);
+
+ preempt_disable();
+ t = ktime_get();
+ for (l = 0; l < num_iters; l++)
+ xor_gen(test_dest, test_buffers, nr, len);
+ t = ktime_get_ns() - t;
+ preempt_enable();
+
+ speed[j] = div_u64((u64)len * num_iters * nr, t);
+ }
+
+ static_assert(ARRAY_SIZE(len_to_test) == 2);
+ kunit_info(test, "%3u disks:\t%5llu GB/s\t%5llu GB/s\n",
+ nr, speed[0], speed[1]);
+ }
+}
+#endif /* CONFIG_XOR_BENCHMARK */
+
static struct kunit_case xor_test_cases[] = {
KUNIT_CASE(xor_test),
+#ifdef CONFIG_XOR_BENCHMARK
+ KUNIT_CASE(xor_benchmark),
+#endif
{},
};
--
2.53.0
^ permalink raw reply related
* [PATCH 1/2] xor/kunit: fix a spelling error
From: Christoph Hellwig @ 2026-06-17 5:44 UTC (permalink / raw)
To: Andrew Morton; +Cc: Eric Biggers, linux-kernel, linux-raid
In-Reply-To: <20260617054416.3841334-1-hch@lst.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
lib/raid/xor/tests/xor_kunit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/raid/xor/tests/xor_kunit.c b/lib/raid/xor/tests/xor_kunit.c
index 0c2a3a420bf9..659ae3edbc25 100644
--- a/lib/raid/xor/tests/xor_kunit.c
+++ b/lib/raid/xor/tests/xor_kunit.c
@@ -85,7 +85,7 @@ static void xor_test(struct kunit *test)
xor_generate_random_data();
/*
- * If we're not using the entire buffer size, inject randomize
+ * If we're not using the entire buffer size, inject randomized
* alignment into the buffer.
*/
max_alignment = XOR_KUNIT_MAX_BYTES - len;
--
2.53.0
^ permalink raw reply related
* xor: add a kunit benchmark
From: Christoph Hellwig @ 2026-06-17 5:44 UTC (permalink / raw)
To: Andrew Morton; +Cc: Eric Biggers, linux-kernel, linux-raid
From: Eric Biggers <ebiggers@kernel.org>
Hi all,
this series adds a benchmark modelled after the CRC benchmark for to the
xor kunit test.
This is based on current Linus' master so probably needs a minor rebase
for Kconfig conflicts once the raid6 series is merged.
^ permalink raw reply
* Re: [PATCH] md/raid5: protect bitmap batch counters aka seq_flush/seq_write
From: sashiko-bot @ 2026-06-16 13:30 UTC (permalink / raw)
To: Chen Cheng; +Cc: yukuai, linux-raid
In-Reply-To: <20260616131310.3790554-1-chencheng@fnnas.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Missing full memory barriers between the bitmap update/scan and `seq_flush` access allows torn writes.
--
commit 45ef15a300eccd14518bc90e254146a73a49bf51
Author: Chen Cheng <chencheng@fnnas.com>
md/raid5: protect bitmap batch counters aka seq_flush/seq_write
This commit attempts to fix a KCSAN data-race report on conf->seq_flush
in the raid5 code by introducing READ_ONCE() and WRITE_ONCE().
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index d0d7f96fd7cd9..ffc89ebbc4857 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -3538,7 +3538,7 @@ static void __add_stripe_bio(struct stripe_head *sh, struct bio *bi,
> sh->dev[dd_idx].sector);
>
> if (conf->mddev->bitmap && firstwrite && !sh->batch_head) {
> - sh->bm_seq = conf->seq_flush+1;
> + sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
[Severity: Critical]
This is a pre-existing issue, but does this lockless synchronization require
a full memory barrier (smp_mb()) before reading seq_flush?
The producer sequence updates the in-memory bitmap (which drops a lock,
providing only a Store-Release barrier) and then reads seq_flush. Because
there is no full memory barrier between the store to the bitmap and the load
of seq_flush, weakly-ordered CPUs (and even x86, which allows Store-Load
reordering) can reorder the read of seq_flush before the store to the
bitmap becomes globally visible.
Could this lead to silent data corruption on a crash if the consumer sees
the old bitmap state but the producer assigns the stripe to the current
batch?
> set_bit(STRIPE_BIT_DELAY, &sh->state);
> }
> }
[ ... ]
> @@ -5755,7 +5755,7 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
> }
> spin_unlock_irq(&sh->stripe_lock);
> if (conf->mddev->bitmap) {
> - sh->bm_seq = conf->seq_flush + 1;
> + sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
[Severity: Critical]
This is a pre-existing issue, but the same missing full memory barrier
pattern appears here in the discard path as well.
> set_bit(STRIPE_BIT_DELAY, &sh->state);
> }
>
[ ... ]
> @@ -6805,12 +6805,13 @@ static void raid5d(struct md_thread *thread)
> if (
> !list_empty(&conf->bitmap_list)) {
> /* Now is a good time to flush some bitmap updates */
> - conf->seq_flush++;
> + int seq = READ_ONCE(conf->seq_flush) + 1;
> + WRITE_ONCE(conf->seq_flush, seq);
[Severity: Critical]
This isn't a bug introduced by this patch, but does the consumer side
also rely dangerously on implicit barriers?
The consumer (raid5d) writes seq_flush and relies on an implicit full
barrier deep inside unplug() (via test_and_clear_bit) to order the
subsequent bitmap scan.
If the producer's missing barrier allows the read of seq_flush to be
reordered, the consumer might flush the old batch and advance seq_write,
unblocking the producer's stripe to be written to disk without its
write-intent bitmap bit being flushed.
> spin_unlock_irq(&conf->device_lock);
> if (md_bitmap_enabled(mddev, true))
> mddev->bitmap_ops->unplug(mddev, true);
> spin_lock_irq(&conf->device_lock);
> - conf->seq_write = conf->seq_flush;
> + WRITE_ONCE(conf->seq_write, seq);
> activate_bit_delay(conf, conf->temp_inactive_list);
> }
> raid5_activate_delayed(conf);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616131310.3790554-1-chencheng@fnnas.com?part=1
^ permalink raw reply
* [PATCH] md/raid5: protect bitmap batch counters aka seq_flush/seq_write
From: Chen Cheng @ 2026-06-16 13:13 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, linux-kernel
From: Chen Cheng <chencheng@fnnas.com>
kcsan detect race :
- raid5d() closes the current bitmap batch by updating
conf->seq_flush under conf->device_lock.
- __add_stripe_bio() read conf->seq_flush without that
lock when assigning sh->bm_seq.
so, protect seq_flush/seq_write by READ_ONCE()/WRITE_ONCE().
re-explain the stripe batch sequence number update flow:
1. sh->bm_seq declare which batch number the stripe belongs to
when perform bitmap-related write.
==> bm_seq = seq_flush+1
2. stripe be handled,
* if sh->bm_seq - conf->seq_write > 0, means the
batch stripes **newer than** the last written
batch, it cannot proceed yet, queued on bitmap_list.
* otherwise , has already proceed.
3. raid5d() `++seq_flush` to closes the current batch, means
* no more stripes join that old batch
* just-closed batch ready to write-out to disk
4. raid5d() calls bitmap hooks unplug() or writeout, then,
`++seq_write` to the same as bm_seq.
- seq_flush - for producer, to close batches.
- seq_write - for consumer, the checkpoint number.
the report:
====================================
BUG: KCSAN: data-race in __add_stripe_bio / raid5d
write to 0xffff88ba5625d470 of 4 bytes by task 82401 on cpu 0:
raid5d+0x1d9/0xba0
[.....]
read to 0xffff88ba5625d470 of 4 bytes by task 82421 on cpu 8:
__add_stripe_bio+0x332/0x400
raid5_make_request+0x6ac/0x2930
md_handle_request+0x4a2/0xa40
md_submit_bio+0x109/0x1a0
__submit_bio+0x2ec/0x390
[.....]
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/raid5.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index a320b71d7117..f3c2959b5606 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -3536,11 +3536,11 @@ static void __add_stripe_bio(struct stripe_head *sh, struct bio *bi,
pr_debug("added bi b#%llu to stripe s#%llu, disk %d, logical %llu\n",
(*bip)->bi_iter.bi_sector, sh->sector, dd_idx,
sh->dev[dd_idx].sector);
if (conf->mddev->bitmap && firstwrite && !sh->batch_head) {
- sh->bm_seq = conf->seq_flush+1;
+ sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
set_bit(STRIPE_BIT_DELAY, &sh->state);
}
}
/*
@@ -5827,11 +5827,11 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
md_write_inc(mddev, bi);
sh->overwrite_disks++;
}
spin_unlock_irq(&sh->stripe_lock);
if (conf->mddev->bitmap) {
- sh->bm_seq = conf->seq_flush + 1;
+ sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
set_bit(STRIPE_BIT_DELAY, &sh->state);
}
set_bit(STRIPE_HANDLE, &sh->state);
clear_bit(STRIPE_DELAYED, &sh->state);
@@ -6877,16 +6877,17 @@ static void raid5d(struct md_thread *thread)
clear_bit(R5_DID_ALLOC, &conf->cache_state);
if (
!list_empty(&conf->bitmap_list)) {
/* Now is a good time to flush some bitmap updates */
- conf->seq_flush++;
+ int seq = READ_ONCE(conf->seq_flush) + 1;
+ WRITE_ONCE(conf->seq_flush, seq);
spin_unlock_irq(&conf->device_lock);
if (md_bitmap_enabled(mddev, true))
mddev->bitmap_ops->unplug(mddev, true);
spin_lock_irq(&conf->device_lock);
- conf->seq_write = conf->seq_flush;
+ WRITE_ONCE(conf->seq_write, seq);
activate_bit_delay(conf, conf->temp_inactive_list);
}
raid5_activate_delayed(conf);
while ((bio = remove_bio_from_retry(conf, &offset))) {
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: David Laight @ 2026-06-16 8:13 UTC (permalink / raw)
To: Eric Biggers
Cc: x86, Christoph Hellwig, linux-crypto, linux-raid, Andrew Morton,
linux-kernel
In-Reply-To: <20260615201050.GB1764@quark>
On Mon, 15 Jun 2026 13:10:50 -0700
Eric Biggers <ebiggers@kernel.org> wrote:
> On Mon, Jun 15, 2026 at 12:03:38PM -0700, Eric Biggers wrote:
> > Note: for now I omitted the cpu_has_xfeatures() check that the AVX-512
> > optimized crypto and CRC code does, since it's not implemented on
> > User-Mode Linux and it's never been present in the RAID6 code either.
>
> By the way, Sashiko keeps complaining about this decision.
>
> Maybe the x86 maintainers have some advice here?
>
> For context: on x86 processors, executing AVX or AVX512 instructions
> requires not just that the CPU supports the feature, but also that the
> operating system has set certain bits in XCR0. For example all EVEX
> coded instructions (i.e. AVX-512) require XCR0=111xx111b. (See Intel
> manual "2.6.11.1 State Dependent #UD".)
>
> Therefore most of the kernel's AVX and AVX512 optimized code checks not
> just X86_FEATURE_AVX* but also calls cpu_has_xfeatures() to check XCR0.
>
> But "most" isn't all. The RAID6 code for example doesn't check
> cpu_has_xfeatures(). So if you e.g. boot a kernel in QEMU using
> "-cpu max,xsave=off", it already crashes when the RAID6 code does its
> boot-time benchmark.
>
> Part of the reason for that omission probably is that UML doesn't
> provide an implementation of cpu_has_xfeatures(). And the x86 RAID (XOR
> and RAID6) code is enabled on UML.
>
> It could be implemented for UML by using the xgetbv instruction, like
> what userspace programs do. (We'd also need to copy the XFEATURE_MASK_*
> constants, as UML can't include arch/x86/include/asm/fpu/types.h)
>
> But I wanted to ask: do we really care about the case where features are
> "supported" but their XCR0 bits aren't set? Perhaps the kernel just
> doesn't/shouldn't support weird cases like "-cpu max,xsave=off"?
I think that case definitely matters for userspace.
Isn't it what happens when you run an old OS on a new cpu?
I remember cases where people were compiling programs that used AVX
(possibly from gcc's cpu=native) but the os hadn't been updated to
actually save the relevant registers.
The programs 'sort of worked' until a process switch failed to preserve
the registers.
So the check you need to do is looking at XCR0 rather than anything else.
>
> If this case indeed needs to be handled, could we make things easier for
> the kernel's AVX and AVX-512 optimized code? Currently AVX-512 needs:
>
> if (boot_cpu_has(X86_FEATURE_AVX512F) &&
> cpu_has_xfeatures(XFEATURE_MASK_FP | XFEATURE_MASK_SSE |
> XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512, NULL))
>
> How about we make X86_FEATURE_AVX512F depend on XCR0=111xx111, and
> X86_FEATURE_AVX depend on XCR0=xxxxx111? Then the cpu_has_xfeatures()
> check wouldn't be needed. Is there any reason not to do that?
If cpu_has_xfeatures() is checking (a copy of) XCR0 isn't it enough
to just check that XFEATURE_MASK_AVX512 is set - it doesn't make any
sense for the other bits to be clear at the same time.
If the XCR0 copy is sane/sanitised you only need to check one bit.
That would let you #define the constant to 0 if the kernel is built without
the feature and the compiler will optimise the code away.
Then the test would just be:
if (can_use_xfeature(XFEATURE_AVX512))
-- David
>
> - Eric
^ permalink raw reply
* Re: [PATCH v2] md/raid5: fix reshape deadlock while failed devices more than max degraded
From: chencheng @ 2026-06-16 4:04 UTC (permalink / raw)
To: sashiko-reviews; +Cc: yukuai, linux-raid
In-Reply-To: <20260615114929.E5BAD1F00A3A@smtp.kernel.org>
在 2026/6/15 19:49, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] Clearing STRIPE_EXPANDING prematurely in handle_failed_reshape() allows a destination stripe to be freed and reused while asynchronous DMA is still writing to its pages, leading to DMA-after-free memory corruption.
> --
>
> commit ec55b78c975b4f9f35e287b83bee72d76b92b33a
> Author: Chen Cheng <chencheng@fnnas.com>
>
> md/raid5: fix reshape deadlock while failed devices more than max degraded
>
> This patch fixes a deadlock in raid5 reshape by cleaning up stripe states
> (including destination and source stripes) when failed devices exceed the
> max degraded count.
>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index d0d7f96fd7cd9..2f4d82bd7c2e6 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -3730,6 +3730,78 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
>> md_sync_error(conf->mddev);
>> }
>>
>> +/*
>> + * handle_failed_reshape - handl failed stripes when reshape failed and
>> + * degraded devices >= max_degraded
> [ ... ]
>
>> +static void handle_failed_reshape(struct r5conf *conf, struct stripe_head *sh,
>> + struct stripe_head_state *s)
>> +{
>> + int i;
> [ ... ]
>
>> + /* release the destination stripes that are waiting to be filled */
>> + if (test_and_clear_bit(STRIPE_EXPAND_SOURCE, &sh->state)) {
>> + for (i = 0; i < sh->disks; i++) {
>> + int dd_idx;
>> + struct stripe_head *sh2;
>> + sector_t bn, sec;
>> +
>> + if (i == sh->pd_idx)
>> + continue;
>> + if (conf->level == 6 && i == sh->qd_idx)
>> + continue;
>> +
>> + bn = raid5_compute_blocknr(sh, i, 1);
>> + sec = raid5_compute_sector(conf, bn, 0, &dd_idx, NULL);
>> + sh2 = raid5_get_active_stripe(conf, NULL, sec,
>> + R5_GAS_NOBLOCK | R5_GAS_NOQUIESCE);
>> + if (!sh2)
>> + continue;
>> +
>> + if (test_and_clear_bit(STRIPE_EXPANDING, &sh2->state)) {
> [Severity: Critical]
> Could clearing STRIPE_EXPANDING prematurely here lead to a DMA-after-free?
>
> During a reshape, a destination stripe (sh2) might require data from multiple
> source stripes. STRIPE_EXPANDING normally prevents the destination stripe from
> being returned to the free list before all data is copied.
>
> If Worker 1 is handling a source stripe via handle_stripe_expansion(), it
> submits an asynchronous DMA copy to sh2's pages and then calls
> raid5_release_stripe(sh2) to drop its reference BEFORE waiting for the DMA to
> complete:
>
> drivers/md/raid5.c:handle_stripe_expansion() {
> ...
> raid5_release_stripe(sh2);
> }
> /* done submitting copies, wait for them to complete */
> async_tx_quiesce(&tx);
> ...
> }
>
> If the array concurrently fails, Worker 2 could enter handle_failed_reshape()
> for a different source stripe, look up sh2, clear its STRIPE_EXPANDING bit,
> and release it.
>
> If sh2's refcount drops to 0 without STRIPE_EXPANDING set, could it be
> added to the inactive_list and reallocated (or have its pages freed) while
> the DMA from Worker 1 is still actively writing to it?
that make sense, however I can't find a card or processer to support offload
memcpy , neither emulate, so I can't re-produce this scenario.
theoretically, the fix way I can think of is use refcnt to point-out the number
of offloading memcpy tasks, after reach to zero, the destination stripe do
cleanup, otherwise , delay source stripe handle.
>> + atomic_dec(&conf->reshape_stripes);
>> + wake_up(&conf->wait_for_reshape);
>> + md_done_sync(conf->mddev,
>> + RAID5_STRIPE_SECTORS(conf));
>> + }
>> +
>> + clear_bit(STRIPE_EXPAND_READY, &sh2->state);
>> +
>> + raid5_release_stripe(sh2);
>> + }
>> + }
>> +}
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Dave Hansen @ 2026-06-16 0:29 UTC (permalink / raw)
To: Borislav Petkov, Eric Biggers, Richard Weinberger
Cc: x86, Christoph Hellwig, linux-crypto, David Laight, linux-raid,
Andrew Morton, linux-kernel, linux-um
In-Reply-To: <20260615235318.GBajCQbuy9dBgKH8L_@fat_crate.local>
On 6/15/26 16:53, Borislav Petkov wrote:
>
>> In any case, I'd like these to go away:
>>
>> $ git grep cpu_has_xfeatures | wc -l
>> 31
> Yeah, all in crypto. I can certainly see why.
>
> @dhansen, any other thoughts?
If we can get rid of cpu_has_xfeatures(), I'm all for it. I'm not quite
sure how the code would look so I'm reserving judgement until I see the
patches. But it's worth a try.
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Borislav Petkov @ 2026-06-15 23:53 UTC (permalink / raw)
To: Eric Biggers, Richard Weinberger
Cc: x86, Christoph Hellwig, linux-crypto, David Laight, linux-raid,
Andrew Morton, linux-kernel, linux-um
In-Reply-To: <20260615212922.GA28589@quark>
On Mon, Jun 15, 2026 at 02:29:22PM -0700, Eric Biggers wrote:
> On Mon, Jun 15, 2026 at 09:16:55PM +0000, Borislav Petkov wrote:
> > On June 15, 2026 8:10:50 PM UTC, Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > >But I wanted to ask: do we really care about the case where features are
> > >"supported" but their XCR0 bits aren't set? Perhaps the kernel just
> > >doesn't/shouldn't support weird cases like "-cpu max,xsave=off"?
> > >
> >
> > Yes, our aim is to support only configurations which are actually
> > present in real hardware and not a "oh, it would be good if it did
> > that, just because..."
>
> Seems reasonable to me. Would the same apply to UML here?
Good question.
Richi?
> > >If this case indeed needs to be handled, could we make things easier for
> > >the kernel's AVX and AVX-512 optimized code? Currently AVX-512 needs:
> > >
> > > if (boot_cpu_has(X86_FEATURE_AVX512F) &&
> > > cpu_has_xfeatures(XFEATURE_MASK_FP | XFEATURE_MASK_SSE |
> > > XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512, NULL))
> > >
> > >How about we make X86_FEATURE_AVX512F depend on XCR0=111xx111, and
> > >X86_FEATURE_AVX depend on XCR0=xxxxx111? Then the cpu_has_xfeatures()
> > >check wouldn't be needed. Is there any reason not to do that?
> >
> > How do you want to accomplish that? Very early during boot on the BSP
> > you sanity-check XCR0 and clear feature flags if components are not
> > set?
>
> That would be the idea. Something similar to what
> arch/x86/kernel/cpu/cpuid-deps.c does.
Yap.
> Except that seems to only enforce the dependencies when the kernel itself is
> disabling things; if the hypervisor is broken then it just warns.
Not the kernel's problem. We deliberately don't want to maintain a zoo of
options which are not present in real hw. If HV is doing funny things, oh
well...
> In any case, I'd like these to go away:
>
> $ git grep cpu_has_xfeatures | wc -l
> 31
Yeah, all in crypto. I can certainly see why.
@dhansen, any other thoughts?
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH v2] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: David Laight @ 2026-06-15 22:57 UTC (permalink / raw)
To: Eric Biggers
Cc: Andrew Morton, linux-kernel, Christoph Hellwig, linux-crypto, x86,
linux-raid
In-Reply-To: <20260615184435.GA17731@quark>
On Mon, 15 Jun 2026 11:44:35 -0700
Eric Biggers <ebiggers@kernel.org> wrote:
> On Sun, Jun 14, 2026 at 11:16:28AM +0100, David Laight wrote:
> > On Sat, 13 Jun 2026 18:03:57 -0700
> > Eric Biggers <ebiggers@kernel.org> wrote:
...
> > Some 'not very important' comments:
> >
> > I did wonder whether moving the loop into the asm() would help.
> > gcc has a nasty habit of pessimising loops when you try to be clever.
> > It is certainly safer for tight loops like these.
>
> I originally tried leaving the loops to the compiler, but gcc unrolled
> the 1x ones by 2x, despite it having no visibility into the asm block.
> That broke the intent with the indexed addressing, since to achieve the
> unrolling it generated code that incremented the pointers.
I did suspect that might happen.
> So I just ended up moving the loop to the asm, which reliably gives us
> the code we want.
Yep...
...
> > The code should be limited by the memory reads, so the 3-argument xor and
> > the interleave of the unroll may make no difference.
>
> The unroll by 2x in the 2 and 3-buffer cases helped a little bit on
> Sapphire Rapids. I don't know exactly why, but it makes sense that
> those cases are where the loop overhead is most likely to matter.
Each iteration does 2 (or 3) reads and a write.
The cpu can do two reads and a write every clock.
However Intel cpu can only execute a branch every other clock,
so the shortest loop is two clocks.
That means you need need to unroll once to keep the memory logic busy.
The zen5 seems to be able to execute 1-clock loops, so wouldn't need
the unroll.
> > Some cpu do have constraints on the cache alignment in order to do two
> > reads per clock, but I've forgotten them and they got better before AVX-512.
> > If that were affecting this code (on the tested cpu) then I'd expect the
> > interleaved unroll would improve the _4 and -5 functions.
> > So it probably doesn't affect this code.
>
> The buffers are always 64-byte aligned here, as documented.
It is all more complex that that.
Whether you can do two reads/clock depends on whether the reads manage to
avoid needing the same buffers (etc) in the cache logic.
For instance it might not work if the addresses differ by the size of the
cache (one of Agner's books might have the answer).
(It was pretty hard to get two reads/clock on Sandy Bridge.)
Then there are some really strange effects.
On zen5 (at least on the one I've got) 'rep movsb' is very slow (setup and copy)
if (IIRC) (%di - %si) mod 4k is between 1 and 127.
The only other alignment that makes much difference is 64byte aligning %di (which
doubles throughput).
-- David
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Eric Biggers @ 2026-06-15 21:29 UTC (permalink / raw)
To: Borislav Petkov
Cc: x86, Christoph Hellwig, linux-crypto, David Laight, linux-raid,
Andrew Morton, linux-kernel
In-Reply-To: <255CAE3E-7FD3-4DC2-B3DE-46BE67EF22A8@alien8.de>
On Mon, Jun 15, 2026 at 09:16:55PM +0000, Borislav Petkov wrote:
> On June 15, 2026 8:10:50 PM UTC, Eric Biggers <ebiggers@kernel.org> wrote:
> >
> >But I wanted to ask: do we really care about the case where features are
> >"supported" but their XCR0 bits aren't set? Perhaps the kernel just
> >doesn't/shouldn't support weird cases like "-cpu max,xsave=off"?
> >
>
> Yes, our aim is to support only configurations which are actually
> present in real hardware and not a "oh, it would be good if it did
> that, just because..."
Seems reasonable to me. Would the same apply to UML here?
> >If this case indeed needs to be handled, could we make things easier for
> >the kernel's AVX and AVX-512 optimized code? Currently AVX-512 needs:
> >
> > if (boot_cpu_has(X86_FEATURE_AVX512F) &&
> > cpu_has_xfeatures(XFEATURE_MASK_FP | XFEATURE_MASK_SSE |
> > XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512, NULL))
> >
> >How about we make X86_FEATURE_AVX512F depend on XCR0=111xx111, and
> >X86_FEATURE_AVX depend on XCR0=xxxxx111? Then the cpu_has_xfeatures()
> >check wouldn't be needed. Is there any reason not to do that?
>
> How do you want to accomplish that? Very early during boot on the BSP
> you sanity-check XCR0 and clear feature flags if components are not
> set?
That would be the idea. Something similar to what
arch/x86/kernel/cpu/cpuid-deps.c does. Except that seems to only
enforce the dependencies when the kernel itself is disabling things; if
the hypervisor is broken then it just warns.
In any case, I'd like these to go away:
$ git grep cpu_has_xfeatures | wc -l
31
- Eric
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Borislav Petkov @ 2026-06-15 21:16 UTC (permalink / raw)
To: Eric Biggers, x86
Cc: Christoph Hellwig, linux-crypto, David Laight, linux-raid,
Andrew Morton, linux-kernel
In-Reply-To: <20260615201050.GB1764@quark>
On June 15, 2026 8:10:50 PM UTC, Eric Biggers <ebiggers@kernel.org> wrote:
>
>But I wanted to ask: do we really care about the case where features are
>"supported" but their XCR0 bits aren't set? Perhaps the kernel just
>doesn't/shouldn't support weird cases like "-cpu max,xsave=off"?
>
Yes, our aim is to support only configurations which are actually present in real hardware and not a "oh, it would be good if it did that, just because..."
>If this case indeed needs to be handled, could we make things easier for
>the kernel's AVX and AVX-512 optimized code? Currently AVX-512 needs:
>
> if (boot_cpu_has(X86_FEATURE_AVX512F) &&
> cpu_has_xfeatures(XFEATURE_MASK_FP | XFEATURE_MASK_SSE |
> XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512, NULL))
>
>How about we make X86_FEATURE_AVX512F depend on XCR0=111xx111, and
>X86_FEATURE_AVX depend on XCR0=xxxxx111? Then the cpu_has_xfeatures()
>check wouldn't be needed. Is there any reason not to do that?
How do you want to accomplish that? Very early during boot on the BSP you sanity-check XCR0 and clear feature flags if components are not set?
Thx.
--
Small device. Typos and formatting crap
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Eric Biggers @ 2026-06-15 20:10 UTC (permalink / raw)
To: x86
Cc: Christoph Hellwig, linux-crypto, David Laight, linux-raid,
Andrew Morton, linux-kernel
In-Reply-To: <20260615190338.26581-1-ebiggers@kernel.org>
On Mon, Jun 15, 2026 at 12:03:38PM -0700, Eric Biggers wrote:
> Note: for now I omitted the cpu_has_xfeatures() check that the AVX-512
> optimized crypto and CRC code does, since it's not implemented on
> User-Mode Linux and it's never been present in the RAID6 code either.
By the way, Sashiko keeps complaining about this decision.
Maybe the x86 maintainers have some advice here?
For context: on x86 processors, executing AVX or AVX512 instructions
requires not just that the CPU supports the feature, but also that the
operating system has set certain bits in XCR0. For example all EVEX
coded instructions (i.e. AVX-512) require XCR0=111xx111b. (See Intel
manual "2.6.11.1 State Dependent #UD".)
Therefore most of the kernel's AVX and AVX512 optimized code checks not
just X86_FEATURE_AVX* but also calls cpu_has_xfeatures() to check XCR0.
But "most" isn't all. The RAID6 code for example doesn't check
cpu_has_xfeatures(). So if you e.g. boot a kernel in QEMU using
"-cpu max,xsave=off", it already crashes when the RAID6 code does its
boot-time benchmark.
Part of the reason for that omission probably is that UML doesn't
provide an implementation of cpu_has_xfeatures(). And the x86 RAID (XOR
and RAID6) code is enabled on UML.
It could be implemented for UML by using the xgetbv instruction, like
what userspace programs do. (We'd also need to copy the XFEATURE_MASK_*
constants, as UML can't include arch/x86/include/asm/fpu/types.h)
But I wanted to ask: do we really care about the case where features are
"supported" but their XCR0 bits aren't set? Perhaps the kernel just
doesn't/shouldn't support weird cases like "-cpu max,xsave=off"?
If this case indeed needs to be handled, could we make things easier for
the kernel's AVX and AVX-512 optimized code? Currently AVX-512 needs:
if (boot_cpu_has(X86_FEATURE_AVX512F) &&
cpu_has_xfeatures(XFEATURE_MASK_FP | XFEATURE_MASK_SSE |
XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512, NULL))
How about we make X86_FEATURE_AVX512F depend on XCR0=111xx111, and
X86_FEATURE_AVX depend on XCR0=xxxxx111? Then the cpu_has_xfeatures()
check wouldn't be needed. Is there any reason not to do that?
- Eric
^ permalink raw reply
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