kernel timer API rework #8647
kernel timer API rework #8647
Conversation
|
Looks like I need to check for both |
Codecov Report
@@ Coverage Diff @@
## master #8647 +/- ##
==========================================
- Coverage 78.97% 78.69% -0.29%
==========================================
Files 381 382 +1
Lines 117797 117801 +4
==========================================
- Hits 93034 92703 -331
- Misses 24763 25098 +335
Continue to review full report at Codecov.
|
rkitover
changed the title
|
I think it's ready now, should be the normal CI results. Will wait for your feedback. |
include/spl/sys/timer.h
Outdated
| container_of(timer, typeof(*var), timer_field) | ||
| #endif | ||
|
|
||
|
|
behlendorf
added
Status: Code Review Needed
|
@behlendorf thank you for taking a look, I just made these changes and will see what the CI says. Also I was wondering, do we have documentation for spl? Should we? |
|
@behlendorf I wanted to ask you also, I have a lovely Project Trident install going, and that runs on CURRENT, where are the freebsd ZoL incorporation efforts happening and can I get involved in that. |
behlendorf
added
Status: Code Review Needed
|
@rkitover can you address the one cstyle issue reported, then this looks good to me. We have one other issue with the 5.0.8 kernel causing the Federa failures above. I should be able to look in to that next week, unless some beats me to it. So I'll include this change for that testing.
Only in the source code. The public interfaces are designed to match those from illumos in behavior so you can draw on some of that documentation. But internally, the implementations are different. I think improving the documentation would definitely be a good thing.
Great timing. There we a call for testing recently send out to the openzfs-developer list which should reference everything you need to get going. We'd love the feedback and help! https://openzfs.topicbox.com/groups/developer/T2b2cb7346ab24f1e/call-for-testing-of-freebsd-zol |
|
@behlendorf thank you for the info, I will try to spend some time with the freebsd port soon. I couldn't figure out before what that style error meant before, but I got it this time. |
@rkitover would you posting exactly what build errors this PR resolved on Alpine since we're not seeing a problem in the CI environments. @kusumi would you mind providing a second review for this as well. |
|
@behlendorf the errors were specific to 5.0-rc7 rather than alpine, I wouldn't have even bothered but I thought it was a different kernel, and hopefully this is a useful refactor anyway. I will post the exact errors in a bit. |
There was a problem hiding this comment.
What does as it is sometimes defined as a function. in
Also, to fix a problem in 5.0-rc7 which was the impetus for this work,
in config/kernel-pde-data.m4 check that the PDE_DATA() is available
in any form rather than checking for the symbol, as it is sometimes
defined as a function.
mean ? It looks to me it's always been a function.
config/kernel-timer.m4
Outdated
| dnl # Check if timer_list.func get passed a timer_list or an unsigned long | ||
| dnl # (older kernels). Also sanity check the from_timer() and timer_setup() | ||
| dnl # macros are available as well, since they will be used in the same newer | ||
| dnl # kernels that support the new timer_list.func signature. |
There was a problem hiding this comment.
It looks to me this shouldn't be removed as the timer_list.func compile time test still exists.
config/kernel-pde-data.m4
Outdated
| @@ -4,11 +4,11 @@ dnl # PDE is replaced by PDE_DATA | |||
| dnl # | |||
| AC_DEFUN([ZFS_AC_KERNEL_PDE_DATA], [ | |||
| AC_MSG_CHECKING([whether PDE_DATA() is available]) | |||
| ZFS_LINUX_TRY_COMPILE_SYMBOL([ | |||
| ZFS_LINUX_TRY_COMPILE([ | |||
There was a problem hiding this comment.
It looks like PDE_DATA() first appeared in 3.10 and has always been an exported function, i.e. EXPORT_SYMBOL(PDE_DATA).
To be more specific, it was first added as an inline function in upstream commit
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d9dda78bad879595d8c4220a067fc029d6484a16
and then changed to non-inline exported function in upstream commit
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c30480b92cf497aa3b463367a82f1c2fdc5c46e9
both of which first appeared in 3.10 in terms of release kernel, so PDE_DATA() has always been a exported function.
| ],[ | ||
| AC_MSG_RESULT(no) | ||
| ]) | ||
|
|
There was a problem hiding this comment.
This change is from a commit in 4.2 kernel.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0eeda71bc30d74f66f8231f45621d5ace3419186
|
This is very strange, with 5.0-rc7 on gentoo master compiles correctly for me. Now I need to figure out what it is that's specific to the alpine environment or the particular commit I was testing at the time to make it fail. I thought these errors were specific to 5.0-rc7 but apparently not. |
|
I cannot reproduce this on alpine either, unfortunately I deleted that kernel tree and am using the git tags now. I have no idea what happened. In this case, I will remove the |
behlendorf
added
Type: Building
In `config/kernel-timer.m4` refactor slightly to check more generally
for the new `timer_setup()` APIs, but also check the callback signature
because some kernels (notably 4.14) have the new `timer_setup()` API but
use the old callback signature. Also add a check for a `flags` member in
`struct timer_list`, which was added in 4.1-rc8.
Add compatibility shims to `include/spl/sys/timer.h` to allow using the
new timer APIs with the only two caveats being that the callback
argument type must be declared as `spl_timer_list_t` and an explicit
assignment is required to get the timer variable for the `timer_of()`
macro. So the callback would look like this:
```c
__cv_wakeup(spl_timer_list_t t)
{
struct timer_list *tmr = (struct timer_list *)t;
struct thing *parent = from_timer(parent, tmr,
parent_timer_field);
... /* do stuff with parent */
```
Make some minor changes to `spl-condvar.c` and `spl-taskq.c` to use the
new timer APIs instead of conditional code.
Signed-off-by: Rafael Kitover <rkitover@gmail.com>
|
@kusumi apologies for the delay in getting back to this, does this look better? |
|
@behlendorf Hi, can this be merged now? Also any other build related issues you'd like me to look at? |
behlendorf
added
Status: Accepted
Since you're offering it would be great if you could review the outstanding PRs for Linux 5.2 compatibility. |
In `config/kernel-timer.m4` refactor slightly to check more generally
for the new `timer_setup()` APIs, but also check the callback signature
because some kernels (notably 4.14) have the new `timer_setup()` API but
use the old callback signature. Also add a check for a `flags` member in
`struct timer_list`, which was added in 4.1-rc8.
Add compatibility shims to `include/spl/sys/timer.h` to allow using the
new timer APIs with the only two caveats being that the callback
argument type must be declared as `spl_timer_list_t` and an explicit
assignment is required to get the timer variable for the `timer_of()`
macro. So the callback would look like this:
```c
__cv_wakeup(spl_timer_list_t t)
{
struct timer_list *tmr = (struct timer_list *)t;
struct thing *parent = from_timer(parent, tmr,
parent_timer_field);
... /* do stuff with parent */
```
Make some minor changes to `spl-condvar.c` and `spl-taskq.c` to use the
new timer APIs instead of conditional code.
Reviewed-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rafael Kitover <rkitover@gmail.com>
Closes openzfs#8647
In `config/kernel-timer.m4` refactor slightly to check more generally
for the new `timer_setup()` APIs, but also check the callback signature
because some kernels (notably 4.14) have the new `timer_setup()` API but
use the old callback signature. Also add a check for a `flags` member in
`struct timer_list`, which was added in 4.1-rc8.
Add compatibility shims to `include/spl/sys/timer.h` to allow using the
new timer APIs with the only two caveats being that the callback
argument type must be declared as `spl_timer_list_t` and an explicit
assignment is required to get the timer variable for the `timer_of()`
macro. So the callback would look like this:
```c
__cv_wakeup(spl_timer_list_t t)
{
struct timer_list *tmr = (struct timer_list *)t;
struct thing *parent = from_timer(parent, tmr,
parent_timer_field);
... /* do stuff with parent */
```
Make some minor changes to `spl-condvar.c` and `spl-taskq.c` to use the
new timer APIs instead of conditional code.
Reviewed-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rafael Kitover <rkitover@gmail.com>
Closes openzfs#8647
In `config/kernel-timer.m4` refactor slightly to check more generally
for the new `timer_setup()` APIs, but also check the callback signature
because some kernels (notably 4.14) have the new `timer_setup()` API but
use the old callback signature. Also add a check for a `flags` member in
`struct timer_list`, which was added in 4.1-rc8.
Add compatibility shims to `include/spl/sys/timer.h` to allow using the
new timer APIs with the only two caveats being that the callback
argument type must be declared as `spl_timer_list_t` and an explicit
assignment is required to get the timer variable for the `timer_of()`
macro. So the callback would look like this:
```c
__cv_wakeup(spl_timer_list_t t)
{
struct timer_list *tmr = (struct timer_list *)t;
struct thing *parent = from_timer(parent, tmr,
parent_timer_field);
... /* do stuff with parent */
```
Make some minor changes to `spl-condvar.c` and `spl-taskq.c` to use the
new timer APIs instead of conditional code.
Reviewed-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rafael Kitover <rkitover@gmail.com>
Closes openzfs#8647
In `config/kernel-timer.m4` refactor slightly to check more generally
for the new `timer_setup()` APIs, but also check the callback signature
because some kernels (notably 4.14) have the new `timer_setup()` API but
use the old callback signature. Also add a check for a `flags` member in
`struct timer_list`, which was added in 4.1-rc8.
Add compatibility shims to `include/spl/sys/timer.h` to allow using the
new timer APIs with the only two caveats being that the callback
argument type must be declared as `spl_timer_list_t` and an explicit
assignment is required to get the timer variable for the `timer_of()`
macro. So the callback would look like this:
```c
__cv_wakeup(spl_timer_list_t t)
{
struct timer_list *tmr = (struct timer_list *)t;
struct thing *parent = from_timer(parent, tmr,
parent_timer_field);
... /* do stuff with parent */
```
Make some minor changes to `spl-condvar.c` and `spl-taskq.c` to use the
new timer APIs instead of conditional code.
Reviewed-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rafael Kitover <rkitover@gmail.com>
Closes openzfs#8647
In `config/kernel-timer.m4` refactor slightly to check more generally
for the new `timer_setup()` APIs, but also check the callback signature
because some kernels (notably 4.14) have the new `timer_setup()` API but
use the old callback signature. Also add a check for a `flags` member in
`struct timer_list`, which was added in 4.1-rc8.
Add compatibility shims to `include/spl/sys/timer.h` to allow using the
new timer APIs with the only two caveats being that the callback
argument type must be declared as `spl_timer_list_t` and an explicit
assignment is required to get the timer variable for the `timer_of()`
macro. So the callback would look like this:
```c
__cv_wakeup(spl_timer_list_t t)
{
struct timer_list *tmr = (struct timer_list *)t;
struct thing *parent = from_timer(parent, tmr,
parent_timer_field);
... /* do stuff with parent */
```
Make some minor changes to `spl-condvar.c` and `spl-taskq.c` to use the
new timer APIs instead of conditional code.
Reviewed-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rafael Kitover <rkitover@gmail.com>
Closes openzfs#8647
In `config/kernel-timer.m4` refactor slightly to check more generally
for the new `timer_setup()` APIs, but also check the callback signature
because some kernels (notably 4.14) have the new `timer_setup()` API but
use the old callback signature. Also add a check for a `flags` member in
`struct timer_list`, which was added in 4.1-rc8.
Add compatibility shims to `include/spl/sys/timer.h` to allow using the
new timer APIs with the only two caveats being that the callback
argument type must be declared as `spl_timer_list_t` and an explicit
assignment is required to get the timer variable for the `timer_of()`
macro. So the callback would look like this:
```c
__cv_wakeup(spl_timer_list_t t)
{
struct timer_list *tmr = (struct timer_list *)t;
struct thing *parent = from_timer(parent, tmr,
parent_timer_field);
... /* do stuff with parent */
```
Make some minor changes to `spl-condvar.c` and `spl-taskq.c` to use the
new timer APIs instead of conditional code.
Reviewed-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rafael Kitover <rkitover@gmail.com>
Closes openzfs#8647
In `config/kernel-timer.m4` refactor slightly to check more generally
for the new `timer_setup()` APIs, but also check the callback signature
because some kernels (notably 4.14) have the new `timer_setup()` API but
use the old callback signature. Also add a check for a `flags` member in
`struct timer_list`, which was added in 4.1-rc8.
Add compatibility shims to `include/spl/sys/timer.h` to allow using the
new timer APIs with the only two caveats being that the callback
argument type must be declared as `spl_timer_list_t` and an explicit
assignment is required to get the timer variable for the `timer_of()`
macro. So the callback would look like this:
```c
__cv_wakeup(spl_timer_list_t t)
{
struct timer_list *tmr = (struct timer_list *)t;
struct thing *parent = from_timer(parent, tmr,
parent_timer_field);
... /* do stuff with parent */
```
Make some minor changes to `spl-condvar.c` and `spl-taskq.c` to use the
new timer APIs instead of conditional code.
Reviewed-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rafael Kitover <rkitover@gmail.com>
Closes openzfs#8647
In `config/kernel-timer.m4` refactor slightly to check more generally
for the new `timer_setup()` APIs, but also check the callback signature
because some kernels (notably 4.14) have the new `timer_setup()` API but
use the old callback signature. Also add a check for a `flags` member in
`struct timer_list`, which was added in 4.1-rc8.
Add compatibility shims to `include/spl/sys/timer.h` to allow using the
new timer APIs with the only two caveats being that the callback
argument type must be declared as `spl_timer_list_t` and an explicit
assignment is required to get the timer variable for the `timer_of()`
macro. So the callback would look like this:
```c
__cv_wakeup(spl_timer_list_t t)
{
struct timer_list *tmr = (struct timer_list *)t;
struct thing *parent = from_timer(parent, tmr,
parent_timer_field);
... /* do stuff with parent */
```
Make some minor changes to `spl-condvar.c` and `spl-taskq.c` to use the
new timer APIs instead of conditional code.
Reviewed-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rafael Kitover <rkitover@gmail.com>
Closes #8647
@behlendorf this is my first version, want to run it through the CI.
These are the errors I noticed when I was doing the alpine testing, I happened to have 5.0-rc7 in that image.
I tested compiling with 4.20, 4.9 and 5.1-rc5.
I don't have a zfs system right now to test on, but I will try to do some testing on my laptop.
I haven't worked on this codebase before, so if anything is out of place or named wrong please let me know and I will fix it.
commit message below
kernel timer API rework
In
config/kernel-timer.m4check more generally for the newtimer_setup()APIs, but keep the old timer callback signature checkbecause some kernels (notably 4.14) have the new
timer_setup()API butuse the old callback signature. Also add a check for a
flagsmember instruct timer_list, much older kernels do not have it.Add compatibility shims to
include/spl/sys/timer.hto allow using thenew timer APIs with the only two caveats being that the callback
argument type must be declared as
spl_timer_list_tand an explicitassignment is required to get the timer variable for the
timer_of()macro. So the callback would look like this:
Make some minor changes to
spl-condvar.candspl-taskq.cto use thenew timer APIs instead of conditional code.
Also, to fix a problem in 5.0-rc7 which was the impetus for this work,
in
config/kernel-pde-data.m4check that thePDE_DATA()is availablein any form rather than checking for the symbol, as it is sometimes
defined as a function.
Signed-off-by: Rafael Kitover rkitover@gmail.com
Motivation and Context
I did this to fix the errors compiling against 5.0-rc7. Also it is my hope that the timer API changes are nicer than the previous iteration.
Types of changes
Checklist:
Signed-off-by.