NVDIMM Device and Persistent Memory development
 help / color / mirror / Atom feed
From: Aboorva Devarajan <aboorvad@linux.ibm.com>
To: Alison Schofield <alison.schofield@intel.com>
Cc: nvdimm@lists.linux.dev, Dan Williams <djbw@kernel.org>,
	Vishal Verma <vishal.l.verma@intel.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	aboorvad@linux.ibm.com
Subject: Re: [PATCH v2] nvdimm/btt: Handle preemption in BTT lane acquisition
Date: Fri, 01 May 2026 16:27:16 +0530	[thread overview]
Message-ID: <7081312ccb45b067cd414a92c923f69cf1285097.camel@linux.ibm.com> (raw)
In-Reply-To: <20260430024652.3920875-1-alison.schofield@intel.com>

On Wed, 2026-04-29 at 19:46 -0700, Alison Schofield wrote:

> BTT (Block Translation Table) makes persistent memory safe for block
> I/O by guaranteeing atomic sector updates. It uses reserved lanes
> for in-flight BTT operations, which must be used exclusively.
> 
> The btt-check unit test reports data mismatches during BTT I/O due
> to a race in lane acquisition, leading to silent data corruption.
> 
> BTT lane acquisition uses per-CPU recursion tracking with
> migrate_disable(). However, migrate_disable() does not prevent
> preemption, so another task can run on the same CPU and share the
> recursion state. That task can observe a non-zero recursion count,
> bypass locking, and use the same lane at the same time.
> 
> Track lane ownership per task and only allow lockless recursion for
> the owning task. Otherwise, serialize access with the lane spinlock.
> Use spin_(un)lock_bh() so softirq re-entry on the same CPU cannot
> bypass ownership checks or deadlock on the lane lock.
> 
> Found with the NDCTL unit test btt-check.sh
> 
> Fixes: 36c75ce3bd29 ("nd_btt: Make BTT lanes preemptible")
> Assisted-by: Claude Sonnet 4.5
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
> 
> Changes in v2:
> Use spin_(un)lock_bh() (Sashiko AI)
> Update commit log per softirq re-enty and spinlock change
> 
> A new unit test to stress this is under review here:
> https://lore.kernel.org/nvdimm/20260424233633.3762217-1-alison.schofield@intel.com/
> 
> 
>  drivers/nvdimm/nd.h          |  1 +
>  drivers/nvdimm/region_devs.c | 48 +++++++++++++++++++++---------------
>  2 files changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> index b199eea3260e..424c38ca4960 100644
> --- a/drivers/nvdimm/nd.h
> +++ b/drivers/nvdimm/nd.h
> @@ -368,6 +368,7 @@ unsigned sizeof_namespace_label(struct nvdimm_drvdata *ndd);
>  struct nd_percpu_lane {
>  	int count;
>  	spinlock_t lock;
> +	struct task_struct *owner;
>  };
>  
>  enum nd_label_flags {
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index e35c2e18518f..f1c6dcd95b5a 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -905,11 +905,10 @@ void nd_region_advance_seeds(struct nd_region *nd_region, struct device *dev)
>   * @nd_region: region id and number of lanes possible
>   *
>   * A lane correlates to a BLK-data-window and/or a log slot in the BTT.
> - * We optimize for the common case where there are 256 lanes, one
> - * per-cpu.  For larger systems we need to lock to share lanes.  For now
> - * this implementation assumes the cost of maintaining an allocator for
> - * free lanes is on the order of the lock hold time, so it implements a
> - * static lane = cpu % num_lanes mapping.
> + * Lanes are shared across CPUs using a static lane = cpu % num_lanes
> + * mapping, with a per-lane spinlock to serialize access when multiple
> + * tasks share a lane (including when preemption causes multiple tasks
> + * to run on the same CPU).
>   *
>   * In the case of a BTT instance on top of a BLK namespace a lane may be
>   * acquired recursively.  We lock on the first instance.
> @@ -920,35 +919,44 @@ void nd_region_advance_seeds(struct nd_region *nd_region, struct device *dev)
>  unsigned int nd_region_acquire_lane(struct nd_region *nd_region)
>  {
>  	unsigned int cpu, lane;
> +	struct nd_percpu_lane *ndl;
>  
>  	migrate_disable();
>  	cpu = smp_processor_id();
> -	if (nd_region->num_lanes < nr_cpu_ids) {
> -		struct nd_percpu_lane *ndl_lock, *ndl_count;
> -
> +	if (nd_region->num_lanes < nr_cpu_ids)
>  		lane = cpu % nd_region->num_lanes;
> -		ndl_count = per_cpu_ptr(nd_region->lane, cpu);
> -		ndl_lock = per_cpu_ptr(nd_region->lane, lane);
> -		if (ndl_count->count++ == 0)
> -			spin_lock(&ndl_lock->lock);
> -	} else
> +	else
>  		lane = cpu;
>  
> +	/*
> +	 * migrate_disable() keeps the lane stable, but does not prevent
> +	 * preemption. Only the owning task may recurse without taking the
> +	 * lock.
> +	 */
> +	ndl = per_cpu_ptr(nd_region->lane, lane);
> +	if (READ_ONCE(ndl->owner) != current) {
> +		spin_lock_bh(&ndl->lock);
> +		WRITE_ONCE(ndl->owner, current);
> +	}
> +	ndl->count++;
> +
>  	return lane;
>  }
>  EXPORT_SYMBOL(nd_region_acquire_lane);
>  
>  void nd_region_release_lane(struct nd_region *nd_region, unsigned int lane)
>  {
> -	if (nd_region->num_lanes < nr_cpu_ids) {
> -		unsigned int cpu = smp_processor_id();
> -		struct nd_percpu_lane *ndl_lock, *ndl_count;
> +	struct nd_percpu_lane *ndl = per_cpu_ptr(nd_region->lane, lane);
>  
> -		ndl_count = per_cpu_ptr(nd_region->lane, cpu);
> -		ndl_lock = per_cpu_ptr(nd_region->lane, lane);
> -		if (--ndl_count->count == 0)
> -			spin_unlock(&ndl_lock->lock);
> +	if (WARN_ON_ONCE(READ_ONCE(ndl->owner) != current))
> +		goto out;
> +
> +	if (--ndl->count == 0) {
> +		WRITE_ONCE(ndl->owner, NULL);
> +		spin_unlock_bh(&ndl->lock);
>  	}
> +
> +out:
>  	migrate_enable();
>  }
>  EXPORT_SYMBOL(nd_region_release_lane);
> 
> base-commit: 028ef9c96e96197026887c0f092424679298aae8


Hi Alison,

Thanks for the fix.

I noticed a similar race in BTT and wrote a small selftest to
exercise concurrent lane usage [1].

Without your patch on the almost latest upstream kernel v7.0.0, I see silent 
data corruption on this workload, most reliably with PREEMPT_DYNAMIC set to
"lazy" or "full". With your patch applied the test passes across all four preempt modes.

  
  # Without the patch
  echo none      -> pass
  echo voluntary -> pass
  echo lazy      -> "not ok 1 BTT lane contention: 6 process(es) saw corruption"
  echo full      -> "not ok 1 BTT lane contention: 8 process(es) saw corruption"
  

  
  # With the patch
  echo none / voluntary / lazy / full -> all pass

(Full output below)

So,

Tested-by: Aboorva Devarajan <aboorvad@linux.ibm.com>


------------------
Without Patch:
------------------

[nvdimm]# echo none > /sys/kernel/debug/sched/preempt
[nvdimm]# ./run_btt_lane_contention.sh
Creating sector-mode namespace on region1...
Namespace: namespace1.4
Block device: /dev/pmem1.4s
Running: /home/abd/linux/tools/testing/selftests/nvdimm/btt_lane_contention /dev/pmem1.4s 16 100
---
TAP version 13
1..1
# device: /dev/pmem1.4s (1789 MB)
# processes: 16 (2 per CPU across 8 CPUs)
# iterations: 100 per process
# I/O size: 256 KB
# logical block size: 4096 bytes
ok 1 BTT lane contention: all data verified
# Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
Cleaning up namespace namespace1.4...
[nvdimm]# echo voluntary > /sys/kernel/debug/sched/preempt
[nvdimm]# cat /sys/kernel/debug/sched/preempt 
none (voluntary) full lazy 
[nvdimm]# ./run_btt_lane_contention.sh
Creating sector-mode namespace on region1...
Namespace: namespace1.3
Block device: /dev/pmem1.3s
Running: /home/abd/linux/tools/testing/selftests/nvdimm/btt_lane_contention /dev/pmem1.3s 16 100
---
TAP version 13
1..1
# device: /dev/pmem1.3s (1789 MB)
# processes: 16 (2 per CPU across 8 CPUs)
# iterations: 100 per process
# I/O size: 256 KB
# logical block size: 4096 bytes
ok 1 BTT lane contention: all data verified
# Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
Cleaning up namespace namespace1.3...
[nvdimm]# echo lazy > /sys/kernel/debug/sched/preempt
[nvdimm]# cat /sys/kernel/debug/sched/preempt 
none voluntary full (lazy) 
[nvdimm]# ./run_btt_lane_contention.sh
Creating sector-mode namespace on region1...
Namespace: namespace1.4
Block device: /dev/pmem1.4s
Running: /home/abd/linux/tools/testing/selftests/nvdimm/btt_lane_contention /dev/pmem1.4s 16 100
---
TAP version 13
1..1
# device: /dev/pmem1.4s (1789 MB)
# processes: 16 (2 per CPU across 8 CPUs)
# iterations: 100 per process
# I/O size: 256 KB
# logical block size: 4096 bytes
# [proc 10] MISCOMPARE iter=4 block=80 off=0x47180000
# [proc 10]   byte 204800: exp 0x4a got 0x42 (4096/262144 bad)
# [proc 10]   from proc 2 (shared CPU 2)
# [proc 12] MISCOMPARE iter=11 block=279 off=0x582c0000
# [proc 12]   byte 102400: exp 0x4c got 0x44 (4096/262144 bad)
# [proc 12]   from proc 4 (shared CPU 4)
# [proc 13] MISCOMPARE iter=19 block=143 off=0x5d080000
# [proc 14] MISCOMPARE iter=19 block=28 off=0x62380000
# [proc 14]   byte 28672: exp 0x4e got 0x46 (4096/262144 bad)
# [proc 14]   from proc 6 (shared CPU 6)
# [proc 13]   byte 249856: exp 0x4d got 0x45 (4096/262144 bad)
# [proc 13]   from proc 5 (shared CPU 5)
# [proc 1] MISCOMPARE iter=57 block=55 off=0x7d80000
# [proc 1]   byte 135168: exp 0x41 got 0x49 (4096/262144 bad)
# [proc 1]   from proc 9 (shared CPU 1)
# [proc 8] MISCOMPARE iter=71 block=392 off=0x3e000000
# [proc 8]   byte 208896: exp 0x48 got 0x40 (4096/262144 bad)
# [proc 8]   from proc 0 (shared CPU 0)
not ok 1 BTT lane contention: 6 process(es) saw corruption
# Totals: pass:0 fail:1 xfail:0 xpass:0 skip:0 error:0
Cleaning up namespace namespace1.4...
[nvdimm]# echo full > /sys/kernel/debug/sched/preempt
[nvdimm]# cat /sys/kernel/debug/sched/preempt 
none voluntary (full) lazy 
[nvdimm]# ./run_btt_lane_contention.sh
Creating sector-mode namespace on region1...
Namespace: namespace1.3
Block device: /dev/pmem1.3s
Running: /home/abd/linux/tools/testing/selftests/nvdimm/btt_lane_contention /dev/pmem1.3s 16 100
---
TAP version 13
1..1
# device: /dev/pmem1.3s (1789 MB)
# processes: 16 (2 per CPU across 8 CPUs)
# iterations: 100 per process
# I/O size: 256 KB
# logical block size: 4096 bytes
# [proc 1] MISCOMPARE iter=0 block=3 off=0x7080000
# [proc 4] MISCOMPARE iter=0 block=3 off=0x1bfc0000
# [proc 4]   byte 114688: exp 0x44 got 0x4c (4096/262144 bad)
# [proc 7] MISCOMPARE iter=0 block=2 off=0x30ec0000
# [proc 7]   byte 212992: exp 0x47 got 0x4f (4096/262144 bad)
# [proc 1]   byte 159744: exp 0x41 got 0x49 (4096/262144 bad)
# [proc 1]   from proc 9 (shared CPU 1)
# [proc 4]   from proc 12 (shared CPU 4)
# [proc 7]   from proc 15 (shared CPU 7)
# [proc 10] MISCOMPARE iter=0 block=2 off=0x45e00000
# [proc 10]   byte 139264: exp 0x4a got 0x42 (1024/262144 bad)
# [proc 10]   from proc 2 (shared CPU 2)
# [proc 3] MISCOMPARE iter=0 block=9 off=0x15180000
# [proc 3]   byte 217088: exp 0x43 got 0x4b (4096/262144 bad)
# [proc 3]   from proc 11 (shared CPU 3)
# [proc 0] MISCOMPARE iter=0 block=9 off=0x240000
# [proc 0]   byte 159744: exp 0x40 got 0x48 (4096/262144 bad)
# [proc 0]   from proc 8 (shared CPU 0)
# [proc 5] MISCOMPARE iter=0 block=9 off=0x23100000
# [proc 5]   byte 122880: exp 0x45 got 0x4d (4096/262144 bad)
# [proc 5]   from proc 13 (shared CPU 5)
# [proc 6] MISCOMPARE iter=0 block=14 off=0x2a200000
# [proc 6]   byte 57344: exp 0x46 got 0x4e (4096/262144 bad)
# [proc 6]   from proc 14 (shared CPU 6)
not ok 1 BTT lane contention: 8 process(es) saw corruption
# Totals: pass:0 fail:1 xfail:0 xpass:0 skip:0 error:0
Cleaning up namespace namespace1.3...
[nvdimm]# 


------------------
With Patch:
------------------

[nvdimm]# cat /sys/kernel/debug/sched/preempt 
(none) voluntary full lazy 
[nvdimm]# ./run_btt_lane_contention.sh 
Creating sector-mode namespace on region1...
Namespace: namespace1.3
Block device: /dev/pmem1.3s
Running: /home/abd/linux/tools/testing/selftests/nvdimm/btt_lane_contention /dev/pmem1.3s 16 100
---
TAP version 13
1..1
# device: /dev/pmem1.3s (1789 MB)
# processes: 16 (2 per CPU across 8 CPUs)
# iterations: 100 per process
# I/O size: 256 KB
# logical block size: 4096 bytes
ok 1 BTT lane contention: all data verified
# Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
Cleaning up namespace namespace1.3...
[nvdimm]# echo voluntary > /sys/kernel/debug/sched/preempt
[nvdimm]# cat /sys/kernel/debug/sched/preempt 
none (voluntary) full lazy 
[nvdimm]# ./run_btt_lane_contention.sh 
Creating sector-mode namespace on region1...
Namespace: namespace1.4
Block device: /dev/pmem1.4s
Running: /home/abd/linux/tools/testing/selftests/nvdimm/btt_lane_contention /dev/pmem1.4s 16 100
---
TAP version 13
1..1
# device: /dev/pmem1.4s (1789 MB)
# processes: 16 (2 per CPU across 8 CPUs)
# iterations: 100 per process
# I/O size: 256 KB
# logical block size: 4096 bytes
ok 1 BTT lane contention: all data verified
# Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
Cleaning up namespace namespace1.4...
[nvdimm]# echo lazy > /sys/kernel/debug/sched/preempt
[nvdimm]# cat /sys/kernel/debug/sched/preempt 
none voluntary full (lazy) 
[nvdimm]# ./run_btt_lane_contention.sh 
Creating sector-mode namespace on region1...
Namespace: namespace1.3
Block device: /dev/pmem1.3s
Running: /home/abd/linux/tools/testing/selftests/nvdimm/btt_lane_contention /dev/pmem1.3s 16 100
---
TAP version 13
1..1
# device: /dev/pmem1.3s (1789 MB)
# processes: 16 (2 per CPU across 8 CPUs)
# iterations: 100 per process
# I/O size: 256 KB
# logical block size: 4096 bytes
ok 1 BTT lane contention: all data verified
# Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
Cleaning up namespace namespace1.3...
[nvdimm]# echo full > /sys/kernel/debug/sched/preempt
[nvdimm]# cat /sys/kernel/debug/sched/preempt 
none voluntary (full) lazy 
[nvdimm]# ./run_btt_lane_contention.sh 
Creating sector-mode namespace on region1...
Namespace: namespace1.4
Block device: /dev/pmem1.4s
Running: /home/abd/linux/tools/testing/selftests/nvdimm/btt_lane_contention /dev/pmem1.4s 16 100
---
TAP version 13
1..1
# device: /dev/pmem1.4s (1789 MB)
# processes: 16 (2 per CPU across 8 CPUs)
# iterations: 100 per process
# I/O size: 256 KB
# logical block size: 4096 bytes
ok 1 BTT lane contention: all data verified
# Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
Cleaning up namespace namespace1.4...

[1] https://github.com/AboorvaDevarajan/linux/blob/btt_preempt_test/v1/tools/testing/selftests/nvdimm/btt_lane_contention.c


Regards,
Aboorva

  reply	other threads:[~2026-05-01 10:57 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-30  2:46 [PATCH v2] nvdimm/btt: Handle preemption in BTT lane acquisition Alison Schofield
2026-05-01 10:57 ` Aboorva Devarajan [this message]
2026-05-01 11:31 ` Aboorva Devarajan
2026-05-02  5:18   ` Alison Schofield
2026-05-03 16:17     ` Aboorva Devarajan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7081312ccb45b067cd414a92c923f69cf1285097.camel@linux.ibm.com \
    --to=aboorvad@linux.ibm.com \
    --cc=alison.schofield@intel.com \
    --cc=dave.jiang@intel.com \
    --cc=djbw@kernel.org \
    --cc=ira.weiny@intel.com \
    --cc=nvdimm@lists.linux.dev \
    --cc=vishal.l.verma@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox