The kthread worker API has been improved in 4.9-rc1. The 2nd
patch uses the new functions and simplifies the kthread worker
creation and destroying.
I have found a possible race when working on the API conversion.
A proposed fix is in the 1st patch.
Both changes are compile tested only. I did not find an easy way
how to test them at runtime.
Petr Mladek (2):
IB/rdmavt: Avoid queuing work into a destroyed cq kthread worker
IB/rdmavt: Handle the kthread worker using the new API
drivers/infiniband/sw/rdmavt/cq.c | 64 +++++++++++++++++----------------------
1 file changed, 27 insertions(+), 37 deletions(-)
--
1.8.5.6
Use the new API to create and destroy the cq kthread worker.
The API hides some implementation details.
In particular, kthread_create_worker() allocates and initializes
struct kthread_worker. It runs the kthread the right way and stores
task_struct into the worker structure. In addition, the *on_cpu()
variant binds the kthread to the given cpu and the related memory
node.
kthread_destroy_worker() flushes all pending works, stops
the kthread and frees the structure.
This patch does not change the existing behavior. Note that we must
use the on_cpu() variant because the function starts the kthread
and it must bind it to the right CPU before waking. The numa node
is associated for given CPU as well.
Signed-off-by: Petr Mladek <[email protected]>
---
drivers/infiniband/sw/rdmavt/cq.c | 34 +++++++++++-----------------------
1 file changed, 11 insertions(+), 23 deletions(-)
diff --git a/drivers/infiniband/sw/rdmavt/cq.c b/drivers/infiniband/sw/rdmavt/cq.c
index 223ec4589fc7..4d0b6992e847 100644
--- a/drivers/infiniband/sw/rdmavt/cq.c
+++ b/drivers/infiniband/sw/rdmavt/cq.c
@@ -503,33 +503,23 @@ int rvt_poll_cq(struct ib_cq *ibcq, int num_entries, struct ib_wc *entry)
*/
int rvt_driver_cq_init(struct rvt_dev_info *rdi)
{
- int ret = 0;
int cpu;
- struct task_struct *task;
+ struct kthread_worker *worker;
if (rdi->worker)
return 0;
+
spin_lock_init(&rdi->n_cqs_lock);
- rdi->worker = kzalloc(sizeof(*rdi->worker), GFP_KERNEL);
- if (!rdi->worker)
- return -ENOMEM;
- kthread_init_worker(rdi->worker);
- task = kthread_create_on_node(
- kthread_worker_fn,
- rdi->worker,
- rdi->dparms.node,
- "%s", rdi->dparms.cq_name);
- if (IS_ERR(task)) {
- kfree(rdi->worker);
- rdi->worker = NULL;
- return PTR_ERR(task);
- }
- set_user_nice(task, MIN_NICE);
cpu = cpumask_first(cpumask_of_node(rdi->dparms.node));
- kthread_bind(task, cpu);
- wake_up_process(task);
- return ret;
+ worker = kthread_create_worker_on_cpu(cpu, 0,
+ "%s", rdi->dparms.cq_name);
+ if (IS_ERR(worker))
+ return PTR_ERR(worker);
+
+ set_user_nice(worker->task, MIN_NICE);
+ rdi->worker = worker;
+ return 0;
}
/**
@@ -549,7 +539,5 @@ void rvt_cq_exit(struct rvt_dev_info *rdi)
rdi->worker = NULL;
spin_unlock_irq(&rdi->n_cqs_lock);
- kthread_flush_worker(worker);
- kthread_stop(worker->task);
- kfree(worker);
+ kthread_destroy_worker(worker);
}
--
1.8.5.6
The memory barrier is not enough to protect queuing works into
a destroyed cq kthread. Just imagine the following situation:
CPU1 CPU2
rvt_cq_enter()
worker = cq->rdi->worker;
rvt_cq_exit()
rdi->worker = NULL;
smp_wmb();
kthread_flush_worker(worker);
kthread_stop(worker->task);
kfree(worker);
// nothing queued yet =>
// nothing flushed and
// happily stopped and freed
if (likely(worker)) {
// true => read before CPU2 acted
cq->notify = RVT_CQ_NONE;
cq->triggered++;
kthread_queue_work(worker, &cq->comptask);
BANG: worker has been flushed/stopped/freed in the meantime.
This patch solves this by protecting the critical sections by
rdi->n_cqs_lock. It seems that this lock is not much contended
and looks reasonable for this purpose.
One catch is that rvt_cq_enter() might be called from IRQ context.
Therefore we must always take the lock with IRQs disabled to avoid
a possible deadlock.
Signed-off-by: Petr Mladek <[email protected]>
---
drivers/infiniband/sw/rdmavt/cq.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/drivers/infiniband/sw/rdmavt/cq.c b/drivers/infiniband/sw/rdmavt/cq.c
index 6d9904a4a0ab..223ec4589fc7 100644
--- a/drivers/infiniband/sw/rdmavt/cq.c
+++ b/drivers/infiniband/sw/rdmavt/cq.c
@@ -119,18 +119,17 @@ void rvt_cq_enter(struct rvt_cq *cq, struct ib_wc *entry, bool solicited)
if (cq->notify == IB_CQ_NEXT_COMP ||
(cq->notify == IB_CQ_SOLICITED &&
(solicited || entry->status != IB_WC_SUCCESS))) {
- struct kthread_worker *worker;
/*
* This will cause send_complete() to be called in
* another thread.
*/
- smp_read_barrier_depends(); /* see rvt_cq_exit */
- worker = cq->rdi->worker;
- if (likely(worker)) {
+ spin_lock(&cq->rdi->n_cqs_lock);
+ if (likely(cq->rdi->worker)) {
cq->notify = RVT_CQ_NONE;
cq->triggered++;
- kthread_queue_work(worker, &cq->comptask);
+ kthread_queue_work(cq->rdi->worker, &cq->comptask);
}
+ spin_unlock(&cq->rdi->n_cqs_lock);
}
spin_unlock_irqrestore(&cq->lock, flags);
@@ -240,15 +239,15 @@ struct ib_cq *rvt_create_cq(struct ib_device *ibdev,
}
}
- spin_lock(&rdi->n_cqs_lock);
+ spin_lock_irq(&rdi->n_cqs_lock);
if (rdi->n_cqs_allocated == rdi->dparms.props.max_cq) {
- spin_unlock(&rdi->n_cqs_lock);
+ spin_unlock_irq(&rdi->n_cqs_lock);
ret = ERR_PTR(-ENOMEM);
goto bail_ip;
}
rdi->n_cqs_allocated++;
- spin_unlock(&rdi->n_cqs_lock);
+ spin_unlock_irq(&rdi->n_cqs_lock);
if (cq->ip) {
spin_lock_irq(&rdi->pending_lock);
@@ -296,9 +295,9 @@ int rvt_destroy_cq(struct ib_cq *ibcq)
struct rvt_dev_info *rdi = cq->rdi;
kthread_flush_work(&cq->comptask);
- spin_lock(&rdi->n_cqs_lock);
+ spin_lock_irq(&rdi->n_cqs_lock);
rdi->n_cqs_allocated--;
- spin_unlock(&rdi->n_cqs_lock);
+ spin_unlock_irq(&rdi->n_cqs_lock);
if (cq->ip)
kref_put(&cq->ip->ref, rvt_release_mmap_info);
else
@@ -541,12 +540,15 @@ void rvt_cq_exit(struct rvt_dev_info *rdi)
{
struct kthread_worker *worker;
- worker = rdi->worker;
- if (!worker)
+ /* block future queuing from send_complete() */
+ spin_lock_irq(&rdi->n_cqs_lock);
+ if (!rdi->worker) {
+ spin_unlock_irq(&rdi->n_cqs_lock);
return;
- /* blocks future queuing from send_complete() */
+ }
rdi->worker = NULL;
- smp_wmb(); /* See rdi_cq_enter */
+ spin_unlock_irq(&rdi->n_cqs_lock);
+
kthread_flush_worker(worker);
kthread_stop(worker->task);
kfree(worker);
--
1.8.5.6
On Wed, 2016-10-19 at 14:07 +0200, Petr Mladek wrote:
> The kthread worker API has been improved in 4.9-rc1. The 2nd
> patch uses the new functions and simplifies the kthread worker
> creation and destroying.
>
> I have found a possible race when working on the API conversion.
> A proposed fix is in the 1st patch.
>
> Both changes are compile tested only. I did not find an easy way
> how to test them at runtime.
>
> Petr Mladek (2):
> IB/rdmavt: Avoid queuing work into a destroyed cq kthread worker
> IB/rdmavt: Handle the kthread worker using the new API
>
> drivers/infiniband/sw/rdmavt/cq.c | 64 +++++++++++++++++----------
> ------------
> 1 file changed, 27 insertions(+), 37 deletions(-)
Thanks for the patches. I'm going to take a closer look, I just now
seen these.
-Denny
On 10/20/2016 8:56 AM, Dalessandro, Dennis wrote:
> On Wed, 2016-10-19 at 14:07 +0200, Petr Mladek wrote:
>> The kthread worker API has been improved in 4.9-rc1. The 2nd
>> patch uses the new functions and simplifies the kthread worker
>> creation and destroying.
>>
>> I have found a possible race when working on the API conversion.
>> A proposed fix is in the 1st patch.
>>
>> Both changes are compile tested only. I did not find an easy way
>> how to test them at runtime.
>>
>> Petr Mladek (2):
>> IB/rdmavt: Avoid queuing work into a destroyed cq kthread worker
>> IB/rdmavt: Handle the kthread worker using the new API
>>
>> drivers/infiniband/sw/rdmavt/cq.c | 64 +++++++++++++++++----------
>> ------------
>> 1 file changed, 27 insertions(+), 37 deletions(-)
>
> Thanks for the patches. I'm going to take a closer look, I just now
> seen these.
>
> -Denny
>
These looked good to me, and you haven't objected, so I'm taking this
series. Thanks.
--
Doug Ledford <[email protected]>
GPG Key ID: 0E572FDD