Fengguang Wu wrote:
quoted text
> On Thu, Sep 27, 2007 at 02:22:20AM -0700, Andrew Morton wrote:
>> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-r=
c8/2.6.23-rc8-mm2/
quoted text
> =20
> Laurent,
>=20
> It triggered a WARNING on first run in qemu:

Thank you to report it.

quoted text
>=20
> [    0.310000] WARNING: at arch/x86_64/kernel/smp.c:397 smp_call_functi=
on_mask()
quoted text
> [    0.310000]
> [    0.310000] Call Trace:
> [    0.310000]  [<ffffffff8100dbde>] dump_trace+0x3ee/0x4a0
> [    0.310000]  [<ffffffff8100dcd3>] show_trace+0x43/0x70
> [    0.310000]  [<ffffffff8100dd15>] dump_stack+0x15/0x20
> [    0.310000]  [<ffffffff8101cd44>] smp_call_function_mask+0x94/0xa0
> [    0.310000]  [<ffffffff8101cd69>] smp_call_function+0x19/0x20
> [    0.310000]  [<ffffffff8104277f>] on_each_cpu+0x1f/0x50
> [    0.310000]  [<ffffffff81026eac>] global_flush_tlb+0x8c/0x110
> [    0.310000]  [<ffffffff81025c85>] free_init_pages+0xe5/0xf0
> [    0.310000]  [<ffffffff81549b5e>] alternative_instructions+0x7e/0x15=
0
quoted text
> [    0.310000]  [<ffffffff8154a2ea>] check_bugs+0x1a/0x20
> [    0.310000]  [<ffffffff81540c4a>] start_kernel+0x2da/0x380
> [    0.310000]  [<ffffffff81540132>] _sinittext+0x132/0x140


the reason is the WARN_ON():

390 int smp_call_function_mask(cpumask_t mask,
391                            void (*func)(void *), void *info,
392                            int wait)
393 {
394         int ret;
395
396         /* Can deadlock when called with interrupts disabled */
397         WARN_ON(irqs_disabled());
398
399         spin_lock(&call_lock);
400         ret =3D __smp_call_function_mask(mask, func, info, wait);
401         spin_unlock(&call_lock);
402         return ret;
403 }

The patch I sent to Andi didn't include this WARN_ON() and it's why I did=
n't
find this issue. (see http://lkml.org/lkml/2007/8/24/101)

smp_call_function_mask() is called by smp_call_function() which calls a f=
unction
on all CPU except current.
The comment of smp_call_function() specifies:
=2E..
 * You must not call this function with disabled interrupts or from a
 * hardware interrupt handler or from a bottom half handler.
 * Actually there are a few legal cases, like panic.
 */

So this WARN_ON() is correct, and the caller (global_flush_tlb()) doesn't=
 follow
this rule.

I guess this WARN_ON() is only needed when we have current CPU in provide=
d mask.
So I think we should change:

int smp_call_function (void (*func) (void *info), void *info, int nonatom=
ic,
                        int wait)
{
        return smp_call_function_mask(cpu_online_map, func, info, wait);
}
("cpu_online_map" is a bad choice, comment also specifies: "run a functio=
n on
all other CPU")

to

int smp_call_function (void (*func) (void *info), void *info, int nonatom=
ic,
                        int wait)
{
        int ret;
	cpumask_t allbutself;

	allbutself =3D cpu_online_map;
	cpu_clear(smp_processor_id(), allbutself);

        spin_lock(&call_lock);
        ret =3D __smp_call_function_mask(allbutself, func, info, wait);
        spin_unlock(&call_lock);
        return ret;
}
(which is smp_call_function_mask() without the WARN_ON() and without curr=
ent cpu
in the mask)

Andi, is this correct ?
Andrew, should I send a patch implementing this change ?

Regards,
Laurent
--=20
------------- Laurent.Vivier@bull.net  --------------
          "Software is hard" - Donald Knuth