Messages in this thread Patch in this message | | | From | Thomas Gleixner <> | Subject | Re: [PATCH] x86/vector: Allow to free vector for managed IRQ | Date | Fri, 13 Mar 2020 15:24:08 +0100 |
| |
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 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.
Ming, I really have to ask why these two situations were not tested before the final submission of that isolation patch. Both issues have been discussed during review of the different versions. So the warning should have triggered back then already....
> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c > index 2c5676b0a6e7..a1142260b123 100644 > --- a/arch/x86/kernel/apic/vector.c > +++ b/arch/x86/kernel/apic/vector.c > @@ -837,14 +837,6 @@ static void free_moved_vector(struct apic_chip_data *apicd) > unsigned int cpu = apicd->prev_cpu; > bool managed = apicd->is_managed; > > - /* > - * This should never happen. Managed interrupts are not > - * migrated except on CPU down, which does not involve the > - * cleanup vector. But try to keep the accounting correct > - * nevertheless. > - */
While the comment is not longer correct, removing it is lame. This should have an explanation why managed interrupts can end up here.
No need to resend. I fixed it up already.
Thanks,
tglx
| |