Re: [PATCH] x86/vector: Allow to free vector for managed IRQ

[Thomas Gleixner]

Peter Xu <peterx@xxxxxxxxxx> writes:
> On Fri, Mar 13, 2020 at 03:24:08PM +0100, Thomas Gleixner wrote:
>> Peter Xu <peterx@xxxxxxxxxx> writes:
>> What is this backtrace for? It's completly useless as it merily shows
>> that the warning triggers. Also even if it'd be useful then it wants to
>> be trimmed properly.
>
> I thought it was a good habit to keep the facts of issues.  Backtrace
> is one of them so I kept them.  It could, for example, help people who
> spot the same issue in an old/downstream kernel so when they google or
> grepping git-log they know the exact issue has been solved by some
> commit, even without much knowledge on the internals (because they can
> exactly compare the whole dmesg error).

This is not really good habit. Changelogs should contain factual
information which is relevant to understand the problem. Backtraces are
useful when the callchain leading to a bug/warn/oops _is_ relevant for
understanding the issue. For things like this where the backtrace is
completly out of context it's more of an distraction than of value.

Aside of that your 'spot the same issue' argument is wrong in this
context as this does not affect anything old/downstream. The feature was
merged during the 5.6 merge window. So it has not reached anything
downstream which needs backports. If distro people pick out such a
feature and backport it to their frankenkernels before the final release
then I really dont care.

One way to preserve the backtrace for google's sake is to write a cover
letter and stick the back trace there if it is not useful in the
changelog.

Also having 40+ lines of untrimmed backtrace is just a horrible
distraction. The timestamps are completely useless and depending on the
type of problem most of the other gunk which is emitted by a
bug/warn/oops backtrace is useless as well.

Why would you be interested in the register values for this problem or
the code or the interrupt flags tracer state? That thing triggered a
warning and nothing in the backtrace gives any clue about why that
happened, IOW it's pure distraction.

If the call chain is relevant to explain the problem, then a trimmed
down version is really sufficient. Here is an example:

     BUG: unable to handle kernel NULL pointer dereference at 0000000000000078
     RIP: 0010:apic_ack_edge+0x1e/0x40
     Call Trace:
       handle_edge_irq+0x7d/0x1e0
       generic_handle_irq+0x27/0x30
       aer_inject_write+0x53a/0x720

This helps, because it illustrates exactly how this BUG was triggered
and it's reduced to exactly the 6 lines because everything else in the
original 50+ lines is useless.

> However I think I still miss one thing in the puzzle (although it
> turns out that we've agreed on removing the warning already, but just
> in case I missed something important) - do you mean that offlining all
> the non-isolated CPUs in the mask won't trigger this already?  Because
> I also saw some similar comment somewhere else...
>
> Here's my understanding - when offlining, we'll disable the CPU and
> reach:
>
>   - irq_migrate_all_off_this_cpu
>     - migrate_one_irq
>       - irq_do_set_affinity
>         - calculate HK_FLAG_MANAGED_IRQ and so on...
>
> Then we can still trigger this irq move event even before we bring
> another housekeeping cpu online, right?  Or could you guide me on what
> I have missed?

The migration when offlining the CPU to which an interrupt is affine
does not trigger this because that uses a different mechanism.

It clears the vector on the outgoing CPU brute force simply because this
CPU can't handle any interrupts anymore. There is some logic to catch
the device interrupt racing against this, but while careful it's not
perfect. There is a theoretical hole there which probably could be
triggered by carefully orchestrating things, but we can't do anything
about it except disabling CPU unplug :)

So the only two ways to trigger this are the ones I described in the
changelog.

Hope that helps.

Thanks,

        tglx