On Sun, Mar 18, 2018 at 11:20:52PM -0400, Zhu Yanjun wrote:
> In mcast recv process, the function skb_clone is used. In fact,
> the refcount can be increased to replace cloning a new skb since
> the original skb will not be modified before it is freed.
Even in the recv flow?
>
> This can make the performance better and save the memory.
>
> CC: Srinivas Eeda <srinivas.eeda@xxxxxxxxxx>
> CC: Junxiao Bi <junxiao.bi@xxxxxxxxxx>
> Signed-off-by: Zhu Yanjun <yanjun.zhu@xxxxxxxxxx>
> ---
> V1->V2: Following Yuval's advice, skb test is removed.
> ---
> drivers/infiniband/sw/rxe/rxe_recv.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c
> index 4c3f899..e0e4202 100644
> --- a/drivers/infiniband/sw/rxe/rxe_recv.c
> +++ b/drivers/infiniband/sw/rxe/rxe_recv.c
> @@ -276,7 +276,7 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
> {
> struct rxe_pkt_info *pkt = SKB_TO_PKT(skb);
> struct rxe_mc_grp *mcg;
> - struct sk_buff *skb_copy;
> + struct sk_buff *skb_reference;
> struct rxe_mc_elem *mce;
> struct rxe_qp *qp;
> union ib_gid dgid;
> @@ -309,16 +309,19 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
> continue;
>
> /* if *not* the last qp in the list
> - * make a copy of the skb to post to the next qp
> + * make a reference of the skb to post to the next qp
> */
> - skb_copy = (mce->qp_list.next != &mcg->qp_list) ?
> - skb_clone(skb, GFP_ATOMIC) : NULL;
> + skb_reference = NULL;
> + if (mce->qp_list.next != &mcg->qp_list) {
> + skb_reference = skb;
> + refcount_inc(&skb->users);
> + }
My question from v1 remains, i don't see why we need this temp variable
(skb_clone or skb_reference).
As i see it (unless i'm wrong), code can be something like:
if (mce->qp_list.next != &mcg->qp_list)
refcount_inc(&skb->users);
Then do the recv and skb remains as it was since we didn't cloned, just inc
ref.
>
> pkt->qp = qp;
> rxe_add_ref(qp);
> rxe_rcv_pkt(rxe, pkt, skb);
>
> - skb = skb_copy;
> + skb = skb_reference;
> if (!skb)
> break;
Also, this check i believe covers only the case where skb_clone fails to
allocate memory since the exit criteria (loop on QP list) is enough and
because we no longer have skb_clone then skb **can never** be NULL.
> }
> @@ -328,8 +331,7 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
> rxe_drop_ref(mcg); /* drop ref from rxe_pool_get_key. */
>
> err1:
> - if (skb)
> - kfree_skb(skb);
> + kfree_skb(skb);
> }
>
> static int rxe_match_dgid(struct rxe_dev *rxe, struct sk_buff *skb)
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
