On Wed, 3 Jul 2019, Nadav Amit wrote:
> > On Jul 3, 2019, at 1:34 PM, Thomas Gleixner <[email protected]> wrote:
> > On Wed, 3 Jul 2019, Nadav Amit wrote:
> >>> On Jul 3, 2019, at 3:54 AM, Thomas Gleixner <[email protected]> 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
