| Message ID | 1522469906-4745-2-git-send-email-yanjun.zhu@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
T24gU2F0LCAyMDE4LTAzLTMxIGF0IDAwOjE4IC0wNDAwLCBaaHUgWWFuanVuIHdyb3RlOg0KPiAr CQkJcmVmY291bnRfaW5jKCZyZXMtPmF0b21pYy5za2ItPnVzZXJzKTsNCg0KSXMgdGhpcyBwZXJo YXBzIGFuIG9wZW4tY29kZWQgc2tiX2dldCgpIGNhbGw/IElmIHNvLCB3aHkgaXNuJ3Qgc2tiX2dl dCgpDQpjYWxsZWQgaGVyZT8NCg0KVGhhbmtzLA0KDQpCYXJ0Lg0KDQoNCg0K -- 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 3/31/2018 7:18 AM, Zhu Yanjun wrote: > In the function duplicate_request, the reference of skb can be increased > to replace the function skb_clone. > > This will make rxe performace better and save memory. > did you make sure that when free_rd_atomic_resources() is called there is no double free ? or no free at all ? thanks. Yonatan. -- 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/4/1 17:12, Yonatan Cohen wrote: > On 3/31/2018 7:18 AM, Zhu Yanjun wrote: >> In the function duplicate_request, the reference of skb can be increased >> to replace the function skb_clone. >> >> This will make rxe performace better and save memory. >> > > did you make sure that when free_rd_atomic_resources() is called there > is no double free ? or no free at all ? Thanks for your reply. For several day's test after this patch is applied, I can not find double free error in softroce. If you can find it, please let me know how to reproduce it. Thanks a lot. Zhu Yanjun > thanks. > > Yonatan. > > > -- 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 Tue, Apr 03, 2018 at 10:58:34AM +0800, Yanjun Zhu wrote: > > > On 2018/4/1 17:12, Yonatan Cohen wrote: > > On 3/31/2018 7:18 AM, Zhu Yanjun wrote: > > > In the function duplicate_request, the reference of skb can be increased > > > to replace the function skb_clone. > > > > > > This will make rxe performace better and save memory. > > > > > > > did you make sure that when free_rd_atomic_resources() is called there > > is no double free ? or no free at all ? > Thanks for your reply. > For several day's test after this patch is applied, I can not find double > free error in softroce. If you can find it, please let me know how to > reproduce it. The CONFIG_DEBUG_OBJECTS_FREE option might be able to prove it. Thanks > > Thanks a lot. > Zhu Yanjun > > thanks. > > > > Yonatan. > > > > > > > > -- > 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/4/3 14:23, Leon Romanovsky wrote: > On Tue, Apr 03, 2018 at 10:58:34AM +0800, Yanjun Zhu wrote: >> >> On 2018/4/1 17:12, Yonatan Cohen wrote: >>> On 3/31/2018 7:18 AM, Zhu Yanjun wrote: >>>> In the function duplicate_request, the reference of skb can be increased >>>> to replace the function skb_clone. >>>> >>>> This will make rxe performace better and save memory. >>>> >>> did you make sure that when free_rd_atomic_resources() is called there >>> is no double free ? or no free at all ? >> Thanks for your reply. >> For several day's test after this patch is applied, I can not find double >> free error in softroce. If you can find it, please let me know how to >> reproduce it. > The CONFIG_DEBUG_OBJECTS_FREE option might be able to prove it. Your help is appreciated. I applied the above patch and built a linux kernel with the kernel option CONFIG_DEBUG_OBJECTS_FREE enabled. I deployed the kernel in my test environment. To now, the tests run for 1 hour. And I can not find any error. I will run for 3 or 4 days. If any error, I will let you know. Zhu Yanjun > > Thanks > > >> Thanks a lot. >> Zhu Yanjun >>> thanks. >>> >>> Yonatan. >>> >>> >>> >> -- >> 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/4/3 22:15, Yanjun Zhu wrote: > > > On 2018/4/3 14:23, Leon Romanovsky wrote: >> On Tue, Apr 03, 2018 at 10:58:34AM +0800, Yanjun Zhu wrote: >>> >>> On 2018/4/1 17:12, Yonatan Cohen wrote: >>>> On 3/31/2018 7:18 AM, Zhu Yanjun wrote: >>>>> In the function duplicate_request, the reference of skb can be >>>>> increased >>>>> to replace the function skb_clone. >>>>> >>>>> This will make rxe performace better and save memory. >>>>> >>>> did you make sure that when free_rd_atomic_resources() is called there >>>> is no double free ? or no free at all ? >>> Thanks for your reply. >>> For several day's test after this patch is applied, I can not find >>> double >>> free error in softroce. If you can find it, please let me know how to >>> reproduce it. >> The CONFIG_DEBUG_OBJECTS_FREE option might be able to prove it. > Your help is appreciated. > I applied the above patch and built a linux kernel with the kernel > option CONFIG_DEBUG_OBJECTS_FREE enabled. > I deployed the kernel in my test environment. > To now, the tests run for 1 hour. And I can not find any error. > I will run for 3 or 4 days. If any error, I will let you know. To now, the whole test system works well for 4 days. Zhu Yanjun > > Zhu Yanjun >> >> Thanks >> >> >>> Thanks a lot. >>> Zhu Yanjun >>>> thanks. >>>> >>>> Yonatan. >>>> >>>> >>>> >>> -- >>> 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 > -- 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/31 12:43, Bart Van Assche wrote: > On Sat, 2018-03-31 at 00:18 -0400, Zhu Yanjun wrote: >> + refcount_inc(&res->atomic.skb->users); > Is this perhaps an open-coded skb_get() call? If so, why isn't skb_get() > called here? Thanks. There are some places that use refcount_inc instead of skb_get. I will fix it with skb_get later in a patch. Zhu Yanjun > > Thanks, > > Bart. > > > -- 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
T24gU3VuLCAyMDE4LTA0LTA4IGF0IDA4OjU0ICswODAwLCBZYW5qdW4gWmh1IHdyb3RlOg0KPiAN Cj4gT24gMjAxOC8zLzMxIDEyOjQzLCBCYXJ0IFZhbiBBc3NjaGUgd3JvdGU6DQo+ID4gT24gU2F0 LCAyMDE4LTAzLTMxIGF0IDAwOjE4IC0wNDAwLCBaaHUgWWFuanVuIHdyb3RlOg0KPiA+ID4gKwkJ CXJlZmNvdW50X2luYygmcmVzLT5hdG9taWMuc2tiLT51c2Vycyk7DQo+ID4gDQo+ID4gSXMgdGhp cyBwZXJoYXBzIGFuIG9wZW4tY29kZWQgc2tiX2dldCgpIGNhbGw/IElmIHNvLCB3aHkgaXNuJ3Qg c2tiX2dldCgpDQo+ID4gY2FsbGVkIGhlcmU/DQo+IA0KPiBUaGFua3MuIFRoZXJlIGFyZSBzb21l IHBsYWNlcyB0aGF0IHVzZSByZWZjb3VudF9pbmMgaW5zdGVhZCBvZiBza2JfZ2V0LiANCj4gSSB3 aWxsIGZpeCBpdCB3aXRoIHNrYl9nZXQgbGF0ZXIgaW4gYSBwYXRjaC4NCg0KVGhhdCdzIGEgd2Vp cmQgYXBwcm9hY2guIFdoYXQgcHJldmVudHMgeW91IGZyb20gbW9kaWZ5aW5nIHRoaXMgcGF0Y2gg c3VjaA0KdGhhdCBpdCB1c2VzIHNrYl9nZXQoKT8NCg0KQmFydC4NCg0KDQoNCg0K -- 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 Sun, Apr 08, 2018 at 01:08:57AM +0000, Bart Van Assche wrote: > On Sun, 2018-04-08 at 08:54 +0800, Yanjun Zhu wrote: > > > > On 2018/3/31 12:43, Bart Van Assche wrote: > > > On Sat, 2018-03-31 at 00:18 -0400, Zhu Yanjun wrote: > > > > + refcount_inc(&res->atomic.skb->users); > > > > > > Is this perhaps an open-coded skb_get() call? If so, why isn't skb_get() > > > called here? > > > > Thanks. There are some places that use refcount_inc instead of skb_get. > > I will fix it with skb_get later in a patch. > > That's a weird approach. What prevents you from modifying this patch such > that it uses skb_get()? Bart, did the regression you reported in rxe get fixed? I don't really want to apply more patches until I see a fix.. Jason -- 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 Mon, 2018-04-09 at 11:17 -0600, Jason Gunthorpe wrote: > On Sun, Apr 08, 2018 at 01:08:57AM +0000, Bart Van Assche wrote: > > On Sun, 2018-04-08 at 08:54 +0800, Yanjun Zhu wrote: > > > > > > On 2018/3/31 12:43, Bart Van Assche wrote: > > > > On Sat, 2018-03-31 at 00:18 -0400, Zhu Yanjun wrote: > > > > > + refcount_inc(&res->atomic.skb->users); > > > > > > > > Is this perhaps an open-coded skb_get() call? If so, why isn't skb_get() > > > > called here? > > > > > > Thanks. There are some places that use refcount_inc instead of skb_get. > > > I will fix it with skb_get later in a patch. > > > > That's a weird approach. What prevents you from modifying this patch such > > that it uses skb_get()? > > Bart, did the regression you reported in rxe get fixed? I don't really > want to apply more patches until I see a fix.. Hello Jason, I'm currently busy with working on a fix for a block layer regression. I will retest the rdma_rxe driver as soon as I have finished with this. See also "[PATCH] blk-mq: Fix recently introduced races in the timeout handling code" (https://www.mail-archive.com/linux-block@vger.kernel.org/msg20073.html). Thanks, Bart.
On 2018/4/8 9:08, Bart Van Assche wrote: > On Sun, 2018-04-08 at 08:54 +0800, Yanjun Zhu wrote: >> On 2018/3/31 12:43, Bart Van Assche wrote: >>> On Sat, 2018-03-31 at 00:18 -0400, Zhu Yanjun wrote: >>>> + refcount_inc(&res->atomic.skb->users); >>> Is this perhaps an open-coded skb_get() call? If so, why isn't skb_get() >>> called here? >> Thanks. There are some places that use refcount_inc instead of skb_get. >> I will fix it with skb_get later in a patch. > That's a weird approach. What prevents you from modifying this patch such > that it uses skb_get()? There are some patches use refcount_inc instead of skb_get. If you insist, I will send a new patch soon. Zhu Yanjun > > Bart. > > > > -- 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
T24gVHVlLCAyMDE4LTA0LTEwIGF0IDA4OjIyICswODAwLCBZYW5qdW4gWmh1IHdyb3RlOg0KPiAN Cj4gT24gMjAxOC80LzggOTowOCwgQmFydCBWYW4gQXNzY2hlIHdyb3RlOg0KPiA+IE9uIFN1biwg MjAxOC0wNC0wOCBhdCAwODo1NCArMDgwMCwgWWFuanVuIFpodSB3cm90ZToNCj4gPiA+IE9uIDIw MTgvMy8zMSAxMjo0MywgQmFydCBWYW4gQXNzY2hlIHdyb3RlOg0KPiA+ID4gPiBPbiBTYXQsIDIw MTgtMDMtMzEgYXQgMDA6MTggLTA0MDAsIFpodSBZYW5qdW4gd3JvdGU6DQo+ID4gPiA+ID4gKwkJ CXJlZmNvdW50X2luYygmcmVzLT5hdG9taWMuc2tiLT51c2Vycyk7DQo+ID4gPiA+IA0KPiA+ID4g PiBJcyB0aGlzIHBlcmhhcHMgYW4gb3Blbi1jb2RlZCBza2JfZ2V0KCkgY2FsbD8gSWYgc28sIHdo eSBpc24ndCBza2JfZ2V0KCkNCj4gPiA+ID4gY2FsbGVkIGhlcmU/DQo+ID4gPiANCj4gPiA+IFRo YW5rcy4gVGhlcmUgYXJlIHNvbWUgcGxhY2VzIHRoYXQgdXNlIHJlZmNvdW50X2luYyBpbnN0ZWFk IG9mIHNrYl9nZXQuDQo+ID4gPiBJIHdpbGwgZml4IGl0IHdpdGggc2tiX2dldCBsYXRlciBpbiBh IHBhdGNoLg0KPiA+IA0KPiA+IFRoYXQncyBhIHdlaXJkIGFwcHJvYWNoLiBXaGF0IHByZXZlbnRz IHlvdSBmcm9tIG1vZGlmeWluZyB0aGlzIHBhdGNoIHN1Y2gNCj4gPiB0aGF0IGl0IHVzZXMgc2ti X2dldCgpPw0KPiANCj4gVGhlcmUgYXJlIHNvbWUgcGF0Y2hlcyB1c2UgcmVmY291bnRfaW5jIGlu c3RlYWQgb2Ygc2tiX2dldC4gSWYgeW91IA0KPiBpbnNpc3QsIEkgd2lsbCBzZW5kIGEgbmV3IHBh dGNoIHNvb24uDQoNCkhlbGxvIFlhbmp1biwNCg0KSW4gY2FzZSB5b3Ugd291bGQgbm90IGJlIGF3 YXJlIG9mIHRoaXM6IGZvciBMaW51eCBrZXJuZWwgcGF0Y2hlcywganVzdCBsaWtlDQpmb3IgcGF0 Y2hlcyBmb3IgYW55IG90aGVyIG9wZW4gc291cmNlIHByb2plY3QgSSBhbSBmYW1pbGlhciB3aXRo LCBpdCBpcw0KZXhwZWN0ZWQgdGhhdCByZWFzb25hYmxlIGZlZWRiYWNrIGlzIGFkZHJlc3NlZCBi ZWZvcmUgYSBwYXRjaCBnb2VzIHVwc3RyZWFtLg0KSGVuY2UgbXkgc3VycHJpc2Ugd2hlbiB5b3Ug cHJvcG9zZWQgdG8gaW50cm9kdWNlIHNrYl9nZXQoKSBsYXRlciBpbnN0ZWFkIG9mDQptYWtpbmcg dGhpcyBjaGFuZ2UgYmVmb3JlIHRoaXMgcGF0Y2ggZ29lcyB1cHN0cmVhbS4NCg0KVGhhbmtzLA0K DQpCYXJ0Lg0KDQoNCg0K -- 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/4/10 9:14, Bart Van Assche wrote: > On Tue, 2018-04-10 at 08:22 +0800, Yanjun Zhu wrote: >> On 2018/4/8 9:08, Bart Van Assche wrote: >>> On Sun, 2018-04-08 at 08:54 +0800, Yanjun Zhu wrote: >>>> On 2018/3/31 12:43, Bart Van Assche wrote: >>>>> On Sat, 2018-03-31 at 00:18 -0400, Zhu Yanjun wrote: >>>>>> + refcount_inc(&res->atomic.skb->users); >>>>> Is this perhaps an open-coded skb_get() call? If so, why isn't skb_get() >>>>> called here? >>>> Thanks. There are some places that use refcount_inc instead of skb_get. >>>> I will fix it with skb_get later in a patch. >>> That's a weird approach. What prevents you from modifying this patch such >>> that it uses skb_get()? >> There are some patches use refcount_inc instead of skb_get. If you >> insist, I will send a new patch soon. > Hello Yanjun, > > In case you would not be aware of this: for Linux kernel patches, just like > for patches for any other open source project I am familiar with, it is > expected that reasonable feedback is addressed before a patch goes upstream. > Hence my surprise when you proposed to introduce skb_get() later instead of > making this change before this patch goes upstream. In the patch commit 99dae690255e90f5cbefcc76ad92b35cdf87d14d Author: Zhu Yanjun <yanjun.zhu@oracle.com> Date: Wed Mar 21 04:08:37 2018 -0400 IB/rxe: optimize mcast recv process 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> Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> commit 86af617641512f4aeb78fd25dcec7e0f4bb1d5e5 Author: Zhu Yanjun <yanjun.zhu@oracle.com> Date: Tue Feb 27 06:04:32 2018 -0500 IB/rxe: remove unnecessary skb_clone In send_atomic_ack function, it is not necessary to make a skb_clone. To gain better performance (high throughput and 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. Based on 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 test results 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@oracle.com> CC: Junxiao Bi <junxiao.bi@oracle.com> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com> Signed-off-by: Doug Ledford <dledford@redhat.com> refcount_inc is used. So in my opinion, I want to make a new patch to fix all the 3 patches. If you insist, I will make a new patch. And I will fix the above patches soon. Zhu Yanjun > > Thanks, > > Bart. > > > -- 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
T24gVHVlLCAyMDE4LTA0LTEwIGF0IDA5OjI4ICswODAwLCBZYW5qdW4gWmh1IHdyb3RlOg0KPiBJ ZiB5b3UgaW5zaXN0LCBJIHdpbGwgbWFrZSBhIG5ldyBwYXRjaC4NCg0KUGxlYXNlIHByZXBhcmUs IHRlc3QgYW5kIHBvc3QgYSBuZXcgcGF0Y2guDQoNClRoYW5rcywNCg0KQmFydC4NCg0KDQoNCg0K -- 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_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c index d37bb9b..5b6916e 100644 --- a/drivers/infiniband/sw/rxe/rxe_resp.c +++ b/drivers/infiniband/sw/rxe/rxe_resp.c @@ -1133,24 +1133,13 @@ static enum resp_states duplicate_request(struct rxe_qp *qp, /* Find the operation in our list of responder resources. */ res = find_resource(qp, pkt->psn); if (res) { - struct sk_buff *skb_copy; - - skb_copy = skb_clone(res->atomic.skb, GFP_ATOMIC); - if (skb_copy) { - rxe_add_ref(qp); /* for the new SKB */ - } else { - pr_warn("Couldn't clone atomic resp\n"); - rc = RESPST_CLEANUP; - goto out; - } - + refcount_inc(&res->atomic.skb->users); /* Resend the result. */ rc = rxe_xmit_packet(to_rdev(qp->ibqp.device), qp, - pkt, skb_copy); + pkt, res->atomic.skb); if (rc) { pr_err("Failed resending result. This flow is not handled - skb ignored\n"); - rxe_drop_ref(qp); - kfree_skb(skb_copy); + kfree_skb(res->atomic.skb); rc = RESPST_CLEANUP; goto out; }
In the function duplicate_request, the reference of skb can be increased to replace the function skb_clone. This will make rxe performace better and save 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_resp.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)