Re: [patch 16/18] x86/apic: Convert 32bit to IPI shorthand static key

[Thomas Gleixner]

On Wed, 3 Jul 2019, Nadav Amit wrote:
> > On Jul 3, 2019, at 1:34 PM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> > On Wed, 3 Jul 2019, Nadav Amit wrote:
> >>> On Jul 3, 2019, at 3:54 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> >>> void default_send_IPI_all(int vector)
> >>> {
> >>> -	if (apic_ipi_shorthand_off || vector == NMI_VECTOR) {
> >>> +	if (static_branch_likely(&apic_use_ipi_shorthand)) {
> >>> 		apic->send_IPI_mask(cpu_online_mask, vector);
> >>> 	} else {
> >>> 		__default_send_IPI_shortcut(APIC_DEST_ALLINC, vector);
> >> 
> >> It may be better to check the static-key in native_send_call_func_ipi() (and
> >> other callers if there are any), and remove all the other checks in
> >> default_send_IPI_all(), x2apic_send_IPI_mask_allbutself(), etc.
> > 
> > That makes sense. Should have thought about that myself, but hunting that
> > APIC emulation issue was affecting my brain obviously :)
> 
> Well, if you used VMware and not KVM... ;-)

Then I would have hunted some other bug probably :)
 
> >> void native_send_call_func_ipi(const struct cpumask *mask)
> >> {
> >> -	cpumask_var_t allbutself;
> >> -
> >> -	if (!alloc_cpumask_var(&allbutself, GFP_ATOMIC)) {
> >> -		apic->send_IPI_mask(mask, CALL_FUNCTION_VECTOR);
> >> -		return;
> >> +	int cpu, this_cpu = smp_processor_id();
> >> +	bool allbutself = true;
> >> +	bool self = false;
> >> +
> >> +	for_each_cpu_and_not(cpu, cpu_online_mask, mask) {
> >> +
> >> +		if (cpu != this_cpu) {
> >> +			allbutself = false;
> >> +			break;
> >> +		}
> >> +		self = true;
> > 
> > That accumulates to a large iteration in the worst case. 
> 
> I donât understand why. There should be at most two iterations - one for
> self and one for another core. So _find_next_bit() will be called at most
> twice. _find_next_bit() has its own loop, but I donât think overall it is as

Indeed, misread the code and right the bit search should be fast.

> bad as calling alloc_cpumask_var(), cpumask_copy() and cpumask_equal(),
> which also have loops.
>
> I donât have numbers (and I doubt they are very significant), but the cpumask
> allocation showed when I was profiling my microbenchmark.

Yes, that alloc/free part is completely bogus.

Thanks,

	tglx