[PATCHv2 1/1] RDMA/rxe: Fix qp error handler

[PATCHv2 1/1] RDMA/rxe: Fix qp error handler

[yanjun.zhu]

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

Re: [PATCHv2 1/1] RDMA/rxe: Fix qp error handler

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

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 

Re: [PATCHv2 1/1] RDMA/rxe: Fix qp error handler

[Yanjun Zhu]

在 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