Linux RAID subsystem development
 help / color / mirror / Atom feed
* Re: [PATCH] md/md-llbitmap: grow the page cache in place for reshape
From: Su Yue @ 2026-06-15 11:16 UTC (permalink / raw)
  To: Yu Kuai; +Cc: Song Liu, Yu Kuai, Li Nan, Xiao Ni, linux-raid, linux-kernel
In-Reply-To: <20260605091527.2463539-7-yukuai@kernel.org>

On Fri 05 Jun 2026 at 17:15, Yu Kuai <yukuai@kernel.org> wrote:

> From: Yu Kuai <yukuai@fygo.io>
>
> Use the page-control helpers to grow llbitmap's cached pages in 
> place
> for resize and later reshape preparation, instead of rebuilding 
> the
> whole cache.
>
> Signed-off-by: Yu Kuai <yukuai@fygo.io>
> ---
>  drivers/md/md-llbitmap.c | 139 
>  +++++++++++++++++++++++++++++++++++----
>  1 file changed, 127 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index 2f2896fe4d6f..91d3dec43d48 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c
> @@ -414,10 +414,23 @@ static char 
> state_machine[BitStateCount][BitmapActionCount] = {
>  		[BitmapActionClearUnwritten]	= BitUnwritten,
>  	},
>  };
>
>  static void __llbitmap_flush(struct mddev *mddev);
> +static void llbitmap_flush(struct mddev *mddev);
> +static void llbitmap_update_sb(void *data);
> +
> +static void llbitmap_resize_chunks(struct mddev *mddev, 
> sector_t blocks,
> +				   unsigned long *chunksize,
> +				   unsigned long *chunks)
>
NIT: I would like call it llbitmap_calculate_chunks.

> +{
> +	*chunks = DIV_ROUND_UP_SECTOR_T(blocks, *chunksize);
> +	while (*chunks > mddev->bitmap_info.space << SECTOR_SHIFT) {
> +		*chunksize = *chunksize << 1;
> +		*chunks = DIV_ROUND_UP_SECTOR_T(blocks, *chunksize);
> +	}
> +}
>
>  static enum llbitmap_state llbitmap_read(struct llbitmap 
>  *llbitmap, loff_t pos)
>  {
>  	unsigned int idx;
>  	unsigned int offset;
> @@ -653,10 +666,52 @@ static unsigned int 
> llbitmap_reserved_pages(struct llbitmap *llbitmap)
>  {
>  	return DIV_ROUND_UP(llbitmap->mddev->bitmap_info.space << 
>  SECTOR_SHIFT,
>  			    PAGE_SIZE);
>  }
>
> +static int llbitmap_expand_pages(struct llbitmap *llbitmap,
> +				 unsigned long chunks)
> +{
> +	struct llbitmap_page_ctl **pctl;
> +	unsigned int old_nr_pages = llbitmap->nr_pages;
> +	unsigned int nr_pages = llbitmap_used_pages(llbitmap, chunks);
> +	int i;
> +	int ret;
> +
> +	if (nr_pages <= old_nr_pages)
> +		return 0;
> +
> +	pctl = kcalloc(nr_pages, sizeof(*pctl), GFP_KERNEL);
> +	if (!pctl)
> +		return -ENOMEM;
> +
> +	if (llbitmap->pctl)
> +		memcpy(pctl, llbitmap->pctl,
> +		       array_size(old_nr_pages, sizeof(*pctl)));
> +
> +	for (i = old_nr_pages; i < nr_pages; i++) {
> +		pctl[i] = llbitmap_alloc_page_ctl(llbitmap, i);
> +		if (IS_ERR(pctl[i]))
> +			goto err_alloc_ptr;
> +	}
> +
> +	kfree(llbitmap->pctl);
> +	llbitmap->pctl = pctl;
> +	llbitmap->nr_pages = nr_pages;
> +	return 0;
> +
> +err_alloc_ptr:
> +	ret = PTR_ERR(pctl[i]);
> +	for (i--; i >= (int)old_nr_pages; i--) {
>
Confused about why not just declare i as an unsigned int?


--
Su
> +		__free_page(pctl[i]->page);
> +		percpu_ref_exit(&pctl[i]->active);
> +		kfree(pctl[i]);
> +	}
> +	kfree(pctl);
> +	return ret;
> +}
> +
>  static int llbitmap_alloc_pages(struct llbitmap *llbitmap)
>  {
>  	unsigned int used_pages = llbitmap_used_pages(llbitmap, 
>  llbitmap->chunks);
>  	unsigned int nr_pages = max(used_pages, 
>  llbitmap_reserved_pages(llbitmap));
>  	int i;
> @@ -728,10 +783,38 @@ static bool llbitmap_zero_all_disks(struct 
> llbitmap *llbitmap)
>  	}
>
>  	return true;
>  }
>
> +static void llbitmap_mark_range(struct llbitmap *llbitmap,
> +				unsigned long start,
> +				unsigned long end,
> +				enum llbitmap_state state)
> +{
> +	while (start <= end) {
> +		llbitmap_write(llbitmap, state, start);
> +		start++;
> +	}
> +}
> +
> +static int llbitmap_prepare_resize(struct llbitmap *llbitmap,
> +				   unsigned long old_chunks,
> +				   unsigned long new_chunks,
> +				   unsigned long cache_chunks)
> +{
> +	int ret;
> +
> +	llbitmap_flush(llbitmap->mddev);
> +	ret = llbitmap_expand_pages(llbitmap, cache_chunks);
> +	if (ret)
> +		return ret;
> +	if (new_chunks > old_chunks)
> +		llbitmap_mark_range(llbitmap, old_chunks, new_chunks - 1,
> +				    BitUnwritten);
> +	return 0;
> +}
> +
>  static void llbitmap_init_state(struct llbitmap *llbitmap)
>  {
>  	struct mddev *mddev = llbitmap->mddev;
>  	enum llbitmap_state state = BitUnwritten;
>  	unsigned long i;
> @@ -1024,14 +1107,14 @@ static int llbitmap_read_sb(struct 
> llbitmap *llbitmap)
>  		pr_err("md/llbitmap: %s: chunksize not a power of 2",
>  		       mdname(mddev));
>  		goto out_put_page;
>  	}
>
> -	if (chunksize < 
> DIV_ROUND_UP_SECTOR_T(mddev->resync_max_sectors,
> +	if (chunksize < DIV_ROUND_UP_SECTOR_T(sync_size,
>  					      mddev->bitmap_info.space << 
>  SECTOR_SHIFT)) {
>  		pr_err("md/llbitmap: %s: chunksize too small %lu < %llu / 
>  %lu",
> -		       mdname(mddev), chunksize, 
> mddev->resync_max_sectors,
> +		       mdname(mddev), chunksize, sync_size,
>  		       mddev->bitmap_info.space);
>  		goto out_put_page;
>  	}
>
>  	daemon_sleep = le32_to_cpu(sb->daemon_sleep);
> @@ -1169,28 +1252,60 @@ static int llbitmap_create(struct mddev 
> *mddev)
>  }
>
>  static int llbitmap_resize(struct mddev *mddev, sector_t 
>  blocks, int chunksize)
>  {
>  	struct llbitmap *llbitmap = mddev->bitmap;
> +	sector_t old_blocks = llbitmap->sync_size;
> +	unsigned long old_chunks = llbitmap->chunks;
>  	unsigned long chunks;
> +	unsigned long cache_chunks;
> +	int ret = 0;
> +	unsigned long bitmap_chunksize;
> +	bool reshape;
>
>  	if (chunksize == 0)
>  		chunksize = llbitmap->chunksize;
>
> -	/* If there is enough space, leave the chunksize unchanged. */
> -	chunks = DIV_ROUND_UP_SECTOR_T(blocks, chunksize);
> -	while (chunks > mddev->bitmap_info.space << SECTOR_SHIFT) {
> -		chunksize = chunksize << 1;
> -		chunks = DIV_ROUND_UP_SECTOR_T(blocks, chunksize);
> -	}
> +	bitmap_chunksize = chunksize;
> +	llbitmap_resize_chunks(mddev, blocks, &bitmap_chunksize, 
> &chunks);
>
> -	llbitmap->chunkshift = ffz(~chunksize);
> -	llbitmap->chunksize = chunksize;
> -	llbitmap->chunks = chunks;
> -	llbitmap->sync_size = blocks;
> +	reshape = mddev->delta_disks || mddev->new_level != 
> mddev->level ||
> +		mddev->new_layout != mddev->layout ||
> +		mddev->new_chunk_sectors != mddev->chunk_sectors;
> +	if (!reshape && bitmap_chunksize != llbitmap->chunksize)
> +		return -EOPNOTSUPP;
> +	if (blocks == old_blocks && chunks == llbitmap->chunks)
> +		return 0;
> +
> +	mutex_lock(&mddev->bitmap_info.mutex);
>
> +	cache_chunks = reshape ? max(old_chunks, chunks) : chunks;
> +	ret = llbitmap_prepare_resize(llbitmap, old_chunks, chunks, 
> cache_chunks);
> +	if (ret)
> +		goto out;
> +
> +	if (reshape) {
> +		llbitmap->reshape_sync_size = blocks;
> +		llbitmap->reshape_chunksize = bitmap_chunksize;
> +		llbitmap->reshape_chunks = chunks;
> +		llbitmap->chunks = max(old_chunks, chunks);
> +	} else {
> +		if (blocks < old_blocks && chunks < old_chunks)
> +			llbitmap_mark_range(llbitmap, chunks, old_chunks - 1,
> +					    BitUnwritten);
> +		mddev->bitmap_info.chunksize = bitmap_chunksize;
> +		llbitmap->chunks = chunks;
> +		llbitmap->sync_size = blocks;
> +		llbitmap_update_sb(llbitmap);
> +	}
> +	__llbitmap_flush(mddev);
> +	mutex_unlock(&mddev->bitmap_info.mutex);
>  	return 0;
> +
> +out:
> +	mutex_unlock(&mddev->bitmap_info.mutex);
> +	return ret;
>  }
>
>  static int llbitmap_load(struct mddev *mddev)
>  {
>  	enum llbitmap_action action = BitmapActionReload;

^ permalink raw reply

* [PATCH v2] md/raid5: fix reshape deadlock while failed devices more than max degraded
From: Chen Cheng @ 2026-06-15 11:34 UTC (permalink / raw)
  To: linux-raid, yukuai, yukuai; +Cc: chencheng, linux-kernel

From: Chen Cheng <chencheng@fnnas.com>

reshape stripe lifetime:
- start reshape ==> reshape_request():
	* get destination stripe,
	  - if need to copy source data chunks, set STRIPE_EXPANDING;
	  - or, if new regions past the old end of the array, zero-filled,
	    no need source data, set STRIPE_EXPANDING | STRIPE_READY
	* get source stripe,
	  - set STRIPE_EXPAND_SOURCE

- handle expand stripe ==> handle_stripe():
	reshape use reconstruct-write to construct stripe,
	four stages:
	1. prepare source data chunks for old geometry stripe
		- fill source stripe data by read or compute
	2. move data from old geometry source stripe to new geometry
	   destination stripe
		- source stripe clear STRIPE_EXPAND_SOURCE
		- drain data from source to destination stripe
		- mark stripe chunk as R5_Expanded|R5_UPTODATE when the
		  drain from source chunk to destionation chunk is completed
		- all stripe chunks drain are completed, then mark
		  STRIPE_EXPAND_READY
	3. calculate p/q chunks for destination stripe
		- if destination stripe does't depends on source dstripe,
		  then we can clear STRIPE_EXPANDING
	4. write-out to disks and release
		- set R5_Wantwrite|R5_Locked, writeout to disk
		- if write-out successed, clear STRIPE_EXPAND_READY, and
		  decrement reshape_stripe, call md_done_sync() to report
		  reshape progress.

1. cleanup the following kinds of **destination stripe**
	when failed device more than max degraded:
  - new regions past the old end of the array, zero-filled in place,
    requires no source data.
	(STRIPE_EXPANDING | STRIPE_EXPAND_READY)
  - prepare source data chunks already done, and writeout failed
	(STRIPE_EXPAND_READY)

2. destination stripes that need source data
	(STRIPE_EXPANDING, no STRIPE_HANDLE)
  - these kind of stripes sit idle in the stripe cache and are never seen
    by handle_stripe(). So clean up indirectly when thier source stripe
    (type 3) is processed.

3. source stripes (STRIPE_EXPAND_SOURCE)
  - hit handle_stripe() after thier member disks are markded Faulty.
  - clear STRIPE_EXPAND_SOURCE, finds and cleanup all dependent destination
    stripes that were waiting for data.
  - walks the source's data disks, compute the corresponding destination
    sector, looks up the destination stripe, and do cleanup(clear flags,
    dec counters, call md_done_sync())

Reproducer:
  - Create a 4-disk RAID5 with mdadm on top of 5 disposable test disks
    wrapped by dm targets.
  - Add the 5th device as a spare and start a 4 -> 5 reshape.
  - Wait until /sys/block/mdX/md/sync_action reports "reshape".
  - Inject failures on two members so reshape exceeds max_degraded.
  - After a few seconds, write "frozen" to /sys/block/mdX/md/sync_action.
    Before this fix, the write blocks indefinitely.

Read-error variant:
  - Use dm-dust on /dev/sd[b-f].
  - Preload bad blocks on two source members, e.g. dust0 and dust1:
      dmsetup message dust0 0 addbadblock <range>
      dmsetup message dust1 0 addbadblock <range>
  - Start reshape:
      mdadm -C /dev/mdX -e 1.2 -l 5 -n 4 -c 64 --assume-clean /dev/mapper/dust{0..3}
      mdadm --manage /dev/mdX --add /dev/mapper/dust4
      mdadm --grow /dev/mdX -n 5 --backup-file=/tmp/grow.backup &
  - Once reshape starts, enable the injected read failures:
      dmsetup message dust0 0 enable
      dmsetup message dust1 0 enable
  - Then:
      echo frozen > /sys/block/mdX/md/sync_action
    hangs forever before the fix.

Write-error variant:
  - Use dm-flakey on /dev/sd[b-f].
  - Start the same 4 -> 5 reshape on flakey0..flakey4.
  - Once reshape starts, switch two members, e.g. flakey3 and flakey4,
    to error_writes.
  - Then:
      echo frozen > /sys/block/mdX/md/sync_action
    hangs forever before the fix.

md_do_sync() exits its main loop on MD_RECOVERY_INTR but then blocks
forever at:

  wait_event(mddev->recovery_wait,
		!atomic_read(&mddev->recovery_active));

After the fix recovery_active drains to zero, md_do_sync() prints

    md/raid:md0: Cannot continue operation (2/5 failed).
    md: md0: reshape interrupted.

changes v1 -> v2:
- handle reshape write deadlock while failed devices more than max degraded

Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
 drivers/md/raid5.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 65ae7d8930fc..a320b71d7117 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -3728,10 +3728,82 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
 
 	if (abort)
 		md_sync_error(conf->mddev);
 }
 
+/*
+ * handle_failed_reshape - handl failed stripes when reshape failed and
+ *			   degraded devices >= max_degraded
+ *
+ * handle following kinds of stripe:
+ * 1. cleanup the following kinds of destination stripe:
+ *	- new regions past the old end of the array, zero-filled in place,
+ *	  requires no source data.
+ *		(STRIPE_EXPANDING | STRIPE_EXPAND_READY)
+ *	- prepare source data chunks already done, and writeout failed
+ *		(STRIPE_EXPAND_READY)
+ * 2. dest stripes that need source data (STRIPE_EXPANDING, no STRIPE_HANDLE)
+ *   - these kind of stripes sit idle in the stripe cache and are never seen
+ *     by handle_stripe(). So clean up indirectly when thier source stripe
+ *     (type 3) is processed.
+ * 3. src stripes (STRIPE_EXPAND_SOURCE)
+ *   - hit handle_stripe() after thier member disks are markded Faulty.
+ *   - clear STRIPE_EXPAND_SOURCE, finds and cleanup all dependent destination
+ *     stripes that were waiting for data.
+ *   - walks the source's data disks, compute the corresponding destination
+ *     sector, looks up the destination stripe, and do cleanup(clear flags,
+ *     dec counters, call md_done_sync())
+ */
+static void handle_failed_reshape(struct r5conf *conf, struct stripe_head *sh,
+				  struct stripe_head_state *s)
+{
+	int i;
+	bool was_expanding = test_and_clear_bit(STRIPE_EXPANDING, &sh->state);
+	bool was_ready = test_and_clear_bit(STRIPE_EXPAND_READY, &sh->state);
+
+	if (was_expanding || was_ready) {
+		atomic_dec(&conf->reshape_stripes);
+		wake_up(&conf->wait_for_reshape);
+		md_done_sync(conf->mddev, RAID5_STRIPE_SECTORS(conf));
+	}
+
+	s->expanded = 0;
+	s->expanding = 0;
+
+	/* 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)) {
+				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);
+		}
+	}
+}
+
 static int want_replace(struct stripe_head *sh, int disk_idx)
 {
 	struct md_rdev *rdev;
 	int rv = 0;
 
@@ -5001,10 +5073,12 @@ static void handle_stripe(struct stripe_head *sh)
 		break_stripe_batch_list(sh, 0);
 		if (s.to_read+s.to_write+s.written)
 			handle_failed_stripe(conf, sh, &s, disks);
 		if (s.syncing + s.replacing)
 			handle_failed_sync(conf, sh, &s);
+		if (s.expanding + s.expanded)
+			handle_failed_reshape(conf, sh, &s);
 	}
 
 	/* Now we check to see if any write operations have recently
 	 * completed
 	 */
-- 
2.54.0

^ permalink raw reply related

* Re: [PATCH v2] md/raid5: fix reshape deadlock while failed devices more than max degraded
From: sashiko-bot @ 2026-06-15 11:49 UTC (permalink / raw)
  To: Chen Cheng; +Cc: yukuai, linux-raid
In-Reply-To: <20260615113450.2088877-1-chencheng@fnnas.com>

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?

> +				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);
> +		}
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615113450.2088877-1-chencheng@fnnas.com?part=1

^ permalink raw reply

* Re: [PATCH] md/md-llbitmap: grow the page cache in place for reshape
From: yu kuai @ 2026-06-15 16:19 UTC (permalink / raw)
  To: Su Yue, Yu Kuai
  Cc: Song Liu, Li Nan, Xiao Ni, linux-raid, linux-kernel, yukuai
In-Reply-To: <8q8gawaz.fsf@damenly.org>

Hi,

在 2026/6/15 19:16, Su Yue 写道:
> On Fri 05 Jun 2026 at 17:15, Yu Kuai <yukuai@kernel.org> wrote:
>
>> From: Yu Kuai <yukuai@fygo.io>
>>
>> Use the page-control helpers to grow llbitmap's cached pages in place
>> for resize and later reshape preparation, instead of rebuilding the
>> whole cache.
>>
>> Signed-off-by: Yu Kuai <yukuai@fygo.io>
>> ---
>>  drivers/md/md-llbitmap.c | 139  +++++++++++++++++++++++++++++++++++----
>>  1 file changed, 127 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
>> index 2f2896fe4d6f..91d3dec43d48 100644
>> --- a/drivers/md/md-llbitmap.c
>> +++ b/drivers/md/md-llbitmap.c
>> @@ -414,10 +414,23 @@ static char 
>> state_machine[BitStateCount][BitmapActionCount] = {
>>          [BitmapActionClearUnwritten]    = BitUnwritten,
>>      },
>>  };
>>
>>  static void __llbitmap_flush(struct mddev *mddev);
>> +static void llbitmap_flush(struct mddev *mddev);
>> +static void llbitmap_update_sb(void *data);
>> +
>> +static void llbitmap_resize_chunks(struct mddev *mddev, sector_t 
>> blocks,
>> +                   unsigned long *chunksize,
>> +                   unsigned long *chunks)
>>
> NIT: I would like call it llbitmap_calculate_chunks.

Agreed this is better, will change it.

>
>> +{
>> +    *chunks = DIV_ROUND_UP_SECTOR_T(blocks, *chunksize);
>> +    while (*chunks > mddev->bitmap_info.space << SECTOR_SHIFT) {
>> +        *chunksize = *chunksize << 1;
>> +        *chunks = DIV_ROUND_UP_SECTOR_T(blocks, *chunksize);
>> +    }
>> +}
>>
>>  static enum llbitmap_state llbitmap_read(struct llbitmap  *llbitmap, 
>> loff_t pos)
>>  {
>>      unsigned int idx;
>>      unsigned int offset;
>> @@ -653,10 +666,52 @@ static unsigned int 
>> llbitmap_reserved_pages(struct llbitmap *llbitmap)
>>  {
>>      return DIV_ROUND_UP(llbitmap->mddev->bitmap_info.space << 
>>  SECTOR_SHIFT,
>>                  PAGE_SIZE);
>>  }
>>
>> +static int llbitmap_expand_pages(struct llbitmap *llbitmap,
>> +                 unsigned long chunks)
>> +{
>> +    struct llbitmap_page_ctl **pctl;
>> +    unsigned int old_nr_pages = llbitmap->nr_pages;
>> +    unsigned int nr_pages = llbitmap_used_pages(llbitmap, chunks);
>> +    int i;
>> +    int ret;
>> +
>> +    if (nr_pages <= old_nr_pages)
>> +        return 0;
>> +
>> +    pctl = kcalloc(nr_pages, sizeof(*pctl), GFP_KERNEL);
>> +    if (!pctl)
>> +        return -ENOMEM;
>> +
>> +    if (llbitmap->pctl)
>> +        memcpy(pctl, llbitmap->pctl,
>> +               array_size(old_nr_pages, sizeof(*pctl)));
>> +
>> +    for (i = old_nr_pages; i < nr_pages; i++) {
>> +        pctl[i] = llbitmap_alloc_page_ctl(llbitmap, i);
>> +        if (IS_ERR(pctl[i]))
>> +            goto err_alloc_ptr;
>> +    }
>> +
>> +    kfree(llbitmap->pctl);
>> +    llbitmap->pctl = pctl;
>> +    llbitmap->nr_pages = nr_pages;
>> +    return 0;
>> +
>> +err_alloc_ptr:
>> +    ret = PTR_ERR(pctl[i]);
>> +    for (i--; i >= (int)old_nr_pages; i--) {
>>
> Confused about why not just declare i as an unsigned int?
>
Yes, this looks better as well.

>
> -- 
> Su
>> +        __free_page(pctl[i]->page);
>> +        percpu_ref_exit(&pctl[i]->active);
>> +        kfree(pctl[i]);
>> +    }
>> +    kfree(pctl);
>> +    return ret;
>> +}
>> +
>>  static int llbitmap_alloc_pages(struct llbitmap *llbitmap)
>>  {
>>      unsigned int used_pages = llbitmap_used_pages(llbitmap, 
>>  llbitmap->chunks);
>>      unsigned int nr_pages = max(used_pages, 
>>  llbitmap_reserved_pages(llbitmap));
>>      int i;
>> @@ -728,10 +783,38 @@ static bool llbitmap_zero_all_disks(struct 
>> llbitmap *llbitmap)
>>      }
>>
>>      return true;
>>  }
>>
>> +static void llbitmap_mark_range(struct llbitmap *llbitmap,
>> +                unsigned long start,
>> +                unsigned long end,
>> +                enum llbitmap_state state)
>> +{
>> +    while (start <= end) {
>> +        llbitmap_write(llbitmap, state, start);
>> +        start++;
>> +    }
>> +}
>> +
>> +static int llbitmap_prepare_resize(struct llbitmap *llbitmap,
>> +                   unsigned long old_chunks,
>> +                   unsigned long new_chunks,
>> +                   unsigned long cache_chunks)
>> +{
>> +    int ret;
>> +
>> +    llbitmap_flush(llbitmap->mddev);
>> +    ret = llbitmap_expand_pages(llbitmap, cache_chunks);
>> +    if (ret)
>> +        return ret;
>> +    if (new_chunks > old_chunks)
>> +        llbitmap_mark_range(llbitmap, old_chunks, new_chunks - 1,
>> +                    BitUnwritten);
>> +    return 0;
>> +}
>> +
>>  static void llbitmap_init_state(struct llbitmap *llbitmap)
>>  {
>>      struct mddev *mddev = llbitmap->mddev;
>>      enum llbitmap_state state = BitUnwritten;
>>      unsigned long i;
>> @@ -1024,14 +1107,14 @@ static int llbitmap_read_sb(struct llbitmap 
>> *llbitmap)
>>          pr_err("md/llbitmap: %s: chunksize not a power of 2",
>>                 mdname(mddev));
>>          goto out_put_page;
>>      }
>>
>> -    if (chunksize < DIV_ROUND_UP_SECTOR_T(mddev->resync_max_sectors,
>> +    if (chunksize < DIV_ROUND_UP_SECTOR_T(sync_size,
>>                            mddev->bitmap_info.space <<  SECTOR_SHIFT)) {
>>          pr_err("md/llbitmap: %s: chunksize too small %lu < %llu / 
>>  %lu",
>> -               mdname(mddev), chunksize, mddev->resync_max_sectors,
>> +               mdname(mddev), chunksize, sync_size,
>>                 mddev->bitmap_info.space);
>>          goto out_put_page;
>>      }
>>
>>      daemon_sleep = le32_to_cpu(sb->daemon_sleep);
>> @@ -1169,28 +1252,60 @@ static int llbitmap_create(struct mddev *mddev)
>>  }
>>
>>  static int llbitmap_resize(struct mddev *mddev, sector_t  blocks, 
>> int chunksize)
>>  {
>>      struct llbitmap *llbitmap = mddev->bitmap;
>> +    sector_t old_blocks = llbitmap->sync_size;
>> +    unsigned long old_chunks = llbitmap->chunks;
>>      unsigned long chunks;
>> +    unsigned long cache_chunks;
>> +    int ret = 0;
>> +    unsigned long bitmap_chunksize;
>> +    bool reshape;
>>
>>      if (chunksize == 0)
>>          chunksize = llbitmap->chunksize;
>>
>> -    /* If there is enough space, leave the chunksize unchanged. */
>> -    chunks = DIV_ROUND_UP_SECTOR_T(blocks, chunksize);
>> -    while (chunks > mddev->bitmap_info.space << SECTOR_SHIFT) {
>> -        chunksize = chunksize << 1;
>> -        chunks = DIV_ROUND_UP_SECTOR_T(blocks, chunksize);
>> -    }
>> +    bitmap_chunksize = chunksize;
>> +    llbitmap_resize_chunks(mddev, blocks, &bitmap_chunksize, &chunks);
>>
>> -    llbitmap->chunkshift = ffz(~chunksize);
>> -    llbitmap->chunksize = chunksize;
>> -    llbitmap->chunks = chunks;
>> -    llbitmap->sync_size = blocks;
>> +    reshape = mddev->delta_disks || mddev->new_level != mddev->level ||
>> +        mddev->new_layout != mddev->layout ||
>> +        mddev->new_chunk_sectors != mddev->chunk_sectors;
>> +    if (!reshape && bitmap_chunksize != llbitmap->chunksize)
>> +        return -EOPNOTSUPP;
>> +    if (blocks == old_blocks && chunks == llbitmap->chunks)
>> +        return 0;
>> +
>> +    mutex_lock(&mddev->bitmap_info.mutex);
>>
>> +    cache_chunks = reshape ? max(old_chunks, chunks) : chunks;
>> +    ret = llbitmap_prepare_resize(llbitmap, old_chunks, chunks, 
>> cache_chunks);
>> +    if (ret)
>> +        goto out;
>> +
>> +    if (reshape) {
>> +        llbitmap->reshape_sync_size = blocks;
>> +        llbitmap->reshape_chunksize = bitmap_chunksize;
>> +        llbitmap->reshape_chunks = chunks;
>> +        llbitmap->chunks = max(old_chunks, chunks);
>> +    } else {
>> +        if (blocks < old_blocks && chunks < old_chunks)
>> +            llbitmap_mark_range(llbitmap, chunks, old_chunks - 1,
>> +                        BitUnwritten);
>> +        mddev->bitmap_info.chunksize = bitmap_chunksize;
>> +        llbitmap->chunks = chunks;
>> +        llbitmap->sync_size = blocks;
>> +        llbitmap_update_sb(llbitmap);
>> +    }
>> +    __llbitmap_flush(mddev);
>> +    mutex_unlock(&mddev->bitmap_info.mutex);
>>      return 0;
>> +
>> +out:
>> +    mutex_unlock(&mddev->bitmap_info.mutex);
>> +    return ret;
>>  }
>>
>>  static int llbitmap_load(struct mddev *mddev)
>>  {
>>      enum llbitmap_action action = BitmapActionReload;

-- 
Thanks,
Kuai

^ permalink raw reply

* Re: [PATCH v2] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Eric Biggers @ 2026-06-15 18:44 UTC (permalink / raw)
  To: David Laight
  Cc: Andrew Morton, linux-kernel, Christoph Hellwig, linux-crypto, x86,
	linux-raid
In-Reply-To: <20260614111628.00af46b9@pumpkin>

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:
> 
> > 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)
> 
> 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.

> 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.

> Since I suggested it :-)
> 
> Reviewed-By: David Laight <david.laight.linux@gmail.com>
> 
> 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.

So I just ended up moving the loop to the asm, which reliably gives us
the code we want.

> That does have the side effect of making p0 be %1 which doesn't improve
> readability. Either used named parameters or possibly just change p0 to p1 (etc)
> so they match.
> 
> 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.

> 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.

> Using the same loop for the avx-256 and sse (and even smaller) functions could
> well generate code that runs 'pretty much as fast as possible' on older cpu.
> Intel cpu (going back to Sandy bridge) are likely to execute the loop in the
> same number of clocks - but clearly copying half or a quarter of the data.
> But I've no experience of zen1.
> 
> Might be worth doing for avx-256, does any care about anything older :-)

Yes, the existing AVX code is probably excessively unrolled.  It
generates almost 4 KiB of code.

- Eric

^ permalink raw reply

* [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Eric Biggers @ 2026-06-15 19:03 UTC (permalink / raw)
  To: Andrew Morton, linux-kernel
  Cc: Christoph Hellwig, linux-crypto, x86, Eric Biggers, David Laight,
	linux-raid

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%

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.

Reviewed-by: David Laight <david.laight.linux@gmail.com>
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---

Changed in v3:
    - Renamed p0-p4 to p1-p5 to match the inline asm indices
    - Swapped the base and index registers to the logical order
    - Added David's Reviewed-by

Changed in v2:
    - Fixed build on UML
    - Reworked the implementation

 lib/raid/xor/Makefile         |   2 +-
 lib/raid/xor/x86/xor-avx512.c | 121 ++++++++++++++++++++++++++++++++++
 lib/raid/xor/x86/xor_arch.h   |  26 ++++----
 3 files changed, 137 insertions(+), 12 deletions(-)
 create mode 100644 lib/raid/xor/x86/xor-avx512.c

diff --git a/lib/raid/xor/Makefile b/lib/raid/xor/Makefile
index 4d633dfd5b90..4af945861a51 100644
--- a/lib/raid/xor/Makefile
+++ b/lib/raid/xor/Makefile
@@ -26,11 +26,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_arm/xor-neon.o		+= $(CC_FLAGS_FPU)
 CFLAGS_REMOVE_arm/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 99fe85a213c6..b5d49376fc97 100644
--- a/lib/raid/xor/x86/xor_arch.h
+++ b/lib/raid/xor/x86/xor_arch.h
@@ -4,26 +4,30 @@
 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) &&
-	    boot_cpu_has(X86_FEATURE_OSXSAVE)) {
+	if (IS_ENABLED(CONFIG_X86_64) && boot_cpu_has(X86_FEATURE_AVX512F) &&
+	    boot_cpu_has(X86_FEATURE_OSXSAVE) &&
+	    !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) &&
+		   boot_cpu_has(X86_FEATURE_OSXSAVE)) {
+		/* 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);

base-commit: 8cd9520d35a6c38db6567e97dd93b1f11f185dc6
-- 
2.54.0


^ permalink raw reply related

* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: sashiko-bot @ 2026-06-15 19:25 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-raid, yukuai
In-Reply-To: <20260615190338.26581-1-ebiggers@kernel.org>

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The AVX-512 XOR implementation is forced based solely on `boot_cpu_has(X86_FEATURE_AVX512F)` without checking if the OS actually manages ZMM registers via `cpu_has_xfeatures()`. This introduces an Undefined Opcode (#UD) kernel panic vulnerability on systems where AVX-512 is supported by the CPU but restricted in XCR0.
--

commit b63aaca8e8b386ef1820d1caac3bce2cb328e8c3
Author: Eric Biggers <ebiggers@kernel.org>

lib/raid/xor: x86: Add AVX-512 optimized xor_gen()

This commit adds an AVX-512 implementation of xor_gen() using 512-bit ZMM
registers and the vpternlogq instruction. It enables this implementation on
x86_64 CPUs that support AVX512F but do not prefer YMM registers, avoiding
known downclocking issues on older architectures. The implementation
explicitly omits the cpu_has_xfeatures() check.

> 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.

[Severity: High]
Does omitting the cpu_has_xfeatures() check introduce a potential Invalid
Opcode exception in virtualized environments?

If a hypervisor advertises AVX512F in CPUID but omits the AVX-512 FPU
extended state from XSTATE, boot_cpu_has(X86_FEATURE_AVX512F) will evaluate
to true, but the OS will not enable AVX-512 in XCR0. Executing 512-bit
EVEX instructions without XCR0[7:5] enabled causes an Invalid Opcode
exception and kernel panic.

Would it be safer to preserve this check, perhaps providing a stub or
using ifdef for User-Mode Linux compatibility instead?

> diff --git a/lib/raid/xor/x86/xor_arch.h b/lib/raid/xor/x86/xor_arch.h
> index 99fe85a213c66..b5d49376fc974 100644
> --- a/lib/raid/xor/x86/xor_arch.h
> +++ b/lib/raid/xor/x86/xor_arch.h
[ ... ]
> @@ -6,22 +6,26 @@ 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;
>  
>  static __always_inline void __init arch_xor_init(void)
>  {
> -	if (boot_cpu_has(X86_FEATURE_AVX) &&
> -	    boot_cpu_has(X86_FEATURE_OSXSAVE)) {
> +	if (IS_ENABLED(CONFIG_X86_64) && boot_cpu_has(X86_FEATURE_AVX512F) &&
> +	    boot_cpu_has(X86_FEATURE_OSXSAVE) &&
> +	    !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);

[Severity: High]
Can this force the AVX-512 implementation on systems that do not actually
support ZMM registers in the OS?

During boot, fpu__init_system_xstate() limits fpu_kernel_cfg.max_features
based on the XSTATE leaf, but does not clear X86_FEATURE_AVX512F from
boot_cpu_data.x86_capability. As a result, this check passes and forces
xor_block_avx512, which then attempts to execute AVX-512 instructions on
an unsupported FPU context.

Is it necessary to include cpu_has_xfeatures(XFEATURE_MASK_AVX512, NULL)
here to verify the FPU context supports these instructions before forcing
the block?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615190338.26581-1-ebiggers@kernel.org?part=1

^ 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

* 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 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 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: 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 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 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: 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

* [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] 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

* 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

* [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

* [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

* 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

* 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: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 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

* [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


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox