Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just a few suggested tweaks. We should manually run the splat taskq tests on the new kernel as well to verify correct functionality.
config/spl-build.m4
Outdated
SPL_LINUX_TRY_COMPILE([ | ||
#include <linux/timer.h> | ||
],[ | ||
void task_expire(struct timer_list *tl) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to pull this under the #include
so it's outside the main function.
module/spl/spl-taskq.c
Outdated
(_ptr)->function = _func;\ | ||
(_ptr)->data = _data;\ | ||
} while (0) | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this as a normal function.
module/spl/spl-taskq.c
Outdated
task_expire(unsigned long data) | ||
{ | ||
taskq_ent_t *w, *t = (taskq_ent_t *)data; | ||
taskq_ent_t *t = (taskq_ent_t *)data; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this with a wrapper function instead for readability.
static void
task_expire_impl(taskq_ent_t *t)
{
}
#if defined(HAVE_KERNEL_TASK_EXPIRE_TIMER_LIST)
static void
task_expire(struct timer_list *tl)
{
return (task_expire_impl(from_timer(t, tl, tqent_timer)));
}
#else
static void
task_expire(unsigned long data)
{
return (task_expire_impl((taskq_ent_t *)data));
}
#endif /* HAVE_KERNEL_TASK_EXPIRE_TIMER_LIST */
f164ff7
to
82a0d7a
Compare
@behlendorf I simplified the code in my latest push, please take another look. It's also now passing splat testing on ubuntu17-w/4.15, F27, and Centos 6.8. |
config/spl-build.m4
Outdated
dnl # | ||
dnl # 4.15 API change | ||
dnl # https://lkml.org/lkml/2017/11/25/90 | ||
dnl # struct timer_list.data exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be checking for the new interface not the old one, why not check for the from_timer
interface which happens to have been added in the same torvalds/linux@686fef9 commit.
module/spl/spl-taskq.c
Outdated
task_expire(struct timer_list *tl) | ||
{ | ||
taskq_ent_t *t; | ||
task_expire_impl(from_timer(t, tl, tqent_timer)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Let's use the new recommended kernel style convention for this is:
task_expire(struct timer_list *tl)
{
taskq_ent_t *t = from_timer(t, tl, tqent_timer);
task_expire_impl(t);
}
module/spl/spl-taskq.c
Outdated
init_timer(&t->tqent_timer); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to hide away all this uglyness away in a spl_timer_setup()
wrapper.
82a0d7a
to
df5cba3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I like this a lot better. Just a few additional comments.
config/spl-build.m4
Outdated
],[ | ||
AC_MSG_RESULT(yes) | ||
AC_DEFINE(HAVE_KERNEL_TIMER_FUNCTION_TIMER_LIST, 1, | ||
[timer_list.function gets a timer_list ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra tailing whitespace after timer_list
config/spl-build.m4
Outdated
dnl # 4.15 API change | ||
dnl # https://lkml.org/lkml/2017/11/25/90 | ||
dnl # Check if timer_list.func get passed a timer_list or an unsigned long | ||
dnl # (older kernels). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to checking for the callback prototype can you also verify from_timer()
and timer_setup()
in this configure
check. Let's be strict about only using this new interface when everything is exactly as expected.
Use timer_setup() macro and new timeout function definition. Signed-off-by: Tony Hutter <hutter2@llnl.gov> Closes: openzfs#670
df5cba3
to
60b22ba
Compare
@behlendorf my latest push now checks for |
Use timer_setup() macro and new timeout function definition. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Tony Hutter <hutter2@llnl.gov> Closes openzfs#670 Closes openzfs#671
Use timer_setup() macro and new timeout function definition.
Closes: #670