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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxx>
CC: Junxiao Bi <junxiao.bi@xxxxxxxxxx>
Signed-off-by: Zhu Yanjun <yanjun.zhu@xxxxxxxxxx>
---
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?
Hi, Yuval

I checked the source code again. In rxe_send, rxe_add_ref is called.
So IMHO, this rxe_add_ref is not needed here.
...
send_atomic_ack
        rxe_xmit_packet
                rxe_send
...
int rxe_send(struct rxe_pkt_info *pkt, struct sk_buff *skb)
{
        struct rxe_av *av;
        int err;

        av = rxe_get_av(pkt);

        skb->destructor = rxe_skb_tx_dtor;
        skb->sk = pkt->qp->sk->sk;

        rxe_add_ref(pkt->qp);  <----here
...

If I am wrong, please correct me.

Thanks a lot.
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@xxxxxxxxxxxxxxx
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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux