On 5/17/22 10:14, Yanjun Zhu wrote:
>
> 在 2022/5/17 22:51, Bob Pearson 写道:
>> On 5/17/22 14:08, yanjun.zhu@xxxxxxxxx wrote:
>>> From: Zhu Yanjun <yanjun.zhu@xxxxxxxxx>
>>>
>>> Compact the function and move it to the header file.
>> I have two issues with this patch.
>>
>> There is no advantage of having an inline function in a header file
>> that is only called once. The compiler is perfectly capable of (and does)
>> inlining a static function in a .c file if only called once. This just
>> makes the code harder to read.
>
> When this function is put into the header file, this function can be included into the caller function file.
>
> The compiler does not need to call this function in different file. In theory, this can increase
>
> the compile speed.
>
> Why do you insist on putting this function in .c file?
I don't insist. I just don't like them. Jason is correct the subroutine is in rxe_qp.c
and there are two calls in rxe_resp.c and one in rxe_qp.c. Given there are three calls
I would prefer to not do anything. The responder resources code
alloc_rd_atomic_resources()
free_rd_atomic_resources()
free_rd_atomic_resource()
cleanup_rd_atomic_resources()
is mostly called by rxe_qp.c so they are in the right place. The 2-3 instructions
you might save by inlining free_rd_atomic_resource() just aren't worth it in the
tens of thousands of instructions in the response to an rdma_read or atomic request.
To Jason's question. 8a1a0be894da and 290c4a902b79 are already in for-rc. There
isn't any res.read.mr left.
Bob
>
> Zhu Yanjun
>
>>
>> There is a patch in for-rc that gets rid of read.mr in favor of an rkey.
>> This patch is out of date.
>>
>> Bob
>>> Signed-off-by: Zhu Yanjun <yanjun.zhu@xxxxxxxxx>
>>> ---
>>> drivers/infiniband/sw/rxe/rxe_loc.h | 11 ++++++++++-
>>> drivers/infiniband/sw/rxe/rxe_qp.c | 15 ++-------------
>>> drivers/infiniband/sw/rxe/rxe_resp.c | 4 ++--
>>> 3 files changed, 14 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
>>> index 409efeecd581..6517b4f104b1 100644
>>> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
>>> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
>>> @@ -145,7 +145,16 @@ static inline int rcv_wqe_size(int max_sge)
>>> max_sge * sizeof(struct ib_sge);
>>> }
>>> -void free_rd_atomic_resource(struct rxe_qp *qp, struct resp_res *res);
>>> +static inline void free_rd_atomic_resource(struct resp_res *res)
>>> +{
>>> + if (res->type == RXE_ATOMIC_MASK) {
>>> + kfree_skb(res->atomic.skb);
>>> + } else if (res->type == RXE_READ_MASK) {
>>> + if (res->read.mr)
>>> + rxe_drop_ref(res->read.mr);
>>> + }
>>> + res->type = 0;
>>> +}
>>> static inline void rxe_advance_resp_resource(struct rxe_qp *qp)
>>> {
>>> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
>>> index 5f270cbf18c6..b29208852bc4 100644
>>> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
>>> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
>>> @@ -126,24 +126,13 @@ static void free_rd_atomic_resources(struct rxe_qp *qp)
>>> for (i = 0; i < qp->attr.max_dest_rd_atomic; i++) {
>>> struct resp_res *res = &qp->resp.resources[i];
>>> - free_rd_atomic_resource(qp, res);
>>> + free_rd_atomic_resource(res);
>>> }
>>> kfree(qp->resp.resources);
>>> qp->resp.resources = NULL;
>>> }
>>> }
>>> -void free_rd_atomic_resource(struct rxe_qp *qp, struct resp_res *res)
>>> -{
>>> - if (res->type == RXE_ATOMIC_MASK) {
>>> - kfree_skb(res->atomic.skb);
>>> - } else if (res->type == RXE_READ_MASK) {
>>> - if (res->read.mr)
>>> - rxe_drop_ref(res->read.mr);
>>> - }
>>> - res->type = 0;
>>> -}
>>> -
>>> static void cleanup_rd_atomic_resources(struct rxe_qp *qp)
>>> {
>>> int i;
>>> @@ -152,7 +141,7 @@ static void cleanup_rd_atomic_resources(struct rxe_qp *qp)
>>> if (qp->resp.resources) {
>>> for (i = 0; i < qp->attr.max_dest_rd_atomic; i++) {
>>> res = &qp->resp.resources[i];
>>> - free_rd_atomic_resource(qp, res);
>>> + free_rd_atomic_resource(res);
>>> }
>>> }
>>> }
>>> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
>>> index c369d78fc8e8..923a71ff305c 100644
>>> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
>>> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
>>> @@ -663,7 +663,7 @@ static enum resp_states read_reply(struct rxe_qp *qp,
>>> */
>>> res = &qp->resp.resources[qp->resp.res_head];
>>> - free_rd_atomic_resource(qp, res);
>>> + free_rd_atomic_resource(res);
>>> rxe_advance_resp_resource(qp);
>>> res->type = RXE_READ_MASK;
>>> @@ -977,7 +977,7 @@ static int send_atomic_ack(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
>>> }
>>> res = &qp->resp.resources[qp->resp.res_head];
>>> - free_rd_atomic_resource(qp, res);
>>> + free_rd_atomic_resource(res);
>>> rxe_advance_resp_resource(qp);
>>> skb_get(skb);
