On Wed, Nov 15, 2017 at 11:10:54PM +0000, Parav Pandit wrote: > > -----Original Message----- > > From: Or Gerlitz [mailto:gerlitz.or@xxxxxxxxx] > > Sent: Wednesday, November 15, 2017 1:53 AM > > To: Parav Pandit <parav@xxxxxxxxxxxx> > > Cc: Doug Ledford <dledford@xxxxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx; Hefty, > > Sean <sean.hefty@xxxxxxxxx>; Leon Romanovsky <leonro@xxxxxxxxxxxx> > > Subject: Re: [PATCH rdma-next 28/31] IB/cm: Fix avoid sleep while spin lock is > > held > > > > On Wed, Nov 15, 2017 at 12:32 AM, Parav Pandit <parav@xxxxxxxxxxxx> > > wrote: > > >> From: Or Gerlitz [mailto:gerlitz.or@xxxxxxxxx] > > > > >> >>> In case of LAP are used for RoCE > > > > >> >> AFAIK APM is not supported with RoCE, isn't that? if yes, on what > > >> >> HW/RoCE mode and how do you test that? > > > > >> > This particular patch in this series is not tested on RoCE. > > >> > APM is not supported but injecting such frame in network is not impossible. > > >> > We don’t have particular checks to reject such frame for RoCE. > > >> > So I prefer to fix the issue before getting such hang/oops report > > >> > when issue is > > >> evident in code. > > >> > > >> mentioning testing, in this series you did bunch of fixes for the IB > > >> core, CM and CMA. > > >> > > >> Would be good to add a section in the cover letter mentioning under > > >> which type of testing the modified code has gone through. > > > > > It's been tested for RoCEv1, v2, IPoIB with ConnectX4, ConnectX3 HCA with > > perftest, rping, krping tools. > > > In few individual patch, it's been mentioned how is this tested. > > > > Please also add more info (and in the cover letter please) on IPv4 vs IPv6, unicast > > vs multicast, etc. > Is there a template that you can share that linux-rdma follows that I can use to mention what tests are done? > Any example past cover letters that I can refer as reference? > > Depending on how much information you expect in cover letter, this patchset can have many revisions just for cover letter. > So it would be best for you to share the template that I can fill in for v1. Parav, relax There is no need to respin. I reread responses and there are two types of response/disputes: 1. Jason vs. Erez. Jason is in favor of having memory leak as long as IPoIB doesn't filter input (in our case wrong input), Erez in favor of fixing memory leak. I'm with Erez. 2. Or vs. rest of the world. No doubts that the commit messages and cover letter are important, but they are not the most important part of this series to justify resubmission. And yes, you should listen to the feedback provided here and improve your commit messages in next submissions. > > > You touched the CMA and addr modules quite heavily on all these areas, need to > > cover them in testing. > Which are those tests that I should execute apart from above summary? > > > > > did you test APM? I don't think it would be correct to change the CM APM code > > without proper testing. > As I already told this particular patch is not tested. All others are needless to say tested. > I do not have APM setup to test this fix. > I am ok to drop this patch and provide fix when crash occurs later on. Not everything is possible to test, it doesn't mean that we shouldn't fix it. Thanks
Attachment:
signature.asc
Description: PGP signature
