Re: [PATCH v5 19/19] irqdomain: Switch to per-domain locking

On Fri, Feb 10, 2023 at 11:38:58AM +0000, Marc Zyngier wrote:

On Fri, 10 Feb 2023 09:56:03 +0000,
Johan Hovold [off-list ref] wrote:

quoted

On Thu, Feb 09, 2023 at 04:00:55PM +0000, Marc Zyngier wrote:

quoted

On Thu, 09 Feb 2023 13:23:23 +0000,
Johan Hovold [off-list ref] wrote:

quoted

I went back and forth over that a bit, but decided to only use
domain->root->mutex in paths that can be called for hierarchical
domains (i.e. the "shared code paths" mentioned above).

Using it in paths that are clearly only called for non-hierarchical
domains where domain->root == domain felt a bit lazy.

My concern here is that as this code gets further refactored, it may
become much harder to reason about what is the correct level of
locking.

Yeah, that's conceivable.

quoted

The counter argument is of course that using domain->root->lock allows
people to think less about the code they are changing, but that's not
necessarily always a good thing.

Eventually, non-hierarchical domains should simply die and be replaced
with a single level hierarchy. Having a unified locking in place will
definitely make the required work clearer.

quoted

Also note that the lockdep asserts in the revmap helpers would catch
anyone using domain->mutex where they should not (i.e. using
domain->mutex for an hierarchical domain).

Lockdep is great, but lockdep is a runtime thing. It doesn't help
reasoning about what gets locked when changing this code.

Contributers are expected to test their changes with lockdep enabled,
right?

But sure, using root->domain->mutex throughout may prevent prevent
people from getting this wrong.

I'll update this for v6.
 

quoted

quoted

quoted

@@ -1132,6 +1147,7 @@ struct irq_domain *irq_domain_create_hierarchy(struct irq_domain *parent,
 	else
 		domain = irq_domain_create_tree(fwnode, ops, host_data);
 	if (domain) {
+		domain->root = parent->root;
 		domain->parent = parent;
 		domain->flags |= flags;

So we still have a bug here, as we have published a domain that we
keep updating. A parallel probing could find it in the interval and do
something completely wrong.

Indeed we do, even if device links should make this harder to hit these
days.

quoted

Splitting the work would help, as per the following patch.

Looks good to me. Do you want to submit that as a patch that I'll rebase
on or should I submit it as part of a v6?

Just take it directly.

Ok, thanks.

I guess this turns the "Use irq_domain_create_hierarchy()" patches into
fixes that should be backported as well.

But note that your proposed diff may not be sufficient to prevent
lookups from racing with domain registration generally. Many drivers
still update the bus token after the domain has been added (and
apparently some still set flags also after creating hierarchies I just
noticed, e.g. amd_iommu_create_irq_domain).

It seems we'd need to expose a separate allocation and registration
interface, or at least pass in the bus token to a new combined
interface.

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel