| Message ID | 1521339691-27879-1-git-send-email-yanjun.zhu@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Sat, Mar 17, 2018 at 10:21:30PM -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. > > This can make the performance better and save the memory. > > CC: Srinivas Eeda <srinivas.eeda@oracle.com> > CC: Junxiao Bi <junxiao.bi@oracle.com> > Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com> > --- > drivers/infiniband/sw/rxe/rxe_recv.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c > index 4c3f899..5b8f293 100644 > --- a/drivers/infiniband/sw/rxe/rxe_recv.c > +++ b/drivers/infiniband/sw/rxe/rxe_recv.c > @@ -309,10 +309,13 @@ 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_copy = NULL; > + if (mce->qp_list.next != &mcg->qp_list) { > + skb_copy = skb; > + refcount_inc(&skb->users); > + } Something with the code after the change doesn't make sense to me, you save skb in skb_copy (bad name after the change) and then, few lines below skb is set to skb_copy. I think with this change we do not really need skb_copy, just do refcount_inc. No comments about the idea itself, just the code structure. Not related to this proposal, while here, can you remove the "if (skb)" in label err1? looks like kfree_skb takes care of it. > > pkt->qp = qp; > rxe_add_ref(qp); > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018/3/19 4:19, Yuval Shaia wrote: > On Sat, Mar 17, 2018 at 10:21:30PM -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. >> >> This can make the performance better and save the memory. >> >> CC: Srinivas Eeda <srinivas.eeda@oracle.com> >> CC: Junxiao Bi <junxiao.bi@oracle.com> >> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com> >> --- >> drivers/infiniband/sw/rxe/rxe_recv.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c >> index 4c3f899..5b8f293 100644 >> --- a/drivers/infiniband/sw/rxe/rxe_recv.c >> +++ b/drivers/infiniband/sw/rxe/rxe_recv.c >> @@ -309,10 +309,13 @@ 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_copy = NULL; >> + if (mce->qp_list.next != &mcg->qp_list) { >> + skb_copy = skb; >> + refcount_inc(&skb->users); >> + } > Something with the code after the change doesn't make sense to me, you save > skb in skb_copy (bad name after the change) and then, few lines below skb > is set to skb_copy. > > I think with this change we do not really need skb_copy, just do > refcount_inc. > > No comments about the idea itself, just the code structure. > > Not related to this proposal, while here, can you remove the "if (skb)" in Sure. I agree with you. I will make a new patch to fix it. Thanks a lot. Zhu Yanjun > label err1? looks like kfree_skb takes care of it. > >> >> pkt->qp = qp; >> rxe_add_ref(qp); >> -- >> 2.7.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >> the body of a message to majordomo@vger.kernel.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c index 4c3f899..5b8f293 100644 --- a/drivers/infiniband/sw/rxe/rxe_recv.c +++ b/drivers/infiniband/sw/rxe/rxe_recv.c @@ -309,10 +309,13 @@ 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_copy = NULL; + if (mce->qp_list.next != &mcg->qp_list) { + skb_copy = skb; + refcount_inc(&skb->users); + } pkt->qp = qp; rxe_add_ref(qp);
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. This can make the performance better and save the memory. CC: Srinivas Eeda <srinivas.eeda@oracle.com> CC: Junxiao Bi <junxiao.bi@oracle.com> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com> --- drivers/infiniband/sw/rxe/rxe_recv.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)