Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1642448 > unrolled thread

RFC: better timer interface

Back to article view | Back to linux.kernel

Contents

  RFC: better timer interface Christoph Hellwig <hch@lst.de> - 2017-05-16 13:50 +0200
    [PATCH 3/9] kthread: remove unused macros Christoph Hellwig <hch@lst.de> - 2017-05-16 13:50 +0200
      Re: [PATCH 3/9] kthread: remove unused macros Petr Mladek <pmladek@suse.com> - 2017-05-17 14:20 +0200
        Re: [PATCH 3/9] kthread: remove unused macros Christoph Hellwig <hch@lst.de> - 2017-05-18 10:30 +0200
    [PATCH 8/9] tlclk: switch switchover_timer to a modern timer Christoph Hellwig <hch@lst.de> - 2017-05-16 13:50 +0200
    [PATCH 6/9] s390: switch topology_timer to a modern timer Christoph Hellwig <hch@lst.de> - 2017-05-16 13:50 +0200
    [PATCH 4/9] workqueue: switch to modern timers Christoph Hellwig <hch@lst.de> - 2017-05-16 13:50 +0200
    [PATCH 7/9] s390: switch lgr timer to a modern timer Christoph Hellwig <hch@lst.de> - 2017-05-16 13:50 +0200
    [PATCH 9/9] timers: remove old timer initialization macros Christoph Hellwig <hch@lst.de> - 2017-05-16 13:50 +0200
      Re: [PATCH 9/9] timers: remove old timer initialization macros Arnd Bergmann <arnd@arndb.de> - 2017-05-16 21:50 +0200
        Re: [PATCH 9/9] timers: remove old timer initialization macros Christoph Hellwig <hch@lst.de> - 2017-05-18 10:30 +0200
    [PATCH 2/9] timers: provide a "modern" variant of timers Christoph Hellwig <hch@lst.de> - 2017-05-16 14:00 +0200
      Re: [PATCH 2/9] timers: provide a "modern" variant of timers Randy Dunlap <rdunlap@infradead.org> - 2017-05-16 21:30 +0200
      Re: [PATCH 2/9] timers: provide a "modern" variant of timers Arnd Bergmann <arnd@arndb.de> - 2017-05-16 22:10 +0200
        Re: [PATCH 2/9] timers: provide a "modern" variant of timers Christoph Hellwig <hch@lst.de> - 2017-05-18 10:30 +0200
          Re: [PATCH 2/9] timers: provide a "modern" variant of timers Christoph Hellwig <hch@lst.de> - 2017-05-18 10:50 +0200
            Re: [PATCH 2/9] timers: provide a "modern" variant of timers Arnd Bergmann <arnd@arndb.de> - 2017-05-18 11:00 +0200
              Re: [PATCH 2/9] timers: provide a "modern" variant of timers Christoph Hellwig <hch@lst.de> - 2017-05-21 09:10 +0200
                Re: [PATCH 2/9] timers: provide a "modern" variant of timers Arnd Bergmann <arnd@arndb.de> - 2017-05-21 14:30 +0200
            Re: [PATCH 2/9] timers: provide a "modern" variant of timers Thomas Gleixner <tglx@linutronix.de> - 2017-05-21 20:00 +0200
              Re: [PATCH 2/9] timers: provide a "modern" variant of timers Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-21 20:30 +0200
      RE: [PATCH 2/9] timers: provide a "modern" variant of timers David Laight <David.Laight@ACULAB.COM> - 2017-05-19 12:50 +0200
        Re: [PATCH 2/9] timers: provide a "modern" variant of timers 'Christoph Hellwig' <hch@lst.de> - 2017-05-21 09:00 +0200
    [PATCH 5/9] powerpc/numa: switch topology_timer to modern timer Christoph Hellwig <hch@lst.de> - 2017-05-16 14:00 +0200
    Re: RFC: better timer interface Arnd Bergmann <arnd@arndb.de> - 2017-05-16 17:50 +0200
      Re: RFC: better timer interface Christoph Hellwig <hch@lst.de> - 2017-05-16 18:00 +0200
        Re: RFC: better timer interface Arnd Bergmann <arnd@arndb.de> - 2017-05-16 22:30 +0200
          Re: RFC: better timer interface Christoph Hellwig <hch@lst.de> - 2017-05-18 10:30 +0200
          Re: RFC: better timer interface Thomas Gleixner <tglx@linutronix.de> - 2017-05-21 19:20 +0200
            Re: RFC: better timer interface Thomas Gleixner <tglx@linutronix.de> - 2017-05-21 20:20 +0200
              Re: RFC: better timer interface Arnd Bergmann <arnd@arndb.de> - 2017-05-22 13:30 +0200
                Re: RFC: better timer interface Thomas Gleixner <tglx@linutronix.de> - 2017-05-22 21:30 +0200
              RE: RFC: better timer interface David Laight <David.Laight@ACULAB.COM> - 2017-05-23 13:40 +0200
                RE: RFC: better timer interface Thomas Gleixner <tglx@linutronix.de> - 2017-05-23 14:00 +0200
                  RE: RFC: better timer interface David Laight <David.Laight@ACULAB.COM> - 2017-05-23 15:00 +0200
                    RE: RFC: better timer interface Thomas Gleixner <tglx@linutronix.de> - 2017-05-23 15:10 +0200
            Re: RFC: better timer interface Arnd Bergmann <arnd@arndb.de> - 2017-05-22 15:40 +0200
              Re: RFC: better timer interface Thomas Gleixner <tglx@linutronix.de> - 2017-05-22 21:20 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1646389 — Re: [PATCH 2/9] timers: provide a "modern" variant of timers

On Sun, May 21, 2017 at 07:57:53PM +0200, Thomas Gleixner wrote:
> On Thu, 18 May 2017, Christoph Hellwig wrote:
> > On Thu, May 18, 2017 at 10:24:48AM +0200, Christoph Hellwig wrote:
> > > > b) give the union a name (breaks any reference to timer_list->func in C code):
> > > > 
> > > >  +       union {
> > > >  +               void            (*func)(struct timer_list *timer);
> > > >  +               void            (*function)(unsigned long);
> > > >  +       } u;
> > > 
> > > I'll look into that, as it seems a lot safer, and places outside
> > > the timer code shouldn't really touch it (although I bet they do,
> > > so more fixes for this series..)
> > 
> > Meh.  All the old init_timer users set function directly, so
> > I guess we need to use the other approach.
> 
> There is another possibility. Create a coccinelle script which wraps all
> 
>       timer.function = f;
>       timer->function = f;
> 
> assignements into a helper timer_set_function(timer, func) and ask Linus to
> run it right before the next -rc. That handles everything in tree and the
> few new instances in next can be addressed with patches sent to the
> maintainers.

FWIW, there was another possible approach - I toyed with that several years
ago, but it didn't go anywhere.  Namely, make timer.function take void *
*and* turn the setup part into setup(timer, callback, argument), verifying
that
	* callback(argument) will be acceptable expression for C typechecking
	* callback returns void
	* argument is a pointer type
then cast callback to void (*)(void *) and argument to void *.  That way
we get rid of any boilerplate in callbacks and get sane typechecking...

[toc] | [prev] | [next] | [standalone]

#1645508 — RE: [PATCH 2/9] timers: provide a "modern" variant of timers

From: Christoph Hellwig
> Sent: 16 May 2017 12:48
>
> The new callback gets a pointer to the timer_list itself, which can
> then be used to get the containing structure using container_of
> instead of casting from and to unsigned long all the time.

What about sensible drivers that put some other value in the 'data'
field?

Perhaps it ought to have been 'void *data'.

Seems retrograde to be passing the address of the timer structure
(which, in principle, the callers no nothing about).

So I wouldn't call it 'modern', just different.

	David

[toc] | [prev] | [next] | [standalone]

#1646268 — Re: [PATCH 2/9] timers: provide a "modern" variant of timers

On Fri, May 19, 2017 at 10:48:51AM +0000, David Laight wrote:
> From: Christoph Hellwig
> > Sent: 16 May 2017 12:48
> >
> > The new callback gets a pointer to the timer_list itself, which can
> > then be used to get the containing structure using container_of
> > instead of casting from and to unsigned long all the time.
> 
> What about sensible drivers that put some other value in the 'data'
> field?

They will add the equivalent of the data field into the containing
structure of the timer.  Just like we do for all other kernel interfaces
using the container_of patter, which includes just about every
primitive designed in the last 15 years.

[toc] | [prev] | [next] | [standalone]

#1642464 — [PATCH 5/9] powerpc/numa: switch topology_timer to modern timer

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/powerpc/mm/numa.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index 371792e4418f..93a11227716b 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -1437,7 +1437,7 @@ static void topology_schedule_update(void)
 	schedule_work(&topology_work);
 }
 
-static void topology_timer_fn(unsigned long ignored)
+static void topology_timer_fn(struct timer_list *timer)
 {
 	if (prrn_enabled && cpumask_weight(&cpu_associativity_changes_mask))
 		topology_schedule_update();
@@ -1447,8 +1447,7 @@ static void topology_timer_fn(unsigned long ignored)
 		reset_topology_timer();
 	}
 }
-static struct timer_list topology_timer =
-	TIMER_INITIALIZER(topology_timer_fn, 0, 0);
+static struct timer_list topology_timer = INIT_TIMER(topology_timer_fn, 0, 0);
 
 static void reset_topology_timer(void)
 {
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1642619

On Tue, May 16, 2017 at 1:48 PM, Christoph Hellwig <hch@lst.de> wrote:
> Hi all,
>
> this series attempts to provide a "modern" timer interface where the
> callback gets the timer_list structure as an argument so that it
> can use container_of instead of having to cast to/from unsigned long
> all the time (or even worse use function pointer casts, we have quite
> a few of those as well).

This looks really nice, but what is the long-term plan for the interface?
Do you expect that we will eventually change all 700+ users of timer_list
to the new type, or do we keep both variants around indefinitely to avoid
having to do mass-conversions?

If we are going to touch them all in the end, we might want to think
about other changes that could be useful here. The main one I have
in mind would be moving away from 'jiffies + timeout' as the interface,
and instead passing a relative number of milliseconds (or seconds)
into a mod_timer() variant. This is what most drivers want anyway,
and if we have both changes (callback argument and expiration
time) in place, we modernize the API one driver at a time with both
changes at once.

      Arnd

[toc] | [prev] | [next] | [standalone]

#1642633

On Tue, May 16, 2017 at 05:45:07PM +0200, Arnd Bergmann wrote:
> This looks really nice, but what is the long-term plan for the interface?
> Do you expect that we will eventually change all 700+ users of timer_list
> to the new type, or do we keep both variants around indefinitely to avoid
> having to do mass-conversions?

I think we should eventually move everyone over, but it might take
some time.

> If we are going to touch them all in the end, we might want to think
> about other changes that could be useful here. The main one I have
> in mind would be moving away from 'jiffies + timeout' as the interface,
> and instead passing a relative number of milliseconds (or seconds)
> into a mod_timer() variant. This is what most drivers want anyway,
> and if we have both changes (callback argument and expiration
> time) in place, we modernize the API one driver at a time with both
> changes at once.

Yes, that sounds useful to me as well.  As you said it's an independent
but somewhat related change.  I can add it to my series, but I'll
need a suggestions for a good and short name.  That already was the
hardest part for the setup side :)

[toc] | [prev] | [next] | [standalone]

#1642808

On Tue, May 16, 2017 at 5:51 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Tue, May 16, 2017 at 05:45:07PM +0200, Arnd Bergmann wrote:
>> This looks really nice, but what is the long-term plan for the interface?
>> Do you expect that we will eventually change all 700+ users of timer_list
>> to the new type, or do we keep both variants around indefinitely to avoid
>> having to do mass-conversions?
>
> I think we should eventually move everyone over, but it might take
> some time.

Ok.

>> If we are going to touch them all in the end, we might want to think
>> about other changes that could be useful here. The main one I have
>> in mind would be moving away from 'jiffies + timeout' as the interface,
>> and instead passing a relative number of milliseconds (or seconds)
>> into a mod_timer() variant. This is what most drivers want anyway,
>> and if we have both changes (callback argument and expiration
>> time) in place, we modernize the API one driver at a time with both
>> changes at once.
>
> Yes, that sounds useful to me as well.  As you said it's an independent
> but somewhat related change.  I can add it to my series, but I'll
> need a suggestions for a good and short name.  That already was the
> hardest part for the setup side :)

If we keep the unusual *_timer() naming (rather than timer_*() as hrtimer
has), we could use one of

a) start_timer(struct timer_list *timer, unsigned long ms);
b) restart_timer(struct timer_list *timer, unsigned long ms);
c) mod_timer_ms(struct timer_list *timer, unsigned long ms);
    mod_timer_sec(struct timer_list *timer, unsigned long sec);

The first is slightly shorter but conflicts with three files that use
the same name for a local function name. The third one fits
well with the existing interfaces and provides both millisecond
and second versions, I'd probably go with that.

We could consider even passing a default interval as another
argument to prepare_timer(), and using that in add_timer(),
but that would in those cases that have a constant interval
(maybe about half of the users from) and would be a bit surprising
to readers that are only familiar with the existing interfaces.

One final option would be a larger-scale replacement of
the API by mirroring the hrtimer style where possible while
staying compatible with the existing calls, e.g. timer_prepare(),
timer_add_expires(), timer_start(), ...

       Arnd

[toc] | [prev] | [next] | [standalone]

#1643947

On Tue, May 16, 2017 at 10:26:39PM +0200, Arnd Bergmann wrote:
> If we keep the unusual *_timer() naming (rather than timer_*() as hrtimer
> has), we could use one of
> 
> a) start_timer(struct timer_list *timer, unsigned long ms);
> b) restart_timer(struct timer_list *timer, unsigned long ms);
> c) mod_timer_ms(struct timer_list *timer, unsigned long ms);
>     mod_timer_sec(struct timer_list *timer, unsigned long sec);
> 
> The first is slightly shorter but conflicts with three files that use
> the same name for a local function name. The third one fits
> well with the existing interfaces and provides both millisecond
> and second versions, I'd probably go with that.

Yeah, I'd take c) as well.  I'll give it a spin.

> We could consider even passing a default interval as another
> argument to prepare_timer(), and using that in add_timer(),
> but that would in those cases that have a constant interval
> (maybe about half of the users from) and would be a bit surprising
> to readers that are only familiar with the existing interfaces.

That seems rather ugly to me.

> One final option would be a larger-scale replacement of
> the API by mirroring the hrtimer style where possible while
> staying compatible with the existing calls, e.g. timer_prepare(),
> timer_add_expires(), timer_start(), ...

I'd chose timer_* for an entirely new API, but at this point this
seems a bit too much churn to me.

[toc] | [prev] | [next] | [standalone]

#1646381

On Tue, 16 May 2017, Arnd Bergmann wrote:
> On Tue, May 16, 2017 at 5:51 PM, Christoph Hellwig <hch@lst.de> wrote:
> > Yes, that sounds useful to me as well.  As you said it's an independent
> > but somewhat related change.  I can add it to my series, but I'll
> > need a suggestions for a good and short name.  That already was the
> > hardest part for the setup side :)
> 
> If we keep the unusual *_timer() naming (rather than timer_*() as hrtimer
> has), we could use one of
> 
> a) start_timer(struct timer_list *timer, unsigned long ms);
> b) restart_timer(struct timer_list *timer, unsigned long ms);
> c) mod_timer_ms(struct timer_list *timer, unsigned long ms);
>     mod_timer_sec(struct timer_list *timer, unsigned long sec);

Please make new functions prefixed with timer_ and get rid of that old
interface completely. It's horrible.
 
timer_init()
timer_start(timer, ms, abs)
timer_start_on(timer, ms, abs, cpu)
timer_cancel(timer, sync)

Is all what's required to make up a new milliseconds based interface.

We really do not need all that mod/restart/ whatever variants. Where is the
point of those?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1646388

On Sun, 21 May 2017, Thomas Gleixner wrote:
> On Tue, 16 May 2017, Arnd Bergmann wrote:
> > On Tue, May 16, 2017 at 5:51 PM, Christoph Hellwig <hch@lst.de> wrote:
> > > Yes, that sounds useful to me as well.  As you said it's an independent
> > > but somewhat related change.  I can add it to my series, but I'll
> > > need a suggestions for a good and short name.  That already was the
> > > hardest part for the setup side :)
> > 
> > If we keep the unusual *_timer() naming (rather than timer_*() as hrtimer
> > has), we could use one of
> > 
> > a) start_timer(struct timer_list *timer, unsigned long ms);
> > b) restart_timer(struct timer_list *timer, unsigned long ms);
> > c) mod_timer_ms(struct timer_list *timer, unsigned long ms);
> >     mod_timer_sec(struct timer_list *timer, unsigned long sec);
> 
> Please make new functions prefixed with timer_ and get rid of that old
> interface completely. It's horrible.
>  
> timer_init()
> timer_start(timer, ms, abs)

I'm not even sure, whether we need absolute timer wheel timers at
all, because most use cases are relative to now.

But it's easy enough to provide them. All we need for that is something
like

	unsigned long time_msec;	

which gets incremented every tick by the appropriate amount of
milliseconds.

Having that would also allow to replace all the

       end = jiffies + msec_to_jiffies(xxx);

       while (time_before(jiffies, end))
       	     ....

constructs with a milliseconds based machinery. So we can remove all
*_to_jiffies() interfaces over time.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1646764

On Sun, May 21, 2017 at 8:14 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Sun, 21 May 2017, Thomas Gleixner wrote:
>> On Tue, 16 May 2017, Arnd Bergmann wrote:
>> > On Tue, May 16, 2017 at 5:51 PM, Christoph Hellwig <hch@lst.de> wrote:
>> > > Yes, that sounds useful to me as well.  As you said it's an independent
>> > > but somewhat related change.  I can add it to my series, but I'll
>> > > need a suggestions for a good and short name.  That already was the
>> > > hardest part for the setup side :)
>> >
>> > If we keep the unusual *_timer() naming (rather than timer_*() as hrtimer
>> > has), we could use one of
>> >
>> > a) start_timer(struct timer_list *timer, unsigned long ms);
>> > b) restart_timer(struct timer_list *timer, unsigned long ms);
>> > c) mod_timer_ms(struct timer_list *timer, unsigned long ms);
>> >     mod_timer_sec(struct timer_list *timer, unsigned long sec);
>>
>> Please make new functions prefixed with timer_ and get rid of that old
>> interface completely. It's horrible.
>>
>> timer_init()
>> timer_start(timer, ms, abs)
>
> I'm not even sure, whether we need absolute timer wheel timers at
> all, because most use cases are relative to now.
>
> But it's easy enough to provide them. All we need for that is something
> like
>
>         unsigned long time_msec;
>
> which gets incremented every tick by the appropriate amount of
> milliseconds.
>
> Having that would also allow to replace all the
>
>        end = jiffies + msec_to_jiffies(xxx);
>
>        while (time_before(jiffies, end))
>              ....
>
> constructs with a milliseconds based machinery. So we can remove all
> *_to_jiffies() interfaces over time.

A lot of those users could probably just ktime_get()/ktime_before() here,
as they would by definition not be performance critical.

I don't see a way to just tk->tkr_mono.base but with a ktime_get_coarse()
we could just return the ktime_t of the last tick and not even need a seqlock
on 64-bit architectures, or have to introduce a new API.

        Arnd

[toc] | [prev] | [next] | [standalone]

#1647287

On Mon, 22 May 2017, Arnd Bergmann wrote:
> On Sun, May 21, 2017 at 8:14 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > But it's easy enough to provide them. All we need for that is something
> > like
> >
> >         unsigned long time_msec;
> >
> > which gets incremented every tick by the appropriate amount of
> > milliseconds.
> >
> > Having that would also allow to replace all the
> >
> >        end = jiffies + msec_to_jiffies(xxx);
> >
> >        while (time_before(jiffies, end))
> >              ....
> >
> > constructs with a milliseconds based machinery. So we can remove all
> > *_to_jiffies() interfaces over time.
> 
> A lot of those users could probably just ktime_get()/ktime_before() here,
> as they would by definition not be performance critical.

Right.

> I don't see a way to just tk->tkr_mono.base but with a ktime_get_coarse()
> we could just return the ktime_t of the last tick and not even need a seqlock
> on 64-bit architectures, or have to introduce a new API.

Yeah, that would be possible, but OTOH, for those loop thingies it probably
does not matter at all whether you have the overhead of ktime_get() or
not. We need to look at that stuff deeper.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1647956

From: Thomas Gleixner
> Sent: 21 May 2017 19:15
...
> > timer_start(timer, ms, abs)
> 
> I'm not even sure, whether we need absolute timer wheel timers at
> all, because most use cases are relative to now.

Posix requires absolute timers for some userspace calls
(annoying because the code often wants relative).

OTOH how much conditional code is there for the 'abs' argument.
And is there any code that doesn't pass a constant?

Certainly worth a separate timer_start_abs(timer, wall_time)
function since you can't correctly map a wall_time timer
to a jiffies one.

	David

[toc] | [prev] | [next] | [standalone]

#1647963

On Tue, 23 May 2017, David Laight wrote:

> From: Thomas Gleixner
> > Sent: 21 May 2017 19:15
> ...
> > > timer_start(timer, ms, abs)
> > 
> > I'm not even sure, whether we need absolute timer wheel timers at
> > all, because most use cases are relative to now.
> 
> Posix requires absolute timers for some userspace calls
> (annoying because the code often wants relative).

Posix is completely irrelevant here. These timers are purely kernel
internal.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1647996

From: Thomas Gleixner
> Sent: 23 May 2017 12:59
> On Tue, 23 May 2017, David Laight wrote:
> 
> > From: Thomas Gleixner
> > > Sent: 21 May 2017 19:15
> > ...
> > > > timer_start(timer, ms, abs)
> > >
> > > I'm not even sure, whether we need absolute timer wheel timers at
> > > all, because most use cases are relative to now.
> >
> > Posix requires absolute timers for some userspace calls
> > (annoying because the code often wants relative).
> 
> Posix is completely irrelevant here. These timers are purely kernel
> internal.

Somehow pthread_cond_timedwait() has to be implemented.
Doing so without kernel timers that use absolute 'wall clock' time is tricky.

	David

[toc] | [prev] | [next] | [standalone]

#1648010

On Tue, 23 May 2017, David Laight wrote:
> From: Thomas Gleixner
> > Sent: 23 May 2017 12:59
> > On Tue, 23 May 2017, David Laight wrote:
> > 
> > > From: Thomas Gleixner
> > > > Sent: 21 May 2017 19:15
> > > ...
> > > > > timer_start(timer, ms, abs)
> > > >
> > > > I'm not even sure, whether we need absolute timer wheel timers at
> > > > all, because most use cases are relative to now.
> > >
> > > Posix requires absolute timers for some userspace calls
> > > (annoying because the code often wants relative).
> > 
> > Posix is completely irrelevant here. These timers are purely kernel
> > internal.
> 
> Somehow pthread_cond_timedwait() has to be implemented.
> Doing so without kernel timers that use absolute 'wall clock' time is tricky.

Oh well. The timer wheel timers are NOT used to implement any posix
interface. That's all handled by hrtimers and they are not debated here.

So still nothing to see here.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1646912

On Sun, May 21, 2017 at 7:13 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 16 May 2017, Arnd Bergmann wrote:
>> On Tue, May 16, 2017 at 5:51 PM, Christoph Hellwig <hch@lst.de> wrote:
>> > Yes, that sounds useful to me as well.  As you said it's an independent
>> > but somewhat related change.  I can add it to my series, but I'll
>> > need a suggestions for a good and short name.  That already was the
>> > hardest part for the setup side :)
>>
>> If we keep the unusual *_timer() naming (rather than timer_*() as hrtimer
>> has), we could use one of
>>
>> a) start_timer(struct timer_list *timer, unsigned long ms);
>> b) restart_timer(struct timer_list *timer, unsigned long ms);
>> c) mod_timer_ms(struct timer_list *timer, unsigned long ms);
>>     mod_timer_sec(struct timer_list *timer, unsigned long sec);
>
> Please make new functions prefixed with timer_ and get rid of that old
> interface completely. It's horrible.
>
> timer_init()
> timer_start(timer, ms, abs)
> timer_start_on(timer, ms, abs, cpu)
> timer_cancel(timer, sync)
>
> Is all what's required to make up a new milliseconds based interface.
>
> We really do not need all that mod/restart/ whatever variants. Where is the
> point of those?

I agree, one of the above is good enough, if we do the large-scale API
replacement. Having both ms and sec variants would be for convenience
to avoid having lots of open-coded  '*MSEC_PER_SEC) multiplications.

We still need at least three variants of timer_init, for statically initialized
timers, dynamic allocation and on-stack allocation, as before.

For the 'abs' argument, I'd probably leave that out until we find code
that actually needs it and that can't use hrtimer as easily.

For timer_start_on(), that would be a replacement of add_timer_on(),
which only has seven callers today:

arch/x86/kernel/apic/x2apic_uv_x.c:             add_timer_on(timer, cpu);
drivers/tty/metag_da.c: add_timer_on(poll_timer, 0);
drivers/tty/mips_ejtag_fdc.c:
add_timer_on(&priv->poll_timer, dev->cpu);
drivers/tty/mips_ejtag_fdc.c:
add_timer_on(&priv->poll_timer, dev->cpu);
kernel/time/clocksource.c:      add_timer_on(&watchdog_timer, next_cpu);
kernel/time/clocksource.c:      add_timer_on(&watchdog_timer,
cpumask_first(cpu_online_mask));
kernel/workqueue.c:             add_timer_on(timer, cpu);

If hrtimer isn't already a better interface for those, we can probably
convert them all to the new API at once.

       Arnd

[toc] | [prev] | [next] | [standalone]

#1647283

On Mon, 22 May 2017, Arnd Bergmann wrote:
> On Sun, May 21, 2017 at 7:13 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> I agree, one of the above is good enough, if we do the large-scale API
> replacement. Having both ms and sec variants would be for convenience
> to avoid having lots of open-coded  '*MSEC_PER_SEC) multiplications.

Right, but that _sec variant is just a wrapper around the _ms functionality.

> We still need at least three variants of timer_init, for statically initialized
> timers, dynamic allocation and on-stack allocation, as before.

Again, that's variants. What I wanted to avoid is that mod_ restart_ start_
whatever distinction. It's pointless.

> For the 'abs' argument, I'd probably leave that out until we find code
> that actually needs it and that can't use hrtimer as easily.

> For timer_start_on(), that would be a replacement of add_timer_on(),
> which only has seven callers today:

> arch/x86/kernel/apic/x2apic_uv_x.c:             add_timer_on(timer, cpu);
> drivers/tty/metag_da.c: add_timer_on(poll_timer, 0);
> drivers/tty/mips_ejtag_fdc.c:
> add_timer_on(&priv->poll_timer, dev->cpu);
> drivers/tty/mips_ejtag_fdc.c:
> add_timer_on(&priv->poll_timer, dev->cpu);
> kernel/time/clocksource.c:      add_timer_on(&watchdog_timer, next_cpu);
> kernel/time/clocksource.c:      add_timer_on(&watchdog_timer,
> cpumask_first(cpu_online_mask));
> kernel/workqueue.c:             add_timer_on(timer, cpu);
> 
> If hrtimer isn't already a better interface for those, we can probably
> convert them all to the new API at once.

Some of them certainly can be converted. Though I'm starting to worry about
massive hrtimer usage, which can really become a burden because the expiry
functions run from hard interrupt context. We already have this horrible
construct of hrtimer -> tasklet -> real function, which is used to get the
hrtimer expiry out into softirq context.

We have similar issue in RT, where we need to move out everything which is
not hard irq safe to softirq context. I'm working on a solution there which
is less ugly than what we have now in RT. It's moving non irq safe timers
onto a different time base, e.g. MONOTONIC_SOFT, which merily wakes the
softirq when the first queued timer expires and let that one sort out the
expiry of those timers. It's WIP and should be available soon.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web