Re: [PATCH v2 1/3] genirq/affinity: Add irq_update_affinity_desc()

From: John Garry <john.garry@huawei.com>
To: Marc Zyngier <maz@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	<gregkh@linuxfoundation.org>, <rafael@kernel.org>,
	<martin.petersen@oracle.com>, <jejb@linux.ibm.com>,
	<linuxarm@huawei.com>, <linux-scsi@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/3] genirq/affinity: Add irq_update_affinity_desc()
Date: Tue, 24 Nov 2020 17:38:10 +0000	[thread overview]
Message-ID: <702e1729-9a4b-b16f-6a58-33172b1a3220@huawei.com> (raw)
In-Reply-To: <0525a4bcf17a355cd141632d4f3714be@kernel.org>

Hi Marc,

>> So initially in the msi_prepare method we setup the its dev - this is
>> from the mbigen probe. Then when all the irqs are unmapped later for
>> end device driver removal, we release this its device in
>> its_irq_domain_free(). But I don't see anything to set it up again. Is
>> it improper to have released the its device in this scenario?
>> Commenting out the release makes things "good" again.
> 
> Huh, that's ugly. The issue is that the device that deals with the
> interrupts isn't the device that the ITS knows about (there isn't a
> 1:1 mapping between mbigen and the endpoint).
> 
> The mbigen is responsible for the creation of the corresponding
> irqdomain, and and crucially for the "prepare" phase, which results
> in storing the its_dev pointer in info->scratchpad[0].
> 
> As we free all the interrupts associated with the endpoint, we
> free the its_dev (nothing else needs it at this point). On the
> next allocation, we reuse the damn its_dev pointer, and we're SOL.
> This is wrong, because we haven't removed the mbigen, only the
> device *connected* to the mbigen. And since the mbigen can be shared
> across endpoints, we can't reliably tear it down at all. Boo.
> 
> The only thing to do is to convey that by marking the its_dev as
> shared so that it isn't deleted when no LPIs are being used. After
> all, it isn't like the mbigen is going anywhere.

Right, I did consider this.

> 
> It is just that passing that information down isn't a simple affair,
> as msi_alloc_info_t isn't a generic type... Let me have a think.

I think that there is a way to circumvent the problem, which you might 
call hacky, but OTOH, not sure if there's much point changing mbigen or 
related infrastructure at this stage.

Anyway, so we have 128 irqs in total for the mbigen domain, but the 
driver only is interesting in something like irq indexes 1,2,72-81, and 
96-112. So we can just dispose the mappings for irq index 0-112 at 
removal stage, thereby keeping the its device around. We do still call 
platform_irq_count(), which sets up all 128 mappings, so maybe we should 
be unmapping all of these - this would be the contentious part. But 
maybe not, as the device driver is only interested in that subset, and 
has no business unmapping the rest.

With that change, the platform.c API would work a bit more like the pci 
msi code equivalent, where we request a min and max number of vectors. 
In fact, that platform.c change needs to be made anyway as 
platform_get_irqs_affinity() is broken currently for when nr_cpus < #hw 
queues.

Thoughts?

Thanks,
John