> -----Original Message-----
> From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
> ow...@vger.kernel.org] On Behalf Of Sebastian Andrzej Siewior
> Sent: Saturday, March 16, 2019 00:43
> To: Liu, Yongxin
> Cc: linux-ker...@vger.kernel.org; linux-rt-us...@vger.kernel.org;
> t...@linutronix.de; rost...@goodmis.org; dan.j.willi...@intel.com;
> pagu...@redhat.com; Gortmaker, Paul; linux-nvdimm@lists.01.org
> Subject: Re: [PATCH RT] nvdimm: make lane acquirement RT aware
>
> On 2019-03-11 00:44:58 [+0000], Liu, Yongxin wrote:
> > > but you still have the ndl_lock->lock which protects the resource. So
> in
> > > the unlikely (but possible event) that you switch CPUs after
> obtaining
> > > the CPU number you block on the lock. No harm is done, right?
> >
> > The resource "lane" can be acquired recursively, so "ndl_lock->lock" is
> a conditional lock.
> >
> > ndl_count->count is per CPU.
> > ndl_lock->lock is per lane.
> >
> > Here is an example:
> > Thread A on CPU 5 --> nd_region_acquire_lane --> lane# 5 --> get
> "ndl_lock->lock"
> > --> nd_region_acquire_lane --> lane# 5 --> bypass "ndl_lock->lock" due
> to "ndl_count->count++".
> >
> > Thread B on CPU 5 --> nd_region_acquire_lane --> lane# 5 --> bypass
> "ndl_lock->lock" ("ndl_count->count"
> > was changed by Thread A)
> >
> > If we use raw_smp_processor_id(), no matter which CPU the thread was
> migrated to,
> > if there is another thread running on the old CPU, there will be race
> condition
> > due to per CPU variable "ndl_count->count".
>
> so I've been looking at it again. The recursive locking could have been
> solved better. Like the local_lock() on -RT is doing it.
> Given that you lock with preempt_disable() there should be no in-IRQ
> usage.
> But in the "nd_region->num_lanes >= nr_cpu_ids" case you don't take any
> locks. That would be a problem with raw_smp_processor_id() approach.
>
> So what about the completely untested patch here:
>
> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> index 379bf4305e615..98c2e9df4b2e4 100644
> --- a/drivers/nvdimm/nd.h
> +++ b/drivers/nvdimm/nd.h
> @@ -109,7 +109,8 @@ unsigned sizeof_namespace_label(struct nvdimm_drvdata
> *ndd);
> res; res = next, next = next ? next->sibling : NULL)
>
> struct nd_percpu_lane {
> - int count;
> + struct task_struct *owner;
> + int nestcnt;
> spinlock_t lock;
> };
>
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index e2818f94f2928..8a62f9833513f 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -946,19 +946,17 @@ int nd_blk_region_init(struct nd_region *nd_region)
> */
> unsigned int nd_region_acquire_lane(struct nd_region *nd_region)
> {
> + struct nd_percpu_lane *ndl_lock;
> unsigned int cpu, lane;
>
> - cpu = get_cpu();
> - if (nd_region->num_lanes < nr_cpu_ids) {
> - struct nd_percpu_lane *ndl_lock, *ndl_count;
> -
> - 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
> - lane = cpu;
> + cpu = raw_smp_processor_id();
> + lane = cpu % nd_region->num_lanes;
> + ndl_lock = per_cpu_ptr(nd_region->lane, lane);
> + if (ndl_lock->owner != current) {
> + spin_lock(&ndl_lock->lock);
> + ndl_lock->owner = current;
> + }
> + ndl_lock->nestcnt++;
>
> return lane;
> }
> @@ -966,17 +964,16 @@ 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 = get_cpu();
> - struct nd_percpu_lane *ndl_lock, *ndl_count;
> + struct nd_percpu_lane *ndl_lock;
>
> - 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);
> - put_cpu();
> - }
> - put_cpu();
> + ndl_lock = per_cpu_ptr(nd_region->lane, lane);
> + WARN_ON(ndl_lock->nestcnt == 0);
> + WARN_ON(ndl_lock->owner != current);
> + if (--ndl_lock->nestcnt)
> + return;
> +
> + ndl_lock->owner = NULL;
> + spin_unlock(&ndl_lock->lock);
> }
> EXPORT_SYMBOL(nd_region_release_lane);
>
> @@ -1042,7 +1039,8 @@ static struct nd_region *nd_region_create(struct
> nvdimm_bus *nvdimm_bus,
>
> ndl = per_cpu_ptr(nd_region->lane, i);
> spin_lock_init(&ndl->lock);
> - ndl->count = 0;
> + ndl->owner = NULL;
> + ndl->nestcnt = 0;
> }
>
> for (i = 0; i < ndr_desc->num_mappings; i++) {
>
> > Thanks,
> > Yongxin
>
Consider the recursive call to nd_region_acquire_lane() in the following
situation.
Will there be a dead lock?
Thread A Thread B
| |
| |
CPU 1 CPU 2
| |
| |
get lock for Lane 1 get lock for Lane 2
| |
| |
migrate to CPU 2 migrate to CPU 1
| |
| |
wait lock for Lane 2 wait lock for Lane 1
| |
| |
_____________________________
|
dead lock ?
Thanks,
Yognxin
> Sebastian
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
