xen/intr: rework locking, prepare xen_intr_alloc_isrc() for split

[ehem_freebsd_m5p.com]

Summary

There are actually several distinct locking domains in xen_intr.c, but
all were sharing the same lock. Both xen_intr_port_to_isrc[] and the
x86 interrupt structures needed protection. Split these two apart as a
precursor to splitting the architecture portions off the file.

The call structures around xen_intr_alloc_isrc() were rather perverse.
Notably the allocation function should be taking care of locking for
interrupt allocation needs and should also be handling the reuse of
unused interrupts.

[ehem_freebsd_m5p.com]

Add more to comment before xen_intr_isrc_lock. Give explicit condition for unlocking.

[ehem_freebsd_m5p.com]

Small, but significant optimization. The lock doesn't actually need to be held during release, instead things simply need to be clear before it is fully released.

[ehem_freebsd_m5p.com]

Update due to testing failure, apparently there is no non-sized atomic store-release.

[ehem_freebsd_m5p.com]

Small formatting cleanup. Other changes makes removing a line now seem cleaner (no functional change from last version).

[ehem_freebsd_m5p.com]

Update diff to keep synchronized with other diffs. This series has a fair bit of overlap and thus reordering is non-trivial.

[royger]

I'm unsure you actually need to split those apart. Wouldn't Arm also require some kind of locking of the architecture specific bits?

sys/x86/xen/xen_intr.c

640

table? xen-x86-irq-lock it's maybe better?

[ehem_freebsd_m5p.com]

In D30726#742521, @royger wrote:

I'm unsure you actually need to split those apart. Wouldn't Arm also require some kind of locking of the architecture specific bits?

See D30936. The goal is everything being protected by xen_intr_x86_lock is very x86-specific and destined to split away from this file. Whereas the portions which remain protected by xen_intr_isrc_lock are mostly architecture-independent and will remain in this file. Splitting the locks allows them to be static.

The initial ARM64 implementation has a lock. event_lock in sys/kern/kern_intr.c, no others are required. I suspect the ARM64 implementation is going to be replaced with one fully based on INTRNG, at which point it may well require a lock for reusing interrupts (similar to the x86 interrupt code, INTRNG's interrupt release functionality is problematic).

sys/x86/xen/xen_intr.c

640

I was thinking of it being the lock for the x86 interrupt table. One can also think of it handling the Xen section of interrupt_sources[], so seemed like a table lock.

No objection with renaming "xen-x86-irq-lock". This seems like an adjust when it gets into freebsd-src.

[royger]

In D30726#742538, @ehem_freebsd_m5p.com wrote:

In D30726#742521, @royger wrote:

I'm unsure you actually need to split those apart. Wouldn't Arm also require some kind of locking of the architecture specific bits?

See D30936. The goal is everything being protected by xen_intr_x86_lock is very x86-specific and destined to split away from this file. Whereas the portions which remain protected by xen_intr_isrc_lock are mostly architecture-independent and will remain in this file. Splitting the locks allows them to be static.

Would it be possible to just note that the _arch specific functions require external locking by the callers? (and protect them using the common lock) It might be simpler to just keep a single lock, and I don't think there are any performance benefits or otherwise from using two different locks.

[ehem_freebsd_m5p.com]

In D30726#742559, @royger wrote:

In D30726#742538, @ehem_freebsd_m5p.com wrote:

See D30936. The goal is everything being protected by xen_intr_x86_lock is very x86-specific and destined to split away from this file. Whereas the portions which remain protected by xen_intr_isrc_lock are mostly architecture-independent and will remain in this file. Splitting the locks allows them to be static.

Would it be possible to just note that the _arch specific functions require external locking by the callers? (and protect them using the common lock) It might be simpler to just keep a single lock, and I don't think there are any performance benefits or otherwise from using two different locks.

This might not be impossible. At a minimum accomplishing this would be very difficult. Notably, if the core needs a new source, it doesn't know whether the architecture will reuse one versus allocating a new one. The decision as to whether to allocate a new one versus reusing a previously allocated one is an architecture decision. It is possible to implement an architecture side which always allocates (present ARM64 one), it is possible to implement an architecture side which sometimes allocates (present x86 one), or it might be possible to implement one which never allocated.

The result is the architecture side may need to call malloc(). Based on the current code, it doesn't seem M_NOWAIT is justified and thus the architecture code cannot use an external lock.

I doubt there are performance benefits from using two locks, just the resultant implementation will be of substantially lower quality.

[ehem_freebsd_m5p.com]

In D30726#742582, @ehem_freebsd_m5p.com wrote:

I doubt there are performance benefits from using two locks, just the resultant implementation will be of substantially lower quality.

I guess I should state, the main benefit of two locks is it allows one lock being local to sys/xen/xen_intr.c and one lock being local to sys/x86/xen/xen_arch_intr.c (which isn't done here).

Other issue is the current locking during allocation is really gnarly. Notice how before calling xen_intr_alloc_isrc() the lock must be acquired for access to the x86 allocation variables. Once inside xen_intr_alloc_isrc() after the x86 variables are modified (xen_intr_auto_vector_count is incremented); the lock is released in order to call malloc(); then the lock is reacquired for modifying xen_intr_port_to_isrc[].

I cannot dispute the current implementation functions, but it is really awful. This badly breaks many rules of locking. Whereas the new implementation both acquires and releases xen_intr_x86_lock inside xen_intr_alloc_isrc().

[mhorne]

In D30726#742593, @ehem_freebsd_m5p.com wrote:

Other issue is the current locking during allocation is really gnarly. Notice how before calling xen_intr_alloc_isrc() the lock must be acquired for access to the x86 allocation variables. Once inside xen_intr_alloc_isrc() after the x86 variables are modified (xen_intr_auto_vector_count is incremented); the lock is released in order to call malloc(); then the lock is reacquired for modifying xen_intr_port_to_isrc[].

You did not explain what is wrong with this pattern. Can something go wrong if the lock is acquired between these sections, or do you just not like it?

I cannot dispute the current implementation functions, but it is really awful. This badly breaks many rules of locking. Whereas the new implementation both acquires and releases xen_intr_x86_lock inside xen_intr_alloc_isrc().

I wish you would quantify at least one way in which it is awful before you call it that.

[ehem_freebsd_m5p.com]

In D30726#742595, @mhorne wrote:

In D30726#742593, @ehem_freebsd_m5p.com wrote:

Other issue is the current locking during allocation is really gnarly. Notice how before calling xen_intr_alloc_isrc() the lock must be acquired for access to the x86 allocation variables. Once inside xen_intr_alloc_isrc() after the x86 variables are modified (xen_intr_auto_vector_count is incremented); the lock is released in order to call malloc(); then the lock is reacquired for modifying xen_intr_port_to_isrc[].

You did not explain what is wrong with this pattern. Can something go wrong if the lock is acquired between these sections, or do you just not like it?

I cannot dispute the current implementation functions, but it is really awful. This badly breaks many rules of locking. Whereas the new implementation both acquires and releases xen_intr_x86_lock inside xen_intr_alloc_isrc().

I wish you would quantify at least one way in which it is awful before you call it that.

I have no doubt that it in fact works. Issue is rather than entering and leaving xen_intr_alloc_isrc() with locking in similar states, it is entering in one locking state and leaving in a very different locking state. Another way to illustrate this is how the hunk in xen_intr_bind_isrc() has to change if you were to only split the lock:

 		return (EINVAL);
 	}
 
-	mtx_lock(&xen_intr_isrc_lock);
+	mtx_lock(&xen_intr_x86_lock);
 	isrc = xen_intr_find_unused_isrc(type);
 	if (isrc == NULL) {
 		isrc = xen_intr_alloc_isrc(type);
 		if (isrc == NULL) {
-			mtx_unlock(&xen_intr_isrc_lock);
+			mtx_unlock(&xen_intr_x86_lock);
 			return (ENOSPC);
 		}
+	} else {
+		mtx_unlock(&xen_intr_x86_lock);
+		mtx_lock(&xen_intr_isrc_lock);
 	}
 	isrc->xi_port = local_port;
 	xen_intr_port_to_isrc[local_port] = isrc;

xen_intr_alloc_isrc() is being entered with one locking domain held, and will leave with one of two distinct locking domains held. Having the two locking areas use the same lock means the else can disappear with no ill effects, aside from analyzing the code being difficult.

After this xen_intr_isrc_lock still covers 3 locking domains:

1. Protecting xen_intr_port_to_isrc[].
2. Protecting port/cpu masking operations.
3. Protecting ->xi_refcount.

Since there aren't any plans to split those functions off the file, they were left as part of the xen_intr_isrc_lock. Though now that I've looked up that list I have a funny feeling I'm going to be asked to update the comment added before xen_intr_isrc_lock.

[ehem_freebsd_m5p.com]

Adjusting the comment before xen_intr_isrc_lock as per my comment.

I'm unsure about #2 though. Most port/cpu masking operations are inside locks, but those are generally grouped with modifications to xen_intr_port_to_isrc[]. Possibly it is strictly other operations surrounding those bits which require the lock. Those (un)masking operations are mostly done with atomics.

[ehem_freebsd_m5p.com]

Putting another way, there are 4 distinct locking domains:

1. interrupt_sources[]/x86 interrupts available for reuse
2. xen_intr_auto_vector_count/newly allocating x86 interrupts
3. xen_intr_port_to_isrc[]/isrc->xi_port
4. Acquire/release isrc->xi_refcount

Of these #1 and #2 are very closely related. #3 is loosely related to the others. #4 is in the same file, but almost unrelated. Now look at which domains are used where.

Before this, the mtx_lock() on line 410 is locking domain #1. Locking domain #1 is used through line 411, when xen_intr_find_unused_isrc() returns, locking domain #1 is no longer in use. Locking domain #2 is implicitly locked at line 412 and is used by the first half of xen_intr_alloc_isrc(), but is explicitly unlocked at lines 327 (inside xen_intr_alloc_isrc()) and 415. Locking domain #3 is implicitly locked at line 412 if xen_intr_find_unused_isrc() is successful, otherwise it is explicitly locked at line 333 (inside xen_intr_alloc_isrc()).

After this, the mtx_lock() on line 330 is for locking domain #1. Locking domain #1 is explicitly unlocked at line 334 or implicitly unlocked at line 337. Locking domain #2 is implicitly locked at line 337 and is then explicitly unlocked at lines 343 and 353. Locking domain #3 is explicitly locked at line 448.

The big issue is originally, the locking and unlocking of domains #2 and #3 are widely separated. The locking of domain #2 is in xen_intr_bind_isrc(), but is unlocked in both xen_intr_bind_isrc() and xen_intr_alloc_isrc(). The locking of domain #3 is in both xen_intr_bind_isrc() and xen_intr_alloc_isrc(), but the unlocking is in xen_intr_alloc_isrc().

With the replacement, the locking and unlocking of domains #1 and #2 are both strictly inside xen_intr_alloc_isrc(). The locking and unlocking of domain #3 is in xen_intr_bind_isrc().

[royger]

Forgot to hit summit yesterday...

So AFAICT you only need the newly introduced lock to protect accesses to xen_intr_port_to_isrc/xen_intr_auto_vector_count, which Arm won't be using, ie: xen_intr_alloc_isrc and xen_intr_release_isrc. Those will become arch specific functions, is that all?

sys/x86/xen/xen_intr.c

405

I'm not a huge fan of doing this kind of tricks, as such open coded locking primitives often turn out to make the whole thing more fragile in the long term. Isn't there a way you could also hold xen_intr_x86_lock here, which is the one used by xen_intr_find_unused_isrc?

[ehem_freebsd_m5p.com]

I think you've got the gist of it, but the borderline isn't quite where you're placing it and there is a bit more before breaking off.

xen_intr_port_to_isrc[] remains with xen_intr_isrc_lock, but xen_intr_auto_vector_count goes with the new lock to x86-only.

D30935 moves the interrupt balancing into xen_intr_alloc_isrc() since the interrupt balancing functions are not architecture-independent; but then the present xen_intr_alloc_isrc() turns into xen_arch_intr_alloc() and goes to the x86-only side.

Most of the current xen_intr_release_isrc() remains xen_intr_release_isrc(), only lines 394-405 (old 366-370) turn into xen_arch_intr_release().

Full details are in D30936 where the deed is done.

sys/x86/xen/xen_intr.c

405

Yes, I was wondering about this. Problem is if you leave me waiting with time on my hands, sometimes code gets even more optimized...

Can indeed simply be lock, isrc->xi_type = EVTCHN_TYPE_UNBOUND;, unlock. I've been wondering about whether it is worth clearing isrc here. The x86 architecture side only looks at ->xi_type and the other values should be cleared during allocation.

[ehem_freebsd_m5p.com]

Trying to keep Phabricator synchronized locally. Bit of a problem with the number of patches I'm playing with...

[royger]

Thanks for the update, I'm going over the dependencies to check how this all fits together.

[ehem_freebsd_m5p.com]

D30726 doesn't depend on anything. As originally submitted D31995 wants D30726 first. The big dependency is D30936 depends on D30726.

[ehem_freebsd_m5p.com]

Upon examination, it was possible to split this into 3 separate commits somewhat reasonably. I'm doubtful the commits are worthy of individual review, so I'm leaving Phabricator alone.

[ehem_freebsd_m5p.com]

Tiny delta, removes one extra line from the lock/unlock in xen_intr_bind_isrc(). (also keeps Phabricator synchronized)

[ehem_freebsd_m5p.com]

Tiny update, pulling an extra line out of the locked section.