On Wed, Sep 29, 2021 at 12:02 PM Shoaib Rao <rao.shoaib@xxxxxxxxxx> wrote: > > > On 9/28/21 2:58 AM, Zhu Yanjun wrote: > > On Tue, Sep 28, 2021 at 5:41 PM Shoaib Rao <rao.shoaib@xxxxxxxxxx> wrote: > >> > >> On 9/27/21 11:55 PM, Zhu Yanjun wrote: > >>> On Tue, Sep 28, 2021 at 12:38 PM Shoaib Rao <rao.shoaib@xxxxxxxxxx> wrote: > >>>> On 9/27/21 6:46 PM, Zhu Yanjun wrote: > >>>>> On Tue, Sep 28, 2021 at 3:19 AM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote: > >>>>>> On Tue, Sep 14, 2021 at 06:12:20PM -0700, Rao Shoaib wrote: > >>>>>>> In our internal testing we have found that > >>>>>>> default maximum values are too small. > >>>>>>> Ideally there should be no limits, but since > >>>>>>> maximum values are reported via ibv_query_device, > >>>>>>> we have to return some value. So, the default > >>>>>>> maximums have been changed to large values. > >>>>>>> > >>>>>>> Signed-off-by: Rao Shoaib <Rao.Shoaib@xxxxxxxxxx> > >>>>>>> --- > >>>>>>> > >>>>>>> Resubmitting the patch after applying Bob's latest patches and testing > >>>>>>> using via rping. > >>>>>>> > >>>>>>> drivers/infiniband/sw/rxe/rxe_param.h | 30 ++++++++++++++------------- > >>>>>>> 1 file changed, 16 insertions(+), 14 deletions(-) > >>>>>> So are we good with this? Bob? Zhu? > >>>>> I have already checked this commit. And I have found 2 problems with > >>>>> this commit. > >>>>> This commit changes many MAXs. > >>>>> And now rxe is not stable enough. Not sure this commit will cause the > >>>>> new problems. > >>>>> > >>>>> Zhu Yanjun > >>>> Hi Zhu, > >>>> > >>>> A generic statement without any technical data does not help. As far as > >>>> I am aware, currently there are no outstanding issues. If there are, > >>>> please provide data that clearly shows that the issue is caused by this > >>>> patch. > >>> Hi, Shoaib > >>> > >>> With this commit, I found 2 problems. > >>> This is why I suspect that this commit will introduce risks. > >> Hi Zhu, > >> > >> I did full testing before I sent the patch, that is how I found that > >> rping did not work. What are the issues that you found? How to I > >> reproduce those issues? > > Sorry. What tests do you make? > > > > Do you make tests with the followings: > > > > 1. your commit + latest kernel <------rping------- > 5.10 stable kernel > > 2. your commit + latest kernel < ------rping------- > 5.11 stable kernel > > 3. your commit + latest kernel < ------rping------- > 5.12 stable kernel > > 4. your commit + latest kernel < ------rping------- > 5.13 stable kernel > > 5. your commit + latest kernel < ------rping------- > 5.14 stable kernel > > 6. rdma-core tests with your commit + latest kernel > > > > Zhu Yanjun > > Hi Zhu, > > With all due respect, I am a little surprised by the special treatment > being given to this rather simple patch. Normally comments to a patch > are submitted with technical data to back them up. In this case none > have been provided. If we are going by gut feelings, then there are more > involved patches that are being accepted without any tests as listed above. All the commits that I reviewed will pass the mentioned tests. But your commit failed to pass the tests at least 2 times. So I suggest that you make the above tests in your local hosts. This will save us time. I do not treat your commit specially. > > My initial patch increased the value of 3 variables but the community > suggested that values of other variables should be increased as well. > The discussion happened on this mailing list and no objections were raised. > > The two issues that you mention were mechanical issues, (one reported by > you and one by the kernel bot) that have been fixed. They had nothing to > do with the values being increased. As I have said I have tested the > patch several times, most recently about a week or so ago with Bob's > latest change. I do not know how you tested your patch in your host. If your tests include backward compatibility and rdma-core tests that I mentioned in the above mail, I am fine with this commit. Zhu Yanjun > > I am not keen on changing the values of any other parameters, but the 3 > that were contained in my original patch. The link to those patches is > > https://www.spinics.net/lists/linux-rdma/msg103570.html > > Please let me know if those are acceptable. They have been tested > *extensively* running several different kinds of Oracle DB workloads. > > Regards, > > Shoaib > > > > >> Shoaib > >> > >>> Before a commit is sent to the upstream, please make full tests with it. > >>> > >>> Zhu Yanjun > >>> > >>>> Thanks you. > >>>> > >>>> Shoaib > >>>> > >>>>>>> - RXE_MAX_MR_INDEX = 0x00010000, > >>>>>>> + RXE_MAX_MR_INDEX = DEFAULT_MAX_VALUE, > >>>>>>> + RXE_MAX_MR = DEFAULT_MAX_VALUE - RXE_MIN_MR_INDEX, > >>>>>> Bob, were you saying this was what needed to be bigger to pass > >>>>>> blktests?? > >>>>>> > >>>>>> Jason
