* [PATCHv2 1/1] RDMA/rxe: Fix qp error handler
@ 2022-07-10 4:37 yanjun.zhu
2022-07-14 16:54 ` Bob Pearson
0 siblings, 1 reply; 3+ messages in thread
From: yanjun.zhu @ 2022-07-10 4:37 UTC (permalink / raw)
To: jgg, leon, linux-rdma, yanjun.zhu; +Cc: syzbot+833061116fa28df97f3b
From: Zhu Yanjun <yanjun.zhu@linux.dev>
About 7 spin locks in qp creation needs to be initialized. Now these
spin locks are initialized in the function rxe_qp_init_misc. This
will avoid the error "initialize spin locks before use".
Reported-by: syzbot+833061116fa28df97f3b@syzkaller.appspotmail.com
Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
---
drivers/infiniband/sw/rxe/rxe_qp.c | 12 ++++++++----
drivers/infiniband/sw/rxe/rxe_task.c | 1 -
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
index 8355a5b1cb60..259d8bb15116 100644
--- a/drivers/infiniband/sw/rxe/rxe_qp.c
+++ b/drivers/infiniband/sw/rxe/rxe_qp.c
@@ -172,6 +172,14 @@ static void rxe_qp_init_misc(struct rxe_dev *rxe, struct rxe_qp *qp,
spin_lock_init(&qp->state_lock);
+ spin_lock_init(&qp->req.task.state_lock);
+ spin_lock_init(&qp->resp.task.state_lock);
+ spin_lock_init(&qp->comp.task.state_lock);
+
+ spin_lock_init(&qp->sq.sq_lock);
+ spin_lock_init(&qp->rq.producer_lock);
+ spin_lock_init(&qp->rq.consumer_lock);
+
atomic_set(&qp->ssn, 0);
atomic_set(&qp->skb_out, 0);
}
@@ -231,7 +239,6 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
qp->req.opcode = -1;
qp->comp.opcode = -1;
- spin_lock_init(&qp->sq.sq_lock);
skb_queue_head_init(&qp->req_pkts);
rxe_init_task(rxe, &qp->req.task, qp,
@@ -282,9 +289,6 @@ static int rxe_qp_init_resp(struct rxe_dev *rxe, struct rxe_qp *qp,
}
}
- spin_lock_init(&qp->rq.producer_lock);
- spin_lock_init(&qp->rq.consumer_lock);
-
skb_queue_head_init(&qp->resp_pkts);
rxe_init_task(rxe, &qp->resp.task, qp,
diff --git a/drivers/infiniband/sw/rxe/rxe_task.c b/drivers/infiniband/sw/rxe/rxe_task.c
index 0c4db5bb17d7..77c691570673 100644
--- a/drivers/infiniband/sw/rxe/rxe_task.c
+++ b/drivers/infiniband/sw/rxe/rxe_task.c
@@ -98,7 +98,6 @@ int rxe_init_task(void *obj, struct rxe_task *task,
tasklet_setup(&task->tasklet, rxe_do_task);
task->state = TASK_STATE_START;
- spin_lock_init(&task->state_lock);
return 0;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCHv2 1/1] RDMA/rxe: Fix qp error handler
2022-07-10 4:37 [PATCHv2 1/1] RDMA/rxe: Fix qp error handler yanjun.zhu
@ 2022-07-14 16:54 ` Bob Pearson
2022-07-16 10:58 ` Yanjun Zhu
0 siblings, 1 reply; 3+ messages in thread
From: Bob Pearson @ 2022-07-14 16:54 UTC (permalink / raw)
To: yanjun.zhu, jgg, leon, linux-rdma; +Cc: syzbot+833061116fa28df97f3b
On 7/9/22 23:37, yanjun.zhu@linux.dev wrote:
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
>
> About 7 spin locks in qp creation needs to be initialized. Now these
> spin locks are initialized in the function rxe_qp_init_misc. This
> will avoid the error "initialize spin locks before use".
>
> Reported-by: syzbot+833061116fa28df97f3b@syzkaller.appspotmail.com
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
> drivers/infiniband/sw/rxe/rxe_qp.c | 12 ++++++++----
> drivers/infiniband/sw/rxe/rxe_task.c | 1 -
> 2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
> index 8355a5b1cb60..259d8bb15116 100644
> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
> @@ -172,6 +172,14 @@ static void rxe_qp_init_misc(struct rxe_dev *rxe, struct rxe_qp *qp,
>
> spin_lock_init(&qp->state_lock);
>
> + spin_lock_init(&qp->req.task.state_lock);
> + spin_lock_init(&qp->resp.task.state_lock);
> + spin_lock_init(&qp->comp.task.state_lock);
> +
> + spin_lock_init(&qp->sq.sq_lock);
> + spin_lock_init(&qp->rq.producer_lock);
> + spin_lock_init(&qp->rq.consumer_lock);
> +
> atomic_set(&qp->ssn, 0);
> atomic_set(&qp->skb_out, 0);
> }
> @@ -231,7 +239,6 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
> qp->req.opcode = -1;
> qp->comp.opcode = -1;
>
> - spin_lock_init(&qp->sq.sq_lock);
> skb_queue_head_init(&qp->req_pkts);
>
> rxe_init_task(rxe, &qp->req.task, qp,
> @@ -282,9 +289,6 @@ static int rxe_qp_init_resp(struct rxe_dev *rxe, struct rxe_qp *qp,
> }
> }
>
> - spin_lock_init(&qp->rq.producer_lock);
> - spin_lock_init(&qp->rq.consumer_lock);
> -
> skb_queue_head_init(&qp->resp_pkts);
>
> rxe_init_task(rxe, &qp->resp.task, qp,
> diff --git a/drivers/infiniband/sw/rxe/rxe_task.c b/drivers/infiniband/sw/rxe/rxe_task.c
> index 0c4db5bb17d7..77c691570673 100644
> --- a/drivers/infiniband/sw/rxe/rxe_task.c
> +++ b/drivers/infiniband/sw/rxe/rxe_task.c
> @@ -98,7 +98,6 @@ int rxe_init_task(void *obj, struct rxe_task *task,
> tasklet_setup(&task->tasklet, rxe_do_task);
>
> task->state = TASK_STATE_START;
> - spin_lock_init(&task->state_lock);
>
> return 0;
> }
Zhu,
The task.state_lock spinlocks are an implementation detail of the tasklet code. Seems strange to
move the spin_lock_init() calls up into the qp code for these. This breaks encapsulation. We (HPE)
have a patch coming that extends the tasklet code to support tasklets and/or work queues which allow
steering the work to specific cpus. This gives a significant performance boost for IO intensive
work flows.
The only other issue with this patch is that for xrc QPs, which we don't support yet, the QPs only
have one side implemented and there won't be a reason to do unneeded work. Not a big issue though.
Bob
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCHv2 1/1] RDMA/rxe: Fix qp error handler
2022-07-14 16:54 ` Bob Pearson
@ 2022-07-16 10:58 ` Yanjun Zhu
0 siblings, 0 replies; 3+ messages in thread
From: Yanjun Zhu @ 2022-07-16 10:58 UTC (permalink / raw)
To: Bob Pearson, jgg, leon, linux-rdma; +Cc: syzbot+833061116fa28df97f3b
在 2022/7/15 0:54, Bob Pearson 写道:
> On 7/9/22 23:37, yanjun.zhu@linux.dev wrote:
>> From: Zhu Yanjun <yanjun.zhu@linux.dev>
>>
>> About 7 spin locks in qp creation needs to be initialized. Now these
>> spin locks are initialized in the function rxe_qp_init_misc. This
>> will avoid the error "initialize spin locks before use".
>>
>> Reported-by: syzbot+833061116fa28df97f3b@syzkaller.appspotmail.com
>> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>> ---
>> drivers/infiniband/sw/rxe/rxe_qp.c | 12 ++++++++----
>> drivers/infiniband/sw/rxe/rxe_task.c | 1 -
>> 2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
>> index 8355a5b1cb60..259d8bb15116 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
>> @@ -172,6 +172,14 @@ static void rxe_qp_init_misc(struct rxe_dev *rxe, struct rxe_qp *qp,
>>
>> spin_lock_init(&qp->state_lock);
>>
>> + spin_lock_init(&qp->req.task.state_lock);
>> + spin_lock_init(&qp->resp.task.state_lock);
>> + spin_lock_init(&qp->comp.task.state_lock);
>> +
>> + spin_lock_init(&qp->sq.sq_lock);
>> + spin_lock_init(&qp->rq.producer_lock);
>> + spin_lock_init(&qp->rq.consumer_lock);
>> +
>> atomic_set(&qp->ssn, 0);
>> atomic_set(&qp->skb_out, 0);
>> }
>> @@ -231,7 +239,6 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
>> qp->req.opcode = -1;
>> qp->comp.opcode = -1;
>>
>> - spin_lock_init(&qp->sq.sq_lock);
>> skb_queue_head_init(&qp->req_pkts);
>>
>> rxe_init_task(rxe, &qp->req.task, qp,
>> @@ -282,9 +289,6 @@ static int rxe_qp_init_resp(struct rxe_dev *rxe, struct rxe_qp *qp,
>> }
>> }
>>
>> - spin_lock_init(&qp->rq.producer_lock);
>> - spin_lock_init(&qp->rq.consumer_lock);
>> -
>> skb_queue_head_init(&qp->resp_pkts);
>>
>> rxe_init_task(rxe, &qp->resp.task, qp,
>> diff --git a/drivers/infiniband/sw/rxe/rxe_task.c b/drivers/infiniband/sw/rxe/rxe_task.c
>> index 0c4db5bb17d7..77c691570673 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_task.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_task.c
>> @@ -98,7 +98,6 @@ int rxe_init_task(void *obj, struct rxe_task *task,
>> tasklet_setup(&task->tasklet, rxe_do_task);
>>
>> task->state = TASK_STATE_START;
>> - spin_lock_init(&task->state_lock);
>>
>> return 0;
>> }
> Zhu,
>
> The task.state_lock spinlocks are an implementation detail of the tasklet code. Seems strange to
> move the spin_lock_init() calls up into the qp code for these. This breaks encapsulation. We (HPE)
> have a patch coming that extends the tasklet code to support tasklets and/or work queues which allow
> steering the work to specific cpus. This gives a significant performance boost for IO intensive
> work flows.
OK. The reason that I move spin_lock_init() into rxe_qp_init_misc is to
avoid the error "initialize spin locks before use".
Thanks for sharing your features in HPE. If you want to backport these
new features into linux upstream, I can
keep spin_lock_init in rxe_init_task for future use.
I will send the latest commit very soon.
And look forward to your feature that extends the tasklet code to
support tasklets and/or work queues which allow
steering the work to specific cpus in linux upstream.
I am curious about this feautre. And hope I can see it in linux upstream
very soon ^_^
Zhu Yanjun
>
> The only other issue with this patch is that for xrc QPs, which we don't support yet, the QPs only
> have one side implemented and there won't be a reason to do unneeded work. Not a big issue though.
>
> Bob
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-07-16 10:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-10 4:37 [PATCHv2 1/1] RDMA/rxe: Fix qp error handler yanjun.zhu
2022-07-14 16:54 ` Bob Pearson
2022-07-16 10:58 ` Yanjun Zhu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.