Re: [PATCH v3 0/3] ntb_perf: add new 'latency' test set

[Serge Semin <fancer.lancer@gmail.com>]

Hi folks,

On Mon, Jun 20, 2022 at 10:42:54AM -0400, Jon Mason wrote:
> On Mon, Jun 20, 2022 at 01:20:14PM +0300, Alexander Fomichev wrote:
> > Hi Serge,
> > 
> > On Wed, May 25, 2022 at 11:58:01AM +0300, Serge Semin wrote:
> > > 
> > > Ok. I'll have a look at the series on this week.
> > > 
> > 
> > I kindly remind you about the review.

I am very sorry for the huge delay with the review especially after I
promised to have a look at the series one month ago.

> 

> I've pulled this into my ntb branch.  So, barring a nack by Serge,
> I'll push it for v5.20.  And if I see an ack by him, I'll try to add
> that in as well :)

I've thoroughly looked at the entire series and IMO in the current
state I wouldn't recommend to merge it in.
The main issue is that after applying all the changes the ntb_perf
driver will get extended greatly with three additional sub-tests
and thus will loose its coherency. It gets to be obvious after
the patch 2 and 3 applied, which introduce additional client-server
semantic and imply allocating their-own private data. All of that
makes the code much harder to read and breaks the driver specialization.

The latency tests as them-self are very useful though. But it would be
much better to have them implemented in a separate driver
"ntb_latency" or something.

Please also note, there is a special test-script, which rely on the
certain test drivers semantic:
tools/testing/selftests/ntb/ntb_test.sh
It looks like the suggested patches don't change the already defined
ntb_perf DebugFS interface, but still may cause the script to fail if the
latency on the particular system will get measured too high. So should
we have the latency-part in a separate driver, the script won't get
affected by it. If it is required the script could be updated accordingly
taking the new driver specifics into account.

Regarding the tests implementation. As I see it failing the latency
measurements if they're performed with the too few retries isn't a good
idea. Alexander, you said that normally performing 1000 retries is
enough to get the latency with a good precision, but the test driver
returns an error if the number of retries is less than 20. So what
happens between 20 and 1000? The tests get passed, but the results
aren't accurate or what? If so then why don't the test fail in the
case of 30 iterations too? IMO as long as you don't define the strong
accuracy criteria, the failure condition shouldn't be determined by
the number of iterations. So if I were you I would execute the latency
tests with the specified "lat_time_ms" duration and printed a warning
if the number of iterations turned to be too low (100, 200?) most
likely causing to have inaccurate results, but still would calculate
the latency from the determined numbers (even if there were only one
iteration performed).

I am very sorry to spilling it out at this stage. I should have done
it on v1 or v2. Anyway it's up to the driver/subsystem maintainers
(Dave, Jon) to decide whether the suggested update is suitable despite
of all my thoughts.

-Sergey

> 
> Thanks,
> Jon
> 
> > Thanks.
> > 
> > -- 
> > Regards,
> >   Alexander