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

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).

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.

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.

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().

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.

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.

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().

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?

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.

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

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

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.