Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test/integration/rhel-9.0: add kernel-5.14.0-70.13.1.el9_0 tests #1265

Merged
merged 2 commits into from
May 10, 2022

Conversation

joe-lawrence
Copy link
Contributor

gcc-constprop.patch:
In v4.20, 33e26418193f ("y2038: make do_gettimeofday() and get_seconds()
inline"), do_gettimeofdat() no longer exists as a stand alone function
in kernel/time/timekeeping.c.

gcc-static-local-var-4.patch:
Unlike on rhel-8.4, _always_inline put_aio_ring_file() is causing too
many inlines and results in modified, but not ftrace-able,
__do_sys_io_submit() and __do_sys_io_setup(). Remove the annotation
from this function.

module.patch:
In v4.20, 9ceddd9da134 ("knfsd: Allow lockless lookups of the exports"),
the nfs_exports_op seq_operations converted to RCU protected lookups.
Calling yield() from a kpatched e_show() results in a kernel warning,
"Voluntary context switch within RCU read-side critical section!"
Substitute with single_task_running(), which does not context switch or
have any other side effects.

In v5.10, ec6347bb4339 ("x86, powerpc: Rename memcpy_mcsafe() to
copy_mc_to_{user, kernel}()") did away with the mcsafe_key. Use
another exported static key like context_tracking_enabled.

In v5.13, a0e2bf7cb700 ("x86/paravirt: Switch time pvops functions to
use static_call()"), paravirt_sched_clock() was converted from a
paravirt call to a non-exported static call. Update the x86 code to
instead call __flush_tlb_local() (which is still a paravirt call).

Signed-off-by: Joe Lawrence joe.lawrence@redhat.com

* gcc-constprop.patch
In v4.20, 33e26418193f ("y2038: make do_gettimeofday() and get_seconds()
inline"), do_gettimeofdat() no longer exists as a stand alone function
in kernel/time/timekeeping.c.

* gcc-static-local-var-4.patch
Unlike on rhel-8.4, _always_inline put_aio_ring_file() is causing too
many inlines and results in modified, but not ftrace-able,
__do_sys_io_submit() and __do_sys_io_setup().  Remove the annotation
from this function.

* module.patch
In v4.20, 9ceddd9da134 ("knfsd: Allow lockless lookups of the exports"),
the nfs_exports_op seq_operations converted to RCU protected lookups.
Calling yield() from a kpatched e_show() results in a kernel warning,
"Voluntary context switch within RCU read-side critical section!"
Substitute with single_task_running(), which does not context switch or
have any other side effects.

In v5.10, ec6347bb4339 ("x86, powerpc: Rename memcpy_mcsafe() to
copy_mc_to_{user, kernel}()") did away with the mcsafe_key.  Use
another exported static key like context_tracking_enabled.

In v5.13, a0e2bf7cb700 ("x86/paravirt: Switch time pvops functions to
use static_call()"), paravirt_sched_clock() was converted from a
paravirt call to a non-exported static call.  Update the x86 code to
instead call __flush_tlb_local() (which is still a paravirt call).

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
Copy link
Contributor

@yhcote yhcote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@jpoimboe
Copy link
Member

jpoimboe commented May 2, 2022

Many of these probably don't test what they originally intended to test for old kernel versions. That was why I converted many of them to unit tests and removed many of these from the latest (now linux-5.10.11) integration tests.

That said, I guess it doesn't hurt anything to still keep porting and testing the old ones in the hope they'll find new issues.

On the other hand you might want to just drop the old ones and instead base rhel-9.0 tests on the linux-5.10.11 tests. Then going forward we could try to do a better job of documenting exactly what each patch is supposed to test.

But I could go either way.

@joe-lawrence
Copy link
Contributor Author

@jpoimboe : Good point, though there is added value in that the integration tests verify that the kernel plays nice with the generated livepatch. At the moment, the unit testing scripts (mostly) only verify the presence of N modified functions.

Any chance you still have any notes on which ones were obviously not testing what they were originally written for?

@jpoimboe
Copy link
Member

jpoimboe commented May 3, 2022

@jpoimboe : Good point, though there is added value in that the integration tests verify that the kernel plays nice with the generated livepatch. At the moment, the unit testing scripts (mostly) only verify the presence of N modified functions.

True, though the status quo of having more tests which may or may not serve their original purpose will eventually devolve into none of them testing what they were originally intended to, which means a higher likelihood of regressions.

If we do want to keep porting the extra tests, it might still be worth designating in the patch headers which ones we don't care about vs which ones we do, and how to tell if the latter ones are still testing what they're supposed to.

Any chance you still have any notes on which ones were obviously not testing what they were originally written for?

I believe I removed them, and converted most of them to unit tests anyway. For example see my work in tests/integration/fedora-27 including af9f9f4 and e16b418.

Unfortunately I failed to update the remaining patch headers with guidance of how to know they're continuing to test what they're supposed to. But at least it's only 10 patches so it should hopefully not be TOO bad to do the git/github archeology. But that's easy for me to say since I don't really have the bandwidth to do that right now ;-)

@joe-lawrence
Copy link
Contributor Author

True, though the status quo of having more tests which may or may not serve their original purpose will eventually devolve into none of them testing what they were originally intended to, which means a higher likelihood of regressions.

Agreed. I only wanted to mention that loading the generated kpatches can be helpful, too. IIRC, #1228 was detected by loading module.patch.

But that said, now that we have the unit tests, maybe it's time to think about even bigger changes for the integration tests. One example could be to target an OOT module instead of (always moving) vmlinux. It wouldn't guarantee that the tests continue to server their original purpose, but it could cut down on constant rebase churn. Or perhaps we could change the tests in other ways to to maintain their relevance?

Also, cmdline.patch is very handy example patch. It's unfortunate that it changed between rhel-7/8 as it would be nice to always have available for sanity checking. We could throw the two versions in a new examples/ directory, or maybe come up with an equally approachable sample patch that isn't version sensitive.

@jpoimboe
Copy link
Member

jpoimboe commented May 5, 2022

The OOT idea is an interesting one. I don't know if it would help. There's value in having integration tests targeted against vmlinux, which are continually ported forward. Because things tend to break with new kernel features. I think the effort in continuing to forward-port patches indefinitely is worthwhile, as long as that list of patches is reasonably bounded and as long as we document the purpose of each patch and how we know it's still doing what it's supposed to.

Clean out any integration tests that no longer exercise their original
use cases.

Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
@joe-lawrence
Copy link
Contributor Author

(Patches reduced to weed out the deprecated ones. We can figure out more updates/refactoring in a later PR.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants