Le 20/07/2021 à 14:23, Leon Romanovsky a écrit :
On Mon, Jul 19, 2021 at 07:02:15PM +0200, Christophe JAILLET wrote:
A set of IRDMA_CQP_SW_SQSIZE_2048 (i.e. 2048) 'cqp_request' are
pre-allocated and zeroed in 'irdma_create_cqp()' (hw.c). These
structures are managed with the 'cqp->cqp_avail_reqs' list which keeps
track of available entries.
In 'irdma_free_cqp_request()' (utils.c), when an entry is recycled and goes
back to the 'cqp_avail_reqs' list, some fields are reseted.
However, one of these fields, 'compl_info', is initialized within
'irdma_alloc_and_get_cqp_request()'.
Move the corresponding memset to 'irdma_free_cqp_request()' so that the
clean-up is done in only one place. This makes the logic more easy to
understand.
I'm not so sure. The function irdma_alloc_and_get_cqp_request() returns
prepared cqp_request and all users expect that it will returned cleaned
one. The reliance on some other place to clear part of the structure is
prone to errors.
Ok, so maybe, moving:
cqp_request->request_done = false;
cqp_request->callback_fcn = NULL;
cqp_request->waiting = false;
from 'irdma_free_cqp_request()' to 'irdma_alloc_and_get_cqp_request()'
to make explicit what is reseted makes more sense?
From my point of view, it is same same: all (re)initialization are done
at 1 place only.
This would also avoid setting 'waiting' twice (once to false in
'irdma_free_cqp_request()' and one to 'wait'
'irdma_alloc_and_get_cqp_request()')
CJ
Thanks
This also saves this memset in the case that the 'cqp_avail_reqs' list is
empty and a new 'cqp_request' structure must be allocated. This memset is
useless, because the structure is already kzalloc'ed.
Signed-off-by: Christophe JAILLET <christophe.jaillet-39ZsbGIQGT5GWvitb5QawA@xxxxxxxxxxxxxxxx>
---
drivers/infiniband/hw/irdma/utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/infiniband/hw/irdma/utils.c b/drivers/infiniband/hw/irdma/utils.c
index 5bbe44e54f9a..66711024d38b 100644
--- a/drivers/infiniband/hw/irdma/utils.c
+++ b/drivers/infiniband/hw/irdma/utils.c
@@ -445,7 +445,6 @@ struct irdma_cqp_request *irdma_alloc_and_get_cqp_request(struct irdma_cqp *cqp,
cqp_request->waiting = wait;
refcount_set(&cqp_request->refcnt, 1);
- memset(&cqp_request->compl_info, 0, sizeof(cqp_request->compl_info));
return cqp_request;
}
@@ -475,6 +474,7 @@ void irdma_free_cqp_request(struct irdma_cqp *cqp,
cqp_request->request_done = false;
cqp_request->callback_fcn = NULL;
cqp_request->waiting = false;
+ memset(&cqp_request->compl_info, 0, sizeof(cqp_request->compl_info));
spin_lock_irqsave(&cqp->req_lock, flags);
list_add_tail(&cqp_request->list, &cqp->cqp_avail_reqs);
--
2.30.2
