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

Yanjun Zhu
2018-02-17T00:48:10Z

On 2018/2/14 18:36, Yuval Shaia wrote:
>>>> }
>>>> - 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.
> My concern is only with the above ref count.
Hi, Yuval

Thanks for your code review.

From the function send_atomic_ack, the variable skb is used in the
following 2 places.
"
...
res->type = RXE_ATOMIC_MASK;
res->atomic.skb = skb;           <----One is here.
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); <---The other is
here.
...
"
The 2 will call kfree_skb after skb is used.

void kfree_skb(struct sk_buff *skb)
{
if (!skb_unref(skb))
return;

trace_kfree_skb(skb, __builtin_return_address(0));
__kfree_skb(skb);
}

In the function kfree_skb, user ref is decreased and checked. To make
sure in the 2 places, skb can be used.
I increased user ref.

Best Regards,
Zhu Yanjun
>
>> 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