Please accept my apologies for losing track of this one, and for
top-posting to any of you who might be sticklers for that sort of thing.
I must pull this commit out of my set for the next merge window, apply
it to the group for the next merge window, and try out Eric's suggested
changes. Might still make the next merge window, but clearly not in
its current condition.
If it has taken some other path in the meantime, please do let me know!
Thanx, Paul
On Wed, Dec 11, 2019 at 10:57:24PM -0800, Eric Dumazet wrote:
> On Wed, Dec 11, 2019 at 10:38 PM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> >
> > On Wed, Dec 11, 2019 at 10:02 PM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Dec 12, 2019 at 04:06:22PM +1100, Stephen Rothwell wrote:
> > > > Hi all,
> > > >
> > > > After merging the rcu (I think) tree, today's linux-next build (x86_64
> > > > allnoconfig) produced this warning:
> > > >
> > > > kernel/time/timer.c: In function 'schedule_timeout':
> > > > kernel/time/timer.c:969:20: warning: 'timer.expires' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > > 969 | long diff = timer->expires - expires;
> > > > | ~~~~~^~~~~~~~~
> > > >
> > > > Introduced by (bisected to) commit
> > > >
> > > > c4127fce1d02 ("timer: Use hlist_unhashed_lockless() in timer_pending()")
> > > >
> > > > x86_64-linux-gnu-gcc (Debian 9.2.1-21) 9.2.1 20191130
> > >
> > > Well, if the timer is pending, then ->expires has to have been
> > > initialized, but off where the compiler cannot see it, such as during a
> > > previous call to __mod_timer(). And the change may have made it harder
> > > for the compiler to see all of these relationships, but...
> > >
> > > I don't see this warning with gcc version 7.4.0. Just out of curiosity,
> > > what are you running, Stephen?
> > >
> > > Eric, any thoughts for properly educating the compiler on this one?
> >
> > Ah... the READ_ONCE() apparently turns off the compiler ability to
> > infer that this branch should not be taken.
> >
> > Since __mod_timer() is inlined we could perhaps add a new option
> >
> > diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> > index 4820823515e9..8bbce552568b 100644
> > --- a/kernel/time/timer.c
> > +++ b/kernel/time/timer.c
> > @@ -944,6 +944,7 @@ static struct timer_base *lock_timer_base(struct
> > timer_list *timer,
> >
> > #define MOD_TIMER_PENDING_ONLY 0x01
> > #define MOD_TIMER_REDUCE 0x02
> > +#define MOD_TIMER_NOTPENDING 0x04
> >
> > static inline int
> > __mod_timer(struct timer_list *timer, unsigned long expires, unsigned
> > int options)
> > @@ -960,7 +961,7 @@ __mod_timer(struct timer_list *timer, unsigned
> > long expires, unsigned int option
> > * the timer is re-modified to have the same timeout or ends up in the
> > * same array bucket then just return:
> > */
> > - if (timer_pending(timer)) {
> > + if (!(options & MOD_TIMER_NOTPENDING) && timer_pending(timer)) {
> > /*
> > * The downside of this optimization is that it can result in
> > * larger granularity than you would get from adding a new
> > @@ -1891,7 +1892,7 @@ signed long __sched schedule_timeout(signed long timeout)
> >
> > timer.task = current;
> > timer_setup_on_stack(&timer.timer, process_timeout, 0);
> > - __mod_timer(&timer.timer, expire, 0);
> > + __mod_timer(&timer.timer, expire, MOD_TIMER_NOTPENDING);
> > schedule();
> > del_singleshot_timer_sync(&timer.timer);
>
>
> Also add_timer() can benefit from the same hint, since it seems inlined as well.
>
> (untested patch)
>
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 4820823515e9..568564ae3597 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -944,6 +944,7 @@ static struct timer_base *lock_timer_base(struct
> timer_list *timer,
>
> #define MOD_TIMER_PENDING_ONLY 0x01
> #define MOD_TIMER_REDUCE 0x02
> +#define MOD_TIMER_NOTPENDING 0x04
>
> static inline int
> __mod_timer(struct timer_list *timer, unsigned long expires, unsigned
> int options)
> @@ -960,7 +961,7 @@ __mod_timer(struct timer_list *timer, unsigned
> long expires, unsigned int option
> * the timer is re-modified to have the same timeout or ends up in the
> * same array bucket then just return:
> */
> - if (timer_pending(timer)) {
> + if (!(options & MOD_TIMER_NOTPENDING) && timer_pending(timer)) {
> /*
> * The downside of this optimization is that it can result in
> * larger granularity than you would get from adding a new
> @@ -1133,7 +1134,7 @@ EXPORT_SYMBOL(timer_reduce);
> void add_timer(struct timer_list *timer)
> {
> BUG_ON(timer_pending(timer));
> - mod_timer(timer, timer->expires);
> + __mod_timer(timer, timer->expires, MOD_TIMER_NOTPENDING);
> }
> EXPORT_SYMBOL(add_timer);
>
> @@ -1891,7 +1892,7 @@ signed long __sched schedule_timeout(signed long timeout)
>
> timer.task = current;
> timer_setup_on_stack(&timer.timer, process_timeout, 0);
> - __mod_timer(&timer.timer, expire, 0);
> + __mod_timer(&timer.timer, expire, MOD_TIMER_NOTPENDING);
> schedule();
> del_singleshot_timer_sync(&timer.timer);
