Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Nicolas Saenz Julienne <nsaenzju@redhat.com>
Cc:     linux-rt-users@vger.kernel.org, frederic@kernel.org,
        mtosatti@redhat.com, LKML <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>
Subject: Re: [RFC 2/2] timers: Make sure irq_work is handled when no pending timers
In-Reply-To: <20210610125945.558872-2-nsaenzju@redhat.com>
References: <20210610125945.558872-1-nsaenzju@redhat.com> <20210610125945.558872-2-nsaenzju@redhat.com>
Date:   Sat, 19 Jun 2021 00:47:04 +0200
Message-ID: <87mtrmeqon.ffs@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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.