Re: [PATCHSET] timer: clean up initializers and implement irqsafe timers

[Tejun Heo <tj@kernel.org>]

Hello, Thomas.

On Wed, Aug 15, 2012 at 01:33:09AM +0200, Thomas Gleixner wrote:
> To convince me to accept your patches you should start answering my
> questions and suggestions seriously in the first place and not
> discarding them upfront as lunatic visions.
> 
> As long as you can't provide a proper counter argument against
> maintaining the timer in the same context as the work, no matter what
> the underlying mechanism to achieve this will be, I'm not going to
> accept any of this hackery neither near next nor mainline.

Sure, that's exactly why the patches are posted for review, so you're
suggesting for workqueue implement essentially its own timer list - be
that a simple sorted linked list or priority heap.  Am I understanding
you correctly?

If so, we're comparing the following two.

a. Adding IRQSAFE timer.  Runtime cost is one additional if() in timer
   execution path.

b. Implementing workqueue's own timer system which is driven by timer
   so that the timer part can also be protected by the usual workqueue
   synchronization.

To me, #a seems like the better choice here.

delayed_work is one of the more common constructs used widely in the
kernel.  It's often used in device drivers to defer processing to
process context and timer queueing (including modification) is a
frequent operation.

IRQ handlers schedule them, some drivers use it to poll the device,
block layer uses it for most of deferred handling - SCSI layer failing
to issue a command due to resource shortage reschedules delayed_work
repeatedly, and so on.

Essentially, delayed_work might be used for any purpose a timer is
used.  Timer users switch to delayed_work if process context becomes
necessary for whatever reason, so, I don't think we can get away with
simple sorted list implementation.  We might be okay under some
workloads but O(N^2) insertion complexity for something as commonly
used as delayed_work doesn't seem like a good idea to me.

We could go for more involved implementation, say a priority heap or
somewhat simplified version of tvec_base, but that seems like a bad
tradeoff to me.  We would be trading off fairly complex chunk of code
duplicating an existing capability to avoid adding fairly small
feature to the timer.  It will likely be worse than the proper timer
and we have to maintain two chunks of code doing about the same thing
to save single if() in the existing timer code.

Let's see if we can agree on the latter point first.  Do you agree
that it wouldn't be a good idea to implement relatively complex timer
subsystem inside workqueue?

Thanks.

-- 
tejun