* Re: [RFC 1/2] timers: Add pending timer bool in struct timer_base [not found] <20210610125945.558872-1-nsaenzju@redhat.com> @ 2021-06-18 21:06 ` Thomas Gleixner [not found] ` <20210610125945.558872-2-nsaenzju@redhat.com> 1 sibling, 0 replies; 6+ messages in thread From: Thomas Gleixner @ 2021-06-18 21:06 UTC (permalink / raw) To: Nicolas Saenz Julienne, linux-rt-users, frederic Cc: mtosatti, Nicolas Saenz Julienne, LKML 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <20210610125945.558872-2-nsaenzju@redhat.com>]
* Re: [RFC 2/2] timers: Make sure irq_work is handled when no pending timers [not found] ` <20210610125945.558872-2-nsaenzju@redhat.com> @ 2021-06-18 22:47 ` Thomas Gleixner 2021-06-19 8:22 ` Thomas Gleixner 2021-06-22 13:44 ` Peter Zijlstra 0 siblings, 2 replies; 6+ messages in thread From: Thomas Gleixner @ 2021-06-18 22:47 UTC (permalink / raw) To: Nicolas Saenz Julienne Cc: linux-rt-users, frederic, mtosatti, LKML, Peter Zijlstra 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC 2/2] timers: Make sure irq_work is handled when no pending timers 2021-06-18 22:47 ` [RFC 2/2] timers: Make sure irq_work is handled when no pending timers Thomas Gleixner @ 2021-06-19 8:22 ` Thomas Gleixner 2021-06-22 13:44 ` Peter Zijlstra 1 sibling, 0 replies; 6+ messages in thread From: Thomas Gleixner @ 2021-06-19 8:22 UTC (permalink / raw) To: Nicolas Saenz Julienne Cc: linux-rt-users, frederic, mtosatti, LKML, Peter Zijlstra 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC 2/2] timers: Make sure irq_work is handled when no pending timers 2021-06-18 22:47 ` [RFC 2/2] timers: Make sure irq_work is handled when no pending timers Thomas Gleixner 2021-06-19 8:22 ` Thomas Gleixner @ 2021-06-22 13:44 ` Peter Zijlstra 2021-06-22 17:33 ` Jiri Olsa 1 sibling, 1 reply; 6+ messages in thread From: Peter Zijlstra @ 2021-06-22 13:44 UTC (permalink / raw) To: Thomas Gleixner Cc: Nicolas Saenz Julienne, linux-rt-users, frederic, mtosatti, LKML, Jiri Olsa 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC 2/2] timers: Make sure irq_work is handled when no pending timers 2021-06-22 13:44 ` Peter Zijlstra @ 2021-06-22 17:33 ` Jiri Olsa 2021-06-22 17:42 ` Jiri Olsa 0 siblings, 1 reply; 6+ messages in thread From: Jiri Olsa @ 2021-06-22 17:33 UTC (permalink / raw) To: Peter Zijlstra Cc: Thomas Gleixner, Nicolas Saenz Julienne, linux-rt-users, frederic, mtosatti, LKML, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC 2/2] timers: Make sure irq_work is handled when no pending timers 2021-06-22 17:33 ` Jiri Olsa @ 2021-06-22 17:42 ` Jiri Olsa 0 siblings, 0 replies; 6+ messages in thread From: Jiri Olsa @ 2021-06-22 17:42 UTC (permalink / raw) To: Peter Zijlstra Cc: Thomas Gleixner, Nicolas Saenz Julienne, linux-rt-users, frederic, mtosatti, LKML, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-22 17:43 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20210610125945.558872-1-nsaenzju@redhat.com> 2021-06-18 21:06 ` [RFC 1/2] timers: Add pending timer bool in struct timer_base Thomas Gleixner [not found] ` <20210610125945.558872-2-nsaenzju@redhat.com> 2021-06-18 22:47 ` [RFC 2/2] timers: Make sure irq_work is handled when no pending timers Thomas Gleixner 2021-06-19 8:22 ` Thomas Gleixner 2021-06-22 13:44 ` Peter Zijlstra 2021-06-22 17:33 ` Jiri Olsa 2021-06-22 17:42 ` Jiri Olsa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).