On 5/10/24 12:06, Dongli Zhang wrote:
> } else {
> + /*
> + * This call borrows from the comments and implementation
> + * of apic_update_vector(): "If the target CPU is offline
> + * then the regular release mechanism via the cleanup
> + * vector is not possible and the vector can be immediately
> + * freed in the underlying matrix allocator.".
> + */
> + irq_matrix_free(vector_matrix, apicd->prev_cpu,
> + apicd->prev_vector, apicd->is_managed);
> apicd->prev_vector = 0;
> }
I know it's just two sites, but I'd much rather spend the space on a
helper function than a copy-and-pasted comment. Wouldn't something like
this make it like stupidly obvious what's going on:
if (cpu_online(cpu)) {
...
} else {
irq_matrix_free_offline(apicd->prev_cpu,
apicd->prev_vector,
apicd->is_managed);
apicd->prev_vector = 0;
}
/* Free a vector when the target CPU is offline */
static void irq_matrix_free_offline(...)
{
lockdep_assert_held(&vector_lock);
WARN_ON_ONCE(!cpu_offline(apicd->prev_cpu));
/*
* The regular release mechanism via the cleanup vector is not
* possible. Immediately free the vector in the underlying
* matrix allocator.
*/
irq_matrix_free(&whatever, cpu, vector, managed);
}
It would also be rather hard to screw up even if someone called it on an
online CPU because you'd get a nice warning.
