| Message ID | 1521534216-13683-1-git-send-email-yanjun.zhu@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Zhu, Sorry but i have few more questions. Yuval On Tue, Mar 20, 2018 at 04:23:36AM -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. "can"?? If you have numbers suggesting to add them to commit message. Also, suggesting to use this header instead: IB/rxe: optimize mcast recv process > > CC: Srinivas Eeda <srinivas.eeda@oracle.com> > CC: Junxiao Bi <junxiao.bi@oracle.com> > Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com> > --- > V2->v3: Following Yuval's advice, the temp variable is removed. > V1->V2: Following Yuval's advice, skb test is removed. > --- > drivers/infiniband/sw/rxe/rxe_recv.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c > index 4c3f899..04beecd 100644 > --- a/drivers/infiniband/sw/rxe/rxe_recv.c > +++ b/drivers/infiniband/sw/rxe/rxe_recv.c > @@ -276,7 +276,6 @@ 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 rxe_mc_elem *mce; > struct rxe_qp *qp; > union ib_gid dgid; > @@ -309,18 +308,14 @@ 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 > + * increase the users of the skb then post to the next qp > */ > - skb_copy = (mce->qp_list.next != &mcg->qp_list) ? > - skb_clone(skb, GFP_ATOMIC) : NULL; > + if (mce->qp_list.next != &mcg->qp_list) > + refcount_inc(&skb->users); Do we need somewhere to take care for refcount_dec? I assume not as expecting kfree_skb to do so but just wanted to make sure. > > pkt->qp = qp; > rxe_add_ref(qp); > rxe_rcv_pkt(rxe, pkt, skb); > - > - skb = skb_copy; > - if (!skb) > - break; > } > > spin_unlock_bh(&mcg->mcg_lock); > @@ -328,8 +323,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); With or without accepting the comments/suggestions - lgtm. Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com> > } > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018/3/20 17:56, Yuval Shaia wrote: > Hi Zhu, > Sorry but i have few more questions. > > Yuval > > On Tue, Mar 20, 2018 at 04:23:36AM -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. > "can"?? > If you have numbers suggesting to add them to commit message. > > Also, suggesting to use this header instead: > > IB/rxe: optimize mcast recv process OK. I will use this "IB/rxe: optimize mcast recv process".:-) I will make a new patch right now. > >> CC: Srinivas Eeda <srinivas.eeda@oracle.com> >> CC: Junxiao Bi <junxiao.bi@oracle.com> >> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com> >> --- >> V2->v3: Following Yuval's advice, the temp variable is removed. >> V1->V2: Following Yuval's advice, skb test is removed. >> --- >> drivers/infiniband/sw/rxe/rxe_recv.c | 14 ++++---------- >> 1 file changed, 4 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c >> index 4c3f899..04beecd 100644 >> --- a/drivers/infiniband/sw/rxe/rxe_recv.c >> +++ b/drivers/infiniband/sw/rxe/rxe_recv.c >> @@ -276,7 +276,6 @@ 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 rxe_mc_elem *mce; >> struct rxe_qp *qp; >> union ib_gid dgid; >> @@ -309,18 +308,14 @@ 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 >> + * increase the users of the skb then post to the next qp >> */ >> - skb_copy = (mce->qp_list.next != &mcg->qp_list) ? >> - skb_clone(skb, GFP_ATOMIC) : NULL; >> + if (mce->qp_list.next != &mcg->qp_list) >> + refcount_inc(&skb->users); > Do we need somewhere to take care for refcount_dec? I assume not as > expecting kfree_skb to do so but just wanted to make sure. Yeah, I agree with you. kfree_skb do the work. > >> >> pkt->qp = qp; >> rxe_add_ref(qp); >> rxe_rcv_pkt(rxe, pkt, skb); >> - >> - skb = skb_copy; >> - if (!skb) >> - break; >> } >> >> spin_unlock_bh(&mcg->mcg_lock); >> @@ -328,8 +323,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); > With or without accepting the comments/suggestions - lgtm. > > Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com> Cool! Thanks you very much.:-D Zhu Yanjun > >> } >> >> 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@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..04beecd 100644 --- a/drivers/infiniband/sw/rxe/rxe_recv.c +++ b/drivers/infiniband/sw/rxe/rxe_recv.c @@ -276,7 +276,6 @@ 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 rxe_mc_elem *mce; struct rxe_qp *qp; union ib_gid dgid; @@ -309,18 +308,14 @@ 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 + * increase the users of the skb then post to the next qp */ - skb_copy = (mce->qp_list.next != &mcg->qp_list) ? - skb_clone(skb, GFP_ATOMIC) : NULL; + if (mce->qp_list.next != &mcg->qp_list) + refcount_inc(&skb->users); pkt->qp = qp; rxe_add_ref(qp); rxe_rcv_pkt(rxe, pkt, skb); - - skb = skb_copy; - if (!skb) - break; } spin_unlock_bh(&mcg->mcg_lock); @@ -328,8 +323,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)
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> --- V2->v3: Following Yuval's advice, the temp variable is removed. V1->V2: Following Yuval's advice, skb test is removed. --- drivers/infiniband/sw/rxe/rxe_recv.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)