Re: [PATCH 3/3] selftests/livepatch: Test of the API for specifying functions to search for on a stack

On Fri 2021-11-26 10:20:54, Miroslav Benes wrote:

On Thu, 25 Nov 2021, Petr Mladek wrote:

quoted

On Fri 2021-11-19 10:03:27, Miroslav Benes wrote:

quoted

Add a test for the API which allows the user to specify functions which
are then searched for on any tasks's stack during a transition process.

The approach with debugfs is an interesting trick. Though, I slightly
prefer using the scheduled work. The workqueue API looks less tricky
to me than sysfs/debugfs API. Also it does not block the module
in the init() callback[*]. But I might be biased.

It seemed to me that debugfs gave us more control over the process than 
workqueues, but I do not really care. Could you explain the blocking in 
the init callback? I do not follow.

Good question about the blocking! Please, forget it. I am not sure
why I thought that the module might get blocked in the module_init()
script.

quoted

Anyway, it might make sense to use the same trick in both situations.
It would make it easier to maintain the test modules.

True. So I will rewrite it to workqueues as you are proposing below.

quoted

[*] There is actually a race in the workqueue approach. The module
init() callback should wait until the work is really scheduled
and sleeping. It might be achieved by similar hand-shake like
with @block_transition variable. Or completion API might be
even more elegant.

quoted

+	pr_info("%s exit\n", __func__);
+}
+
+static noinline void child2_function(void)
+{
+	pr_info("%s\n", __func__);
+}
+
+static noinline void parent_function(void)
+{
+	pr_info("%s enter\n", __func__);
+	child_function();
+	child2_function();

This would deserve some explanation what we try to simulate here
and how it is achieved. It is not easy for me even with the background
that I have freshly in my mind.

Also I think about more descriptive names ;-)

Hey, I thought it was self-explaining :). So, yes, I started with the 
example given in the .fixup thread, but it is not really tied to .cold 
section, jumps or whatever. The setup is just used to test a new API. 
Moreover, the .fixup example is just a one scenario the new API tries to 
solve.

OK, single child() function should be enough for testing the behavior.
We might sleep/wait in the parent().

I think that I was confused by the two child() functions. It looked
like sleeping in a child function was important. And the "same"
name of the two children did not help much to understand and
distinguish the purpose.

What you propose below, that is function names and comments, is a bit 
confusing for me. Especially if I did not know anything about the original 
issue (which will be the case in a couple of weeks when I forget 
everything).

So I think it I will stick to brevity unless you or someone else really 
insist.

No, I do not resist on the complicated exmaple. When thinking about
it, the easier test case the better. It should be enough to describe
the real-life purpose of the API in the patch that introduces the API.

I can improve tests description in 
tools/testing/selftests/livepatch/test-func-stack.sh if it helps anything.

Yes, please. I miss some top-level descriptions of the tested
functionality, something like:

# Tests for "bla bla" feature.
# It allows to block transition of a process when it is running
# parent() function and only the child() function is livepatched.

# This test does not use the feature. The transition finishes even
# before parent() exits.

# In this test case, the livepatch is instructed to check also
# parent() on stack. The transition must not finish before
# parent() exists.

It would be nice to have these high-level descriptions even in
the test modules.

Best Regards,
Petr