Re: [PATCHv2 1/1] IB/rxe: remove unnecessary skb_clone

[Yanjun Zhu <yanjun.zhu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>]

On 2018/2/13 19:21, Yuval Shaia wrote:
> On Tue, Feb 13, 2018 at 02:59:46AM -0500, Zhu Yanjun wrote:
>> In send_atomic_ack function, it is not necessary make a
> s/make/to make
>
>> skb_clone. To gain better performance(high throughput and
> s/e(h/e (h
>
>> low latency), this skb_clone is removed.
>>
>> The following tests are made.
>>
>>   server                       client
>> ---------                    ---------
>> |1.1.1.1|<----rxe-channel--->|1.1.1.2|
>> ---------                    ---------
>>
>> On server: rping -s -a 1.1.1.1 -v -C 1000 -S 512
>> On client: rping -c -a 1.1.1.1 -v -C 1000 -S 512
>>
>> The kernel config CONFIG_DEBUG_KMEMLEAK is enabled on both server
>> and client.
>>
>> This test runs for several hours. There is no memory leak and the whole
>> system can work well.
>>
>> As the above network, the following tests are made.
>>
>> Server: ibv_rc_pingpong -d rxe0 -g 1
>> Client: ibv_rc_pingpong -d rxe0 -g 1 1.1.1.1
>>
>> The result on Server(10 tests are made).
>> Before:
>> Throughput is 137.07 Mbit/sec
>> Latency is 517.76 usec/iter
>>
>> After:
>> Throughput is 148.85 Mbit/sec
>> Latency is 476.64 usec/iter
>>
>> The throughput is enhanced and the latency is reduced.
>>
>> CC: Srinivas Eeda <srinivas.eeda-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>> CC: Junxiao Bi <junxiao.bi-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Zhu Yanjun <yanjun.zhu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>> ---
>> V1-->V2: 10 tests are made. From throughput and latency, the performance is better.
>> ---
>>   drivers/infiniband/sw/rxe/rxe_resp.c | 15 ++-------------
>>   1 file changed, 2 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
>> index d37bb9b..6d01d16 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
>> @@ -969,7 +969,6 @@ static int send_atomic_ack(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
>>   	int rc = 0;
>>   	struct rxe_pkt_info ack_pkt;
>>   	struct sk_buff *skb;
>> -	struct sk_buff *skb_copy;
>>   	struct rxe_dev *rxe = to_rdev(qp->ibqp.device);
>>   	struct resp_res *res;
>>   
>> @@ -981,15 +980,6 @@ static int send_atomic_ack(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
>>   		goto out;
>>   	}
>>   
>> -	skb_copy = skb_clone(skb, GFP_ATOMIC);
>> -	if (skb_copy)
>> -		rxe_add_ref(qp); /* for the new SKB */
> Are you sure we don't need this?
 From my stress tests and performance tests, it will get better 
performance to remove skb_clone.
And there is no memory leak.  The whole soft RoCE can work well.
So I think removing this function is a good choice.

Zhu Yanjun
>
>> -	else {
>> -		pr_warn("Could not clone atomic response\n");
>> -		rc = -ENOMEM;
>> -		goto out;
>> -	}
>> -
>>   	res = &qp->resp.resources[qp->resp.res_head];
>>   	free_rd_atomic_resource(qp, res);
>>   	rxe_advance_resp_resource(qp);
>> @@ -998,17 +988,16 @@ static int send_atomic_ack(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
>>   	memset((unsigned char *)SKB_TO_PKT(skb) + sizeof(ack_pkt), 0,
>>   	       sizeof(skb->cb) - sizeof(ack_pkt));
>>   
>> +	refcount_inc(&skb->users);
>>   	res->type = RXE_ATOMIC_MASK;
>>   	res->atomic.skb = skb;
>>   	res->first_psn = ack_pkt.psn;
>>   	res->last_psn  = ack_pkt.psn;
>>   	res->cur_psn   = ack_pkt.psn;
>>   
>> -	rc = rxe_xmit_packet(rxe, qp, &ack_pkt, skb_copy);
>> +	rc = rxe_xmit_packet(rxe, qp, &ack_pkt, skb);
>>   	if (rc) {
>>   		pr_err_ratelimited("Failed sending ack\n");
>> -		rxe_drop_ref(qp);
>> -		kfree_skb(skb_copy);
>>   	}
> With this modification suggesting to remove the curly braces.
>
>>   
>>   out:
>> -- 
>> 2.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html