Messages in this thread | | | Date | Mon, 30 Oct 2017 14:22:31 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH] rcu: Convert timers to use timer_setup() |
| |
On Mon, Oct 30, 2017 at 02:13:03PM -0700, Kees Cook wrote: > On Mon, Oct 30, 2017 at 11:04 AM, Paul E. McKenney > <paulmck@linux.vnet.ibm.com> wrote: > > On Tue, Oct 24, 2017 at 02:32:04AM -0700, Kees Cook wrote: > >> In preparation for unconditionally passing the struct timer_list pointer to > >> all timer callbacks, switch to using the new timer_setup() and from_timer() > >> to pass the timer pointer explicitly. > >> > >> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > >> Cc: Josh Triplett <josh@joshtriplett.org> > >> Cc: Steven Rostedt <rostedt@goodmis.org> > >> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > >> Cc: Lai Jiangshan <jiangshanlai@gmail.com> > >> Signed-off-by: Kees Cook <keescook@chromium.org> > > > > One question below. > > > > Thanx, Paul > > > >> --- > >> kernel/rcu/rcutorture.c | 4 ++-- > >> kernel/rcu/tree_plugin.h | 9 +++++---- > >> 2 files changed, 7 insertions(+), 6 deletions(-) > >> > >> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c > >> index e1d3fa534ac0..b6fbbeb5a7da 100644 > >> --- a/kernel/rcu/rcutorture.c > >> +++ b/kernel/rcu/rcutorture.c > >> @@ -1078,7 +1078,7 @@ static void rcu_torture_timer_cb(struct rcu_head *rhp) > >> * counter in the element should never be greater than 1, otherwise, the > >> * RCU implementation is broken. > >> */ > >> -static void rcu_torture_timer(unsigned long unused) > >> +static void rcu_torture_timer(struct timer_list *unused) > >> { > >> int idx; > >> unsigned long started; > >> @@ -1165,7 +1165,7 @@ rcu_torture_reader(void *arg) > >> VERBOSE_TOROUT_STRING("rcu_torture_reader task started"); > >> set_user_nice(current, MAX_NICE); > >> if (irqreader && cur_ops->irq_capable) > >> - setup_timer_on_stack(&t, rcu_torture_timer, 0); > >> + timer_setup_on_stack(&t, rcu_torture_timer, 0); > >> > >> do { > >> if (irqreader && cur_ops->irq_capable) { > >> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h > >> index 8a5a3f9b1250..5fb7beee76aa 100644 > >> --- a/kernel/rcu/tree_plugin.h > >> +++ b/kernel/rcu/tree_plugin.h > >> @@ -2265,9 +2265,11 @@ static void do_nocb_deferred_wakeup_common(struct rcu_data *rdp) > >> } > >> > >> /* Do a deferred wakeup of rcu_nocb_kthread() from a timer handler. */ > >> -static void do_nocb_deferred_wakeup_timer(unsigned long x) > >> +static void do_nocb_deferred_wakeup_timer(struct timer_list *t) > >> { > >> - do_nocb_deferred_wakeup_common((struct rcu_data *)x); > >> + struct rcu_data *x = from_timer(x, t, nocb_timer); > > > > As long as we are creating a real typed variable for this could we > > please call it "rdp" in order to follow the usual RCU conventions? > > Sure thing! My scripts had tried to minimize variable name churn, but > this makes much more sense for what's happening in this function. > > > > > struct rcu_data *rdp = from_timer(rdp, t, nocb_timer); > > > >> + > >> + do_nocb_deferred_wakeup_common(x); > > > > And of course here: > > > > do_nocb_deferred_wakeup_common(rdp); > > > >> } > >> > >> /* > >> @@ -2331,8 +2333,7 @@ static void __init rcu_boot_init_nocb_percpu_data(struct rcu_data *rdp) > >> init_swait_queue_head(&rdp->nocb_wq); > >> rdp->nocb_follower_tail = &rdp->nocb_follower_head; > >> raw_spin_lock_init(&rdp->nocb_lock); > >> - setup_timer(&rdp->nocb_timer, do_nocb_deferred_wakeup_timer, > >> - (unsigned long)rdp); > >> + timer_setup(&rdp->nocb_timer, do_nocb_deferred_wakeup_timer, 0); > > > > Shouldn't this instead be something like this, give or take casts? > > > > timer_setup(&rdp->nocb_timer, do_nocb_deferred_wakeup_timer, rdp); > > Nope, the new timer API will unconditionally pass the timer pointer > (&rdp->nocb_timer) to the callback. The 3rd argument is flags. > > > Otherwise, I don't see how do_nocb_deferred_wakeup_common() avoids a > > NULL-pointer dereference. > > You can see the intermediate step to the API here: > > https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=timers/core&id=686fef928bba6be13cabe639f154af7d72b63120
Ah, got it, from_timer() does the needed conversion.
> > Given the fixes I called out, I am fine with your carrying it separately. > > I'll adjust the variable name and carry it in the timer tree. Thanks!
Sounds good!
Thanx, Paul
| |