Re: Fix quilt merge error in acpi-cpufreq.c

Rusty Russell
2009-04-16T01:27:35Z

On Thu, 16 Apr 2009 12:58:45 am Linus Torvalds wrote:
>
> On Wed, 15 Apr 2009, Rusty Russell wrote:
> >
> > The API is screwy. It excludes the current CPU from the mask,
> > unconditionally. It's a tlb flush helper masquerading as a general function.
> >
> > (smp_call_function has the same issue).
> >
> > Something like this?
> >
> > Subject: smp_call_function_many: add explicit exclude_self flag
>
> No. This just makes the API even screwier. It fixes the
> "smp_processor_id()" thing, but it is
>
> (a) horribly buggy

Sure. Did it even compile?

> Those
>
> if (exclude_self && cpu == this_cpu)
> cpu = cpumask_next_and(cpu, mask, cpu_online_mask);
>
> things are wrong - we need to do that "jump over our own CPU" thing
> regardless of whether 'exclude_self' is set or not, since we're going
> to special-case our own CPU anyway.

I don't think so (smp_call_function_single will DTRT if
cpu == smp_processor_id). But I didn't test to be sure.

> (c) Wrong, even if it wasn't (horribly buggy)^2
>
> Adding "flags" to an interface doesn't make it better. Quite the
> reverse. It makes it worse.

Uglier. Worse? It would have prevented Andrew's mistake.

> It also makes it even MORE different from
> all the other smp_call_function's, which just do the 'self' cpu
> without any stupid conditionals and flags.

You've said this twice, but unfortunately that doesn't make it true.

smp_call_function() is the original from which this derives, and it has
always skipped the current cpu. Hence on_each_cpu().

I'd love to see a fix which isn't ugly and doesn't put a cpumask on the
stack.

> > Impact: clarify and extend confusing API
>
> And what the hell is up with these bogus "Impact:" things? Who started
> doing that, and why?

Ingo wants them. Example:

lguest: don't expect linear addresses in gdt pvops

Impact: fix guest crash 'lguest: bad read address 0x4800000 len 256'

What's more important in the subject line? That it fixes a crash, or what it
does?

Thanks,
Rusty.