[LTP] [PATCH 0/2] Add ground rules doc page

[LTP] [PATCH 0/2] Add ground rules doc page

[Cyril Hrubis]

Changes in v4:

- fixed process state doc (Petr)

- changed the manpage links to match the new format

- adjusted "Split changes" paragrap a suggested by Li

- fixed heading levels (so that headings show up properly in menu)

Cyril Hrubis (2):
  doc: Document process_state
  doc: Add ground rules page

 doc/developers/api_c_tests.rst  |   5 +
 doc/developers/ground_rules.rst | 176 ++++++++++++++++++++++++++++++++
 doc/index.rst                   |   1 +
 include/tst_process_state.h     |  51 +++++----
 4 files changed, 213 insertions(+), 20 deletions(-)
 create mode 100644 doc/developers/ground_rules.rst

-- 
2.52.0


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

[LTP] [PATCH 1/2] doc: Document process_state

[Cyril Hrubis]

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
 doc/developers/api_c_tests.rst |  5 ++++
 include/tst_process_state.h    | 51 +++++++++++++++++++++-------------
 2 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/doc/developers/api_c_tests.rst b/doc/developers/api_c_tests.rst
index be3baeaf0..47b7da6b4 100644
--- a/doc/developers/api_c_tests.rst
+++ b/doc/developers/api_c_tests.rst
@@ -43,6 +43,11 @@ Kernel
 .. kernel-doc:: ../../include/tst_kernel.h
 .. kernel-doc:: ../../include/tst_kvercmp.h
 
+Process state
+-------------
+
+.. kernel-doc:: ../../include/tst_process_state.h
+
 NUMA
 ----
 .. kernel-doc:: ../../include/tst_numa.h
diff --git a/include/tst_process_state.h b/include/tst_process_state.h
index b1d83e109..34e083313 100644
--- a/include/tst_process_state.h
+++ b/include/tst_process_state.h
@@ -15,39 +15,50 @@
 
 #ifdef TST_TEST_H__
 
-/*
- * Waits for process state change.
+/**
+ * TST_PROCESS_STATE_WAIT() - Waits for a process state change.
+ *
+ * Polls `/proc/$PID/state` for a process state changes.
  *
- * The state is one of the following:
+ * @pid: A process pid.
+ * @state: A state to wait for.
+ * @msec_timeout: A timeout for the wait.
  *
- * R - process is running
- * S - process is sleeping
- * D - process sleeping uninterruptibly
- * Z - zombie process
- * T - process is traced
+ * Possible process states (see :manpage:`ps(1)`):
+ *
+ * - **R** Process is running.
+ * - **S** Process is sleeping.
+ * - **D** Process sleeping uninterruptibly.
+ * - **Z** Zombie process.
+ * - **T** Process is traced.
+ * - **t** Tracing stopped.
+ * - **X** Process id dead.
  */
 #define TST_PROCESS_STATE_WAIT(pid, state, msec_timeout) \
 	tst_process_state_wait(__FILE__, __LINE__, NULL, \
 			(pid), (state), (msec_timeout))
 
-/*
- * Check that a given pid is present on the system
+/**
+ * TST_PROCESS_EXIT_WAIT() - Waits while pid is present on the system.
+ *
+ * Loops until `kill($PID, 0)` succeds or timeout is reached.
+ *
+ * @pid: A process pid.
+ * @msec_timeout: A timeout for the wait.
  */
 #define TST_PROCESS_EXIT_WAIT(pid, msec_timeout) \
 	tst_process_exit_wait((pid), (msec_timeout))
 
-/*
- * Waits for thread state change.
+/**
+ * TST_THREAD_STATE_WAIT() - Waits for a thread state change.
+ *
+ * Polls `/proc/self/task/$TID/state` for a thread state change.
  *
- * The state is one of the following:
+ * Possible thread states are the same as for TST_PROCESS_STATE_WAIT().
  *
- * R - running
- * S - sleeping
- * D - disk sleep
- * T - stopped
- * t - tracing stopped
- * Z - zombie
- * X - dead
+ * @tid: A thread tid.
+ * @state: A state to wait for.
+ * @msec_timeout: A timeout for the wait.
  */
 #define TST_THREAD_STATE_WAIT(tid, state, msec_timeout) \
 	tst_thread_state_wait((tid), (state), (msec_timeout))
-- 
2.52.0


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

[LTP] [PATCH 2/2] doc: Add ground rules page

[Cyril Hrubis]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=u, Size: 8954 bytes --]

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.

Reviewed-by: Li Wang <liwang@redhat.com>
Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 doc/developers/ground_rules.rst | 176 ++++++++++++++++++++++++++++++++
 doc/index.rst                   |   1 +
 2 files changed, 177 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..9bb4ec66c
--- /dev/null
+++ b/doc/developers/ground_rules.rst
@@ -0,0 +1,176 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Ground Rules
+============
+
+Do not work around kernel bugs
+------------------------------
+
+We have decided that 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 will likely introduce very rare test failures,
+that means somebody has to spend time looking into these, which is a wasted
+effort. Nobody likes tests that will fail rarely for no good reason. Even more
+so you cannot run such tests with a background load to ensure that everything
+works correctly on a busy system, because that will increase the likehood of a
+failure.
+
+The second problem is that this wastes resources and slows down a test run. If
+you think that adding a sleep to a test is not a big deal, lets have a look at
+the bigger perspective. There are about 1600 syscall tests in Linux Test
+Project, 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 tests, that
+historically misused sleep for synchronization, waited for much longer just to
+be sure that things will works even on slower 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 an outdated 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 test needs to
+synchronize between child and parent process.
+
+The easiest case is when parent needs to wait for a child to finish, that can
+be fixed just be adding a :manpage:`waitpid(2)` in the parent which ensures that child
+has 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
+:c:func:`TST_CHECKPOINT_WAIT()` and :c:func:`TST_CHECKPOINT_WAKE()` functions based
+on futexes on a piece of shared memory set up by the test library.
+
+Another common case is when a child must sleep in a syscall before parent can
+continue, for which we have a :c:func:`TST_PROCESS_STATE_WAIT()` helper that
+polls `/proc/$PID/stat`.
+
+Less often test needs to wait for an action that is done asynchronously, or for
+a kernel resource deallocation that is deferred to a later time. In such cases
+the best we can do is to poll. In LTP we ended up with a macro that polls by
+calling a piece of code in a loop with exponentially increasing sleeps between
+retries until it succeeds. Which means that instead of sleeping for a maximal
+time event can possibly take the sleep is capped by twice of the optimal
+sleeping time while we avoid polling too aggressively.
+
+
+Use runtime checks for kernel features
+--------------------------------------
+
+What is and what isn't supported by kernel is determined by the version and
+configuration of the kernel the system is currently running on.  That
+especially means that any checks done during the compilation cannot be used to
+assume features supported by the kernel the tests end up running on. The
+compile time checks, done by :master:`configure.ac` script, are only useful for
+enabling fallback kernel API definitions when missing, as we do in
+:master:`include/lapi/` directory.
+
+
+Don’t require root unless it’s essential
+----------------------------------------
+
+If root/caps are needed, say why in the test doc comment. Drop privileges for
+the part that doesn’t need them and avoid running the whole test as root
+“because it’s easier”.
+
+
+Always clean up, even on failure
+--------------------------------
+
+Every test should leave the system as it found it: unmount, restore sysctls,
+delete temp files/dirs, kill spawned processes, remove cgroups/namespaces,
+detach loop devices, restore ulimits, etc. Cleanup must run on early-exit
+paths too.
+
+The test library can simplify cleanup greatly as there are various helpers such as:
+
+- :c:type:`tst_test.needs_tmpdir <tst_test>` that creates and deletes a temporary directory for the test
+- :c:type:`tst_test.save_restore <tst_test>` that saves and restores /sys/ and /proc/ files
+- :c:type:`tst_test.needs_device <tst_test>` sets up and tears down a block device for the test
+- :c:type:`tst_test.restore_wallclock <tst_test>` that restores wall clock after the test
+- :c:type:`tst_test.needs_cgroup_ctrls <tst_test>` sets up and cleans up cgroups for the test
+- ...
+
+
+Write portable code
+-------------------
+
+Avoid nonstandard libc APIs when a portable equivalent exists; don’t assume
+64-bit, page size, endianness, or particular tool versions.
+
+If the test is specific to a certain architecture, make sure that it at least
+compiles at the rest of architectures and set the
+:c:type:`tst_test.supported_archs <tst_test>`.
+
+This also applies to shell code where it's easy to use bash features that are
+not available on other shell implementations, e.g. dash or busybox. Make sure
+to stick to portable POSIX shell whenever possible.
+
+You can check for common mistakes, not only in portability, with our
+'make check' tooling.
+
+
+Split changed into well defined chunks
+--------------------------------------
+
+When submitting patches make sure to split the work into small well-defined
+chunks. Patches that touch many files or mix unrelated changes and features are
+difficuilt to review and are likely to be detalyed or even ignored.
+
+Aim for a single logical change per patch. Split more complex works into a
+patch series where each patch:
+
+  - builds/compiles successfully.
+  - keeps tests and tooling functional.
+  - does not introduce intermediate breakage.
+  - has a clear commit message to explain the change.
+  - Significant changes need to be detailed in the cover letter.
+
+
+Be careful when using AI tools
+------------------------------
+
+AI tools can be useful for executing, summarizing, or suggesting approaches,
+but they can also be confidently wrong and give an illusion of correctness.
+Treat AI output as untrusted: verify claims against the code, documentation,
+and actual behavior on a reproducer.
+
+Do not send AI-generated changes as raw patches. AI-generated diffs often
+contain irrelevant churn, incorrect assumptions, inconsistent style, or subtle
+bugs, which creates additional burden for maintainers to review and fix.
+
+Best practice is to write your own patches and have them reviewed by AI before
+submitting them, which helps add beneficial improvements to your work.
+
+
+Kernel features and RCs
+-----------------------
+
+LTP tests or fixes for kernel changes that have not yet been released may be
+posted to the LTP list for a review but they will not be be accepted until
+respective kernel changes are released. Review of such changes is also
+considered to be lower priority than rest of the changes. This is because
+kernel changes especially in the early RC phase are volatile and could be
+changed or reverted.
+
+These patchses should also add a [STAGING] keyword into the patch subject, e.g.
+"Subject: [PATCH v1][STAGING] fanotify: add test for <feature> (requires v6.19-rc3)"
+
+In a case that a test for unrelased kernel is really needed to be merged we do
+not add it to the list of test executed by default and keep it in
+:master:`runtest/staging` file until the kernel code is finalized.
diff --git a/doc/index.rst b/doc/index.rst
index f78e7a448..496a12f80 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
-- 
2.52.0


[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

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

[Petr Vorel]

Hi all,

4 typos below. With that fixed before merge:
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Few small minor things below.
...
> +Always clean up, even on failure
> +--------------------------------
> +
> +Every test should leave the system as it found it: unmount, restore sysctls,
> +delete temp files/dirs, kill spawned processes, remove cgroups/namespaces,
> +detach loop devices, restore ulimits, etc. Cleanup must run on early-exit
> +paths too.

There is an exception on cleanup dynamic memory allocation before tst_brk(),
right? I keep forgetting this, but it's 1) it's a C memory cleanup (not a system
cleanup) 2) a corner case (probably not needed to mention).

> +
> +The test library can simplify cleanup greatly as there are various helpers such as:
> +
> +- :c:type:`tst_test.needs_tmpdir <tst_test>` that creates and deletes a temporary directory for the test

How about use syntax which people actually use in the code?
- :c:type:`.needs_tmpdir = 1 <tst_test>` that creates and deletes a temporary directory for the test

> +- :c:type:`tst_test.save_restore <tst_test>` that saves and restores /sys/ and /proc/ files
> +- :c:type:`tst_test.needs_device <tst_test>` sets up and tears down a block device for the test
> +- :c:type:`tst_test.restore_wallclock <tst_test>` that restores wall clock after the test
> +- :c:type:`tst_test.needs_cgroup_ctrls <tst_test>` sets up and cleans up cgroups for the test
> +- ...
Maybe instead "..." use: "And many more"?

Also (OT( I'm getting convinced that even we transform all headers into
kerneldoc comments and use examples from the old doc [1] we will probably need
some highlevel document similar to the old doc (something shorter than the old
doc, with links pointing to the header docs, pointing out the most important
things).

[1] https://github.com/linux-test-project/ltp/blob/master/doc/old/C-Test-API.asciidoc

> +
> +
> +Write portable code
> +-------------------
> +
> +Avoid nonstandard libc APIs when a portable equivalent exists; don’t assume
> +64-bit, page size, endianness, or particular tool versions.
> +
> +If the test is specific to a certain architecture, make sure that it at least
> +compiles at the rest of architectures and set the
> +:c:type:`tst_test.supported_archs <tst_test>`.
> +
> +This also applies to shell code where it's easy to use bash features that are
> +not available on other shell implementations, e.g. dash or busybox. Make sure
> +to stick to portable POSIX shell whenever possible.
> +
> +You can check for common mistakes, not only in portability, with our
> +'make check' tooling.
nit: ``make check`` tooling.
> +
> +
> +Split changed into well defined chunks
> +--------------------------------------
> +
> +When submitting patches make sure to split the work into small well-defined
> +chunks. Patches that touch many files or mix unrelated changes and features are
> +difficuilt to review and are likely to be detalyed or even ignored.
typo: difficuilt => difficult, detalyed => delayed (?)

> +
> +Aim for a single logical change per patch. Split more complex works into a
> +patch series where each patch:
> +
> +  - builds/compiles successfully.
> +  - keeps tests and tooling functional.
> +  - does not introduce intermediate breakage.
> +  - has a clear commit message to explain the change.
> +  - Significant changes need to be detailed in the cover letter.
> +
> +
> +Be careful when using AI tools
> +------------------------------
> +
> +AI tools can be useful for executing, summarizing, or suggesting approaches,
> +but they can also be confidently wrong and give an illusion of correctness.
> +Treat AI output as untrusted: verify claims against the code, documentation,
> +and actual behavior on a reproducer.
> +
> +Do not send AI-generated changes as raw patches. AI-generated diffs often
> +contain irrelevant churn, incorrect assumptions, inconsistent style, or subtle
> +bugs, which creates additional burden for maintainers to review and fix.
> +
> +Best practice is to write your own patches and have them reviewed by AI before
> +submitting them, which helps add beneficial improvements to your work.
> +
> +
> +Kernel features and RCs
> +-----------------------
> +
> +LTP tests or fixes for kernel changes that have not yet been released may be
> +posted to the LTP list for a review but they will not be be accepted until
> +respective kernel changes are released. Review of such changes is also
> +considered to be lower priority than rest of the changes. This is because
> +kernel changes especially in the early RC phase are volatile and could be
> +changed or reverted.
> +
> +These patchses should also add a [STAGING] keyword into the patch subject, e.g.
typo: patchses => patches

> +"Subject: [PATCH v1][STAGING] fanotify: add test for <feature> (requires v6.19-rc3)"
> +
> +In a case that a test for unrelased kernel is really needed to be merged we do
typo: unrelased => unreleased

Kind regards,
Petr

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

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

[Li Wang via ltp]

> +Don’t require root unless it’s essential
> +----------------------------------------
> +
> +If root/caps are needed, say why in the test doc comment. Drop privileges for
> +the part that doesn’t need them and avoid running the whole test as root
> +“because it’s easier�€?.

This paragraph contains some non-ASCII characters, we need switch to
plain ASCII.
They were inserted by my keyboard originally. Sorry about that.


-Don’t require root unless it’s essential
+Don't require root unless it's essential
 ----------------------------------------

 If root/caps are needed, say why in the test doc comment. Drop privileges for
-the part that doesn’t need them and avoid running the whole test as root
-“because it’s easier”.
+the part that doesn't need them and avoid running the whole test as root
+"because it's easier".


-- 
Regards,
Li Wang


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

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

[Li Wang via ltp]

> +Write portable code
> +-------------------
> +
> +Avoid nonstandard libc APIs when a portable equivalent exists; don’t assume
> +64-bit, page size, endianness, or particular tool versions.

and here the "don't " ^ is also non-ASCII.


-- 
Regards,
Li Wang


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

Re: [LTP] [PATCH 1/2] doc: Document process_state

[Li Wang via ltp]

Reviewed-by: Li Wang <liwang@redhat.com>

-- 
Regards,
Li Wang


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

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

[Cyril Hrubis]

Hi!
Thanks for the reviews, I fixed the problems pointed out by you and Li
and pushed.

-- 
Cyril Hrubis
chrubis@suse.cz

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