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

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

[Cyril Hrubis]

This is v3 of the patchset the changes are:

- formatting changes (mostly links) from Petr
- grammar fixes from Petr
- added a few more paragraphs by Li

I did various changes and adjustenments to the suggestions. Please make
sure to check the text before acking it.

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

 doc/conf.py                     |   1 +
 doc/developers/api_c_tests.rst  |   5 +
 doc/developers/ground_rules.rst | 167 ++++++++++++++++++++++++++++++++
 doc/index.rst                   |   1 +
 include/tst_process_state.h     |  47 +++++----
 5 files changed, 201 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 v3 1/2] doc: Document process_state

[Cyril Hrubis]

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

diff --git a/doc/conf.py b/doc/conf.py
index b8ec1c31c..5d7362439 100644
--- a/doc/conf.py
+++ b/doc/conf.py
@@ -36,6 +36,7 @@ extlinks = {
     'master': (f'{ltp_repo}/blob/master/%s', '%s'),
     'shell_lib': (f'{ltp_repo}/blob/master/testcases/lib/%s', '%s'),
     'git_man': ('https://git-scm.com/docs/git-%s', 'git %s'),
+    'man1': ('https://man7.org/linux/man-pages/man1/%s.1.html', '%s(1)'),
     'man2': ('https://man7.org/linux/man-pages/man2/%s.2.html', '%s(2)'),
     'man3': ('https://man7.org/linux/man-pages/man3/%s.3.html', '%s(3)'),
     # TODO: allow 2nd parameter to show page description instead of plain URL
diff --git a/doc/developers/api_c_tests.rst b/doc/developers/api_c_tests.rst
index 2ca0f0464..13fc8651b 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..fd94ee207 100644
--- a/include/tst_process_state.h
+++ b/include/tst_process_state.h
@@ -15,39 +15,46 @@
 
 #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 :man1:`ps`):
+ *
+ * - **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.
  *
- * The state is one of the following:
+ * Polls `/proc/self/task/$TID/state` for a thread state change.
  *
- * R - running
- * S - sleeping
- * D - disk sleep
- * T - stopped
- * t - tracing stopped
- * Z - zombie
- * X - dead
+ * Possible thread states are the same as for TST_PROCESS_STATE_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 v3 2/2] doc: Add ground rules page

[Cyril Hrubis]

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.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 doc/developers/ground_rules.rst | 167 ++++++++++++++++++++++++++++++++
 doc/index.rst                   |   1 +
 2 files changed, 168 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..f6e4e70c0
--- /dev/null
+++ b/doc/developers/ground_rules.rst
@@ -0,0 +1,167 @@
+.. 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 :man2:`waitpid` 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 sending patches to the project make sure to split the work into small well
+defined chunks. Patch that changes too many files and does too many different
+things is not going to get reviewed soon or possibly may not get any reviews at
+all. Ideally patch should do a single logical change. However at the same time
+make sure not to break the code in the middle of patchset. Each patch in a
+sequence has to compile and function properly after it has been applied.
+
+
+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.
+
+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 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
-- 
2.52.0


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

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

[Petr Vorel]

Hi Cyril, Andrea,

more fixes needed:

> +++ b/include/tst_process_state.h
...
>   */
>  #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.
          ^ missing ':' here leads to "pid – undescribed" in generated doc,
see ./doc/html/developers/api_c_tests.html#macro-tst-process-exit-wait

@Andrea: I wish sphinx doc would warn about "undescribed" and I thought it did
in the past, but I don't see it.

Also we have too many (~ 220x) warnings:
"WARNING: duplicate label description, other instance in ..." it's not usable.
Any hint you could have look on it? If we solve this warning would get useful.

> + * @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.
>   *
> - * The state is one of the following:
> + * Polls `/proc/self/task/$TID/state` for a thread state change.
>   *
> - * R - running
> - * S - sleeping
> - * D - disk sleep
> - * T - stopped
> - * t - tracing stopped
> - * Z - zombie
> - * X - dead
> + * Possible thread states are the same as for TST_PROCESS_STATE_WAIT().

And here we have "undescribed" due not being described:

Parameters:
tid – undescribed
state – undescribed
msec_timeout – undescribed

See ./doc/html/developers/api_c_tests.html#macro-tst-thread-state-wait

Kind regards,
Petr

>   */
>  #define TST_THREAD_STATE_WAIT(tid, state, msec_timeout) \
>  	tst_thread_state_wait((tid), (state), (msec_timeout))

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

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

[Andrea Cervesato via ltp]

Hi!

> @Andrea: I wish sphinx doc would warn about "undescribed" and I thought it did
> in the past, but I don't see it.

That's not sphinx, but the 'linuxdoc.rstKernelDoc' plugin we are using.
It's developed for the kernel documentation (perhaps, we might upgrade
it to the newest version) and it doesn't provide this.

>
> Also we have too many (~ 220x) warnings:
> "WARNING: duplicate label description, other instance in ..." it's not usable.
> Any hint you could have look on it? If we solve this warning would get useful.
>

The warnings you see are generated by:

	pip install linuxdoc
	linuxdoc.lintdoc include/

-- 
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com


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

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

[Li Wang via ltp]

Cyril Hrubis <chrubis@suse.cz> wrote:

> 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.
>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>

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


> +Split changed into well defined chunks
> +======================================
> +
> +When sending patches to the project make sure to split the work into small well
> +defined chunks. Patch that changes too many files and does too many different
> +things is not going to get reviewed soon or possibly may not get any reviews at
> +all. Ideally patch should do a single logical change. However at the same time
> +make sure not to break the code in the middle of patchset. Each patch in a
> +sequence has to compile and function properly after it has been applied.

How about splitting it into sub-items for better readability:

"When submitting patches, split your work into small, well-defined changes.
Patches that touch many files or mix unrelated refactors, behavior changes,
and new features are difficult to review and are likely to be delayed
or ignored.

Aim for one logical change per patch. If the work naturally spans
multiple steps,
send it as 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."


> +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.

I'd add one more requirement for this rule:

"Such patches should explicitly include keywords in the theme to indicate that
the feature is in the staging phase.
Examples:
    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.



--
Regards,
Li Wang


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

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

[Petr Vorel]

> Hi!

> > @Andrea: I wish sphinx doc would warn about "undescribed" and I thought it did
> > in the past, but I don't see it.

> That's not sphinx, but the 'linuxdoc.rstKernelDoc' plugin we are using.
> It's developed for the kernel documentation (perhaps, we might upgrade
> it to the newest version) and it doesn't provide this.

I tried to update linuxdoc in the past. I remember warning were always there.

Update to 20240924 has only:
* a87ed0a ("[fix] export option to kernel-doc directive")
* 942fecd ("[fix]] kernel_include.py: cope with docutils 0.21")

And warnings are still there. But I might send a patch, better to stay on the
latest release (there are few more fixes, but unreleased).

> > Also we have too many (~ 220x) warnings:
> > "WARNING: duplicate label description, other instance in ..." it's not usable.
> > Any hint you could have look on it? If we solve this warning would get useful.


> The warnings you see are generated by:

> 	pip install linuxdoc
> 	linuxdoc.lintdoc include/

Thanks for a hint.

Kind regards,
Petr

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

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

[Petr Vorel]

Hi Cyril,

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

> diff --git a/doc/conf.py b/doc/conf.py
> index b8ec1c31c..5d7362439 100644
> --- a/doc/conf.py
> +++ b/doc/conf.py
> @@ -36,6 +36,7 @@ extlinks = {
>      'master': (f'{ltp_repo}/blob/master/%s', '%s'),
>      'shell_lib': (f'{ltp_repo}/blob/master/testcases/lib/%s', '%s'),
>      'git_man': ('https://git-scm.com/docs/git-%s', 'git %s'),
> +    'man1': ('https://man7.org/linux/man-pages/man1/%s.1.html', '%s(1)'),

I'm sorry, due today's improvement
https://github.com/linux-test-project/ltp/commit/de27451e837a6d17c8ebf1a55b5ade87223882f4
could you please drop doc/conf.py change because :man1: is not needed any more.

>      'man2': ('https://man7.org/linux/man-pages/man2/%s.2.html', '%s(2)'),
>      'man3': ('https://man7.org/linux/man-pages/man3/%s.3.html', '%s(3)'),
>      # TODO: allow 2nd parameter to show page description instead of plain URL
> diff --git a/doc/developers/api_c_tests.rst b/doc/developers/api_c_tests.rst
> index 2ca0f0464..13fc8651b 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..fd94ee207 100644
> --- a/include/tst_process_state.h
> +++ b/include/tst_process_state.h
> @@ -15,39 +15,46 @@

>  #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 :man1:`ps`):

And this should be now :manpage:`ps(1)`.

Thank you!

Kind regards,
Petr

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