Re: [PATCH 1/1] RDMA/rxe: Compact the function free_rd_atomic_resource

[Yanjun Zhu <yanjun.zhu@linux.dev>]

在 2022/5/17 23:49, Bob Pearson 写道:
> On 5/17/22 10:14, Yanjun Zhu wrote:
>> 在 2022/5/17 22:51, Bob Pearson 写道:
>>> On 5/17/22 14:08, yanjun.zhu@linux.dev wrote:
>>>> From: Zhu Yanjun <yanjun.zhu@linux.dev>
>>>>
>>>> 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.

The above 4 functions are in rxe_qp.c because these functions will use 
qp as parameter.

Now free_rd_atomic_resource will not use qp as paramter and other files 
also will call this function.

As such it is a good idea to put this function free_rd_atomic_resource 
to the head file.

Zhu Yanjun

>
> 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@linux.dev>
>>>> ---
>>>>    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);