Re: [RFC 1/2] timers: Add pending timer bool in struct timer_base

Re: [RFC 1/2] timers: Add pending timer bool in struct timer_base

[Thomas Gleixner]

Nicolas,

On Thu, Jun 10 2021 at 14:59, Nicolas Saenz Julienne wrote:

please always Cc the relevant mailing lists and the maintainers.
MAINTAINERS exists for a reason.

> We need to efficiently check whether a timer base has no pending
> events.

'We need' is not a technical explanation. That's close to 'I want a pony'.

Please describe what you are trying to solve and why the existing
mechanisms are not good enough.

See Documentation/process/submitting-patches.rst

> So introduce a new variable in struct timer_base to do so.

The variable solves your problem? Interesting solution.

>  		base->next_expiry = bucket_expiry;
>  		base->next_expiry_recalc = false;
> +#ifdef CONFIG_PREEMPT_RT
> +		base->pending = true;
> +#endif

What is RT specific about that?

>  		trigger_dyntick_cpu(base, timer);
>  	}
>  }
> @@ -1598,6 +1602,9 @@ static unsigned long __next_timer_interrupt(struct timer_base *base)
>  	}
>  
>  	base->next_expiry_recalc = false;
> +#ifdef CONFIG_PREEMPT_RT
> +	base->pending = (next != base->clk + NEXT_TIMER_MAX_DELTA);
> +#endif

This lacks any information about the semantics of this flag:

  - When is it valid and when not?
  - What is the valid use case for this flag?

Summary of the supplied information: We need a flag, so we added one.

Sorry that's not sufficient.

Thanks,

        tglx

  

Re: [RFC 2/2] timers: Make sure irq_work is handled when no pending timers

[Thomas Gleixner]

Nicolas,

On Thu, Jun 10 2021 at 14:59, Nicolas Saenz Julienne wrote:
> PREEMPT_RT systems defer most irq_work handling into the timer softirq.
> This softirq is only triggered when a timer is expired, which adds some
> delay to the irq_work handling. It's a price PREEMPT_RT systems are
> willing to pay in exchange for less IRQ noise.
>
> This works fine for the majority of systems, but there's a catch. What
> if no timer is ever armed after an irq_work is queued. This has been
> observed on nohz_full CPUs while running oslat. The lack of armed timers
> prevents a pending irq_work to run. Which in turn prevents the nohz code
> from fully stopping the tick.
>
> To avoid this situation introduce new logic in run_local_timers(). The
> timer softirq will be triggered when an irq_work is pending but no
> timers have been armed. This situation is only possible in PREEMPT_RT
> systems, so make the code conditional to it.

now I can see the problem you are trying to solve, but unfortunately the
solution is fundamentally wrong.

> NOTE: All in all, this is the best I could think of with my limited
>       timers knowledge. A bigger hammer would be to unanimously trigger
>       the softirq if irq_work_needs_cpu(). But I get the feeling this is
>       something we want to avoid.

Technical decisions based on feelings are not solving anything and they
result in hard to analyse subtle issues:

Q: Assume there is a timer armed to expire 24h from now. What's the
   difference to no timer being armed?

A: None.

   Just because your use case has either no timers armed at all or has
   timers armed with short expiry is no reason to ignore the really
   simple and obvious technical facts.

But that aside, you analyzed the problem pretty good, but then you
stopped short of identifying the root cause and went off to cure the
symptom.

The irq_work deferring on RT to the timer softirq is the root cause of
the problem. It's a sloppy hack and I'm well aware of that. So this
needs to be fixed and not worked around by adding random undocumented
workarounds into the timer code.

Let's take a step back and analyze why this deferring to timer softirq
context exists on RT.

 1) The irq_work callbacks which are deferred on RT cannot be invoked from
    hard interrupt (IPI) context usually - due to spin_lock usage.

 2) Such irq_work has to be delegated to some suitable context and the
    trivial and lazy way out was to just stick into the timer softirq.

That hack survived for a long time and while I was aware of it, it was
not really high on my priority list of cleanups.

The real solution is to delegate this to a suitable context which is
executed independent of any other constraints.

There are two solutions:

  1) Create a IRQ_WORK softirq and raise that

  2) Simply delegate it to a workqueue

So now you might rightfully ask why I did not do that back then:

  #1 is not an option because we don't want to proliferate softirqs for
     various reasons.

  #2 was not feasible because back then queueing work from hard
     interrupt context was not possible.

So yes, I took the sloppy and easy way out and just glued it onto the
timer softirqs. Nobody complained so far.

As we since then made work queues RT aware and it's possible to queue
work from the irq_work IPI context, the obvious solution is to delegate
this to a work queue.

If we do a proper analysis of the affected irq work callbacks then this
probably makes a lot of sense independent of RT. There are only a few
really urgent irq work items which need to be handled immediately in the
IPI.

RT is special, but as we have demonstrated over time it's not _that_
special. It just needs a proper analysis and a real good argument why
something has to be special for RT and does not fit into the common
case. Or to demonstrate that the common case approach of 'do it right
away' is pointless or even harmfull.

Thanks,

        tglx
---
P.S.: I'm not blaming this on you as a newcomer. There are people on
      your team who should have known better.   

 

Re: [RFC 2/2] timers: Make sure irq_work is handled when no pending timers

[Thomas Gleixner]

On Sat, Jun 19 2021 at 00:47, Thomas Gleixner wrote:
> On Thu, Jun 10 2021 at 14:59, Nicolas Saenz Julienne wrote:
> As we since then made work queues RT aware and it's possible to queue
> work from the irq_work IPI context, the obvious solution is to delegate
> this to a work queue.
>
> If we do a proper analysis of the affected irq work callbacks then this
> probably makes a lot of sense independent of RT. There are only a few
> really urgent irq work items which need to be handled immediately in the
> IPI.
>
> RT is special, but as we have demonstrated over time it's not _that_
> special. It just needs a proper analysis and a real good argument why
> something has to be special for RT and does not fit into the common
> case. Or to demonstrate that the common case approach of 'do it right
> away' is pointless or even harmfull.

I skimmed most of the ~40 irq_work instances.

Most of them have no urgency at all. And out of those non-urgent cases
the majority does not even have the requirement to run on the current
CPU, so they can be pushed off to a global work queue which moves them
away from NOHZ full CPUs completely.

That has nothing to do with RT, that's a benefit in general.

Thanks,

        tglx

Re: [RFC 2/2] timers: Make sure irq_work is handled when no pending timers

[Peter Zijlstra]

On Sat, Jun 19, 2021 at 12:47:04AM +0200, Thomas Gleixner wrote:
> There are two solutions:
> 
>   1) Create a IRQ_WORK softirq and raise that
> 
>   2) Simply delegate it to a workqueue

IIRC someone was looking to stick the whole thing in a kthread_worker.
Jiri, was that you?

Re: [RFC 2/2] timers: Make sure irq_work is handled when no pending timers

[Jiri Olsa]

On Tue, Jun 22, 2021 at 03:44:23PM +0200, Peter Zijlstra wrote:
> On Sat, Jun 19, 2021 at 12:47:04AM +0200, Thomas Gleixner wrote:
> > There are two solutions:
> > 
> >   1) Create a IRQ_WORK softirq and raise that
> > 
> >   2) Simply delegate it to a workqueue
> 
> IIRC someone was looking to stick the whole thing in a kthread_worker.
> Jiri, was that you?
> 

yep, I still plan on doing that

jirka

Re: [RFC 2/2] timers: Make sure irq_work is handled when no pending timers

[Jiri Olsa]

On Tue, Jun 22, 2021 at 07:33:23PM +0200, Jiri Olsa wrote:
> On Tue, Jun 22, 2021 at 03:44:23PM +0200, Peter Zijlstra wrote:
> > On Sat, Jun 19, 2021 at 12:47:04AM +0200, Thomas Gleixner wrote:
> > > There are two solutions:
> > > 
> > >   1) Create a IRQ_WORK softirq and raise that
> > > 
> > >   2) Simply delegate it to a workqueue
> > 
> > IIRC someone was looking to stick the whole thing in a kthread_worker.
> > Jiri, was that you?
> > 
> 
> yep, I still plan on doing that

hum, IIRC that was actually perf specific change we discussed some
time ago I should have read the whole thread before answering

I'll check what was my plan to do and get back ;-)

jirka