xen/intr: move interrupt balancing into xen_intr_alloc_isrc()

[ehem_freebsd_m5p.com]

Summary

While not as close to the core function of interrupt allocation, the
functions to balance are not architecture-independent. As a result
moving this into xen_intr_alloc_isrc() makes sense. This also means
passing the port since that is required for assigning the processor.

[ehem_freebsd_m5p.com]

Update to reflect the reordering of D31995. D30935 requires this to be a return.

[ehem_freebsd_m5p.com]

When looking elsewhere an alternative approach came to mind. This removes the need to pass the event channel port to xen_intr_alloc_isrc(), but still moves the intr_next_cpu(0) call into xen_intr_alloc_isrc(). As that is the real need (remove x86 bits from xen_intr.c) this might be better.

What really strikes me is how xen_intr_assign_cpu()'s interface seems distinctly odd. There is another case where ->xi_cpu = 0 is followed by xen_intr_assign_cpu(, old_cpu_value).

sys/xen/xen_intr.c

323

Upon comparing with other places, it is possible to merely do isrc->xi_cpu = intr_next_cpu(0); here. At which point the section in xen_intr_bind_isrc() can do the xen_intr_assign_cpu() call.

[ehem_freebsd_m5p.com]

Broken apart in my local repository. I believe under "Commits" the intermediate point I was thinking of should be visible. This might be a good idea.

[ehem_freebsd_m5p.com]

Breaking D30935 apart. Upon examination, this works as an alternative strategy. As I've looked at this, using merely this half has seemed more and more compelling. In fact this approach meshes very well with D32866/D31188.

[royger]

sys/xen/xen_intr.c

430

newline, and cpu can likely be unsigned.

431

Why not make the call to intr_next_cpu here instead of stashing the value in xi_cpu and carrying it over from xen_intr_alloc_isrc?

[ehem_freebsd_m5p.com]

Sigh, simple goof fix.

[ehem_freebsd_m5p.com]

sys/xen/xen_intr.c

430

<insert cursing at the stupid goof> Yeah, that should be.

431

If you look at diff #3, you'll see I was originally doing that. This approach though has two advantages:

1. This avoids the requirement to pass the event channel port to xen_intr_alloc_isrc()
2. This allows the cpu to be assigned here for all event channels, rather than doing assignments for VIRQ channels in xen_intr_bind_virq() and IPIs in xen_intr_bind_ipi() (see D32866).

[ehem_freebsd_m5p.com]

sys/xen/xen_intr.c

431

Hmm, rereading I'm unsure I answered the right question. The previous is my reasoning for keeping the xen_intr_assign_cpu() call here, while moving the intr_next_cpu() call. The reason for moving the intr_next_cpu() call is in the main summary. Mainly intr_next_cpu() is x86-only. This isn't really an allocation task, but since xen_intr_alloc_isrc() is the portion being broken off, moving it there seems reasonable.

[royger]

sys/xen/xen_intr.c

431

Could we maybe have something like xen_arch_intr_next_cpu() or similar, that is a per-arch function? On x86 it would be a wrapper around intr_next_cpu(), on Arm I'm not sure.

[ehem_freebsd_m5p.com]

sys/xen/xen_intr.c

431

Certainly. The function for ARM is intr_irq_next_cpu() and matching the interface you're proposing would be simple to implement.

I dislike an entire extra function call into the architecture side merely for setting one variable. Particularly when xen_arch_intr_alloc() can readily handle this.

On the flip side, this would remove an access to a core variable by the architecture side.

Presently I still favor setting isrc->xi_cpu, but almost any interface is possible.

[ehem_freebsd_m5p.com]

sys/xen/xen_intr.c

431

(see D31690 for the present ARM64 implementation)

[royger]

sys/xen/xen_intr.c

431

If you have xen intr arch specific headers you could place this as a static inline function, in any case I'm not specially fuzzed by having an extra function call, at the end of day this is not a hot path. I think the extra call is worth having in order to avoid the dance with setting xi_cpu in one function just so that another one can figure out which CPU the interrupt should be bound to.

[ehem_freebsd_m5p.com]

Updating due to fallout from D30743 (which I expect to go in first).

[ehem_freebsd_m5p.com]

Overall this approach seems likely to be simpler in the end. Notably if the approach proposed by D31188/D32866 was brought in, isrc->xi_cpu would always be set and the CPU assignment would be done here in all cases.

I've been trying to get feedback on D31188/D32866 for a while, since that is a major design decision with all sorts of effects.

sys/xen/xen_intr.c

431

I'm not that bothered by the idea of having a xen_arch_intr_next_cpu() function. Including processor balancing as part of xen_arch_intr_alloc() though appears much more flexible. Requiring an extra function in a header means more maintenance than simply including it as part of setup.

It could be reasonable for an architecture to only set the vCPU on newly allocated sources, but leave it completely alone on reused sources. This would leave sources as balanced as they already are, as long as newly allocated sources are reasonably balanced. The interface you're proposing wouldn't allow this.

[ehem_freebsd_m5p.com]

Updating to fix potential, unused, but set warning.