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

[Peter Xu <peterx@redhat.com>]

On Fri, Mar 13, 2020 at 03:24:08PM +0100, Thomas Gleixner wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > After we introduced the "managed_irq" sub-parameter for isolcpus, it's
> > possible to free a kernel managed irq vector now.
> >
> > It can be triggered easily by booting a VM with a few vcpus, with one
> > virtio-blk device and then mark some cores as HK_FLAG_MANAGED_IRQ (in
> > below case, there're 4 vcpus, with vcpu 3 isolated with managed_irq):
> >
> > [    2.889911] ------------[ cut here ]------------
> > [    2.889964] WARNING: CPU: 3 PID: 0 at arch/x86/kernel/apic/vector.c:853 free_moved_vector+0x126/0x160
> 
> <SNIP>
> 
> > [    2.890026] softirqs last disabled at (8757): [<ffffffffbb0ecccd>] irq_enter+0x4d/0x70
> > [    2.890027] ---[ end trace deb5d563d2acb13f ]---
> 
> 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).

> 
> > I believe the same thing will happen to bare metals.
> 
> Believe is not really relevant in engineering.
> 
> The problem has nothing to do with virt or bare metal. It's a genuine
> issue.
> 
> > When allocating the IRQ for the device, activate_managed() will try to
> > allocate a vector based on what we've calculated for kernel managed
> > IRQs (which does not take HK_FLAG_MANAGED_IRQ into account).  However
> > when we bind the IRQ to the IRQ handler, we'll do irq_startup() and
> > irq_do_set_affinity(), in which we will start to consider the whole
> > HK_FLAG_MANAGED_IRQ logic.  This means the chosen core can be
> > different from when we do the allocation.  When that happens, we'll
> > need to be able to properly free the old vector on the old core.
> 
> There's lots of 'we' in that text. We do nothing really. Please describe
> things in neutral and factual language.
> 
> Also there is another way to trigger this: Offline all non-isolated CPUs
> in the mask and then bring one online again.

Thanks for your suggestions on not using subjective words and so on.
I'll remember these.

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?

Thanks,

-- 
Peter Xu