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

[Alexander Fomichev <fomichev.ru@gmail.com>]

Hi Serge,

Thank you for the very detailed comments.

On Tue, Jun 21, 2022 at 05:05:37PM +0300, Serge Semin wrote:
> 
> 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.
>
As described in the cover commit message, the resulting test is fully
backward compatible. I mean that if we don't fiddle with the new sysfs
entries, then no latency measures are performed, and the test works as
it did before the patch set.
Also, I plan to make changes to "ntb_test.sh" script accordingly, after
this patch set is merged.

> 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).
> 
Reasonable. I can easily change this part.

> 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.
>
The whole 'latency' part relies on 'ntb_perf' infrastructure. Moreover,
the first patch adds only one meaningful function.  Thus separatin theg
'latency' part will make me copy a lot of code. As a compromise, I can
offer to put latency-related code into a separate .c file but leave the
whole test in a single module. That should increase readability and
eliminate code duplication.

> 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.
> 
Let's call for the third opinion.


-- 
Regards,
  Alexander