Re: [LTP] [PATCH] doc: Add ground rules page

[Petr Vorel <pvorel@suse.cz>]

> This is a continued effort to write down the unwritten rules we have in
> the project. Feel free to suggest more topics for the page.

Nice work, thanks for doing this!
Few spell checker issues and style below.
I'd slightly prefer avoiding "I" (first-person) style, i.e. use impersonal
language / passive (it's not a blog post but official document), but it's up to
you.

Acked-by: Petr Vorel <pvorel@suse.cz>

> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  doc/developers/ground_rules.rst | 68 +++++++++++++++++++++++++++++++++
>  doc/index.rst                   |  1 +
>  2 files changed, 69 insertions(+)
>  create mode 100644 doc/developers/ground_rules.rst

> diff --git a/doc/developers/ground_rules.rst b/doc/developers/ground_rules.rst
> new file mode 100644
> index 000000000..701dcd09a
> --- /dev/null
> +++ b/doc/developers/ground_rules.rst
> @@ -0,0 +1,68 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Ground Rules
> +============
> +
> +Do not work around kernel bugs
> +------------------------------
> +
> +We have decided what we will not work around bugs in upstream LTP sources. If a
> +test fails on your system for a good reason, e.g. patch wasn't backported and
> +the bug is present, work around for this will not be accepted upstream. The
> +main reason for this decision is that this masks the failure for everyone else.
> +
> +
> +Do not synchronize by sleep
> +---------------------------
> +
> +Why is sleep in tests bad then?
> +```````````````````````````````
> +
> +The first problem is that it may and will introduce very rare test failures,
"that it may and will introduce" => "that it will introduce" or "that it will
likely introduce"

> +that means somebody has to spend time looking into these, which is a wasted
> +effort. Also I'm pretty sure that nobody likes tests that will fail rarely for
nit: first-person style, maybe:

effort. Nobody likes tests that ...

> +no good reason. Even more so you cannot run such tests with a background load
> +to ensure that everything works correctly on a bussy system, because that will
bussy => busy

> +increase the likehood of a failure.
> +
> +The second problem is that this wastes resources and slowns down a test run. If
slowns down => slows down

> +you think that adding a sleep to a test is not a big deal, let me put things
> +into a perspective. There is about 1600 syscall tests in Linux Test Project
Also here is first-person style...
> +(LTP), if 7.5% of them would sleep just for one second, we would end up with
> +two minutes of wasted time per testrun. In practice most of the test I've seen
test => tests
> +waited for much longer just to be sure that things will works even on slower

and here: In practice most of the tests wait ...

> +hardware. With sleeps between 2 and 5 seconds that puts us somewhere between 4
> +and 10 minutes which is between 13% and 33% of the syscall runtime on my dated

and here: the syscall runtime on outdated Thinkpad.

> +thinkpad, where the run finishes in a bit less than half an hour. It's even
> +worse on newer hardware, because this slowdown will not change no matter how
> +fast your machine is, which is maybe the reason why this was acceptable twenty
> +years ago but it's not now.
> +
> +
> +What to do instead?
> +```````````````````
> +
> +Use proper synchronization.
> +
> +There are different problems and different solutions.
> +
> +Most often tests needs to synchronize between child and parent proces.
Maybe join this sentence to the previous one?

> +
> +The easiest case is that parent needs to wait for a child to finish, that can
> +be fixed just be adding a waitpid() in the parent which ensures that child is
very nit: if you use :man2:`waitpid`, it will link the text to man page.

> +finished before parent runs.
> +
> +Commonly child has to execute certain piece of code before parent can continue.
> +For that LTP library implements checkpoints with simple wait and wake functions
> +based on futexes on a piece of shared memory set up by the test library.

Maybe mention some functions, e.g. TST_CHECKPOINT_WAIT()? You could link the
macro with:

:c:func:`TST_CHECKPOINT_WAIT()`

> +
> +Another common case is where child must sleep in a syscall before parent can
> +continue, for which we have a helper that polls /proc/$PID/stat.
> +
> +Less often tests needs to wait for an action that is done asynchronously, or a

tests needs => tests need

> +kernel resource deallocation is deffered to a later time. In such cases the
deffered => deferred

> +best we can do is to poll. In LTP we ended up with a macro that polls by
Again, please mention the function. Unfortunately include/tst_fuzzy_sync.h have
not been converted to kerneldoc, therefore it can't be referenced yet.

> +calling a piece of code in a loop with exponentially increasing sleeps between
> +retries until it succeeeds. Which means that instead of sleeping for a maximal
succeeeds => succeeds

> +time event can possibly take the sleep is capped by twice of the optimal
> +sleeping time while we avoid polling too aggressively.
> diff --git a/doc/index.rst b/doc/index.rst
> index 06b75616f..659549cc3 100644
> --- a/doc/index.rst
> +++ b/doc/index.rst
> @@ -19,6 +19,7 @@
>     :hidden:
>     :caption: For developers

> +   developers/ground_rules
>     developers/setup_mailinglist
>     developers/writing_tests
>     developers/test_case_tutorial

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp