diff mbox

[1/1,v2] ARM: only call smp_send_stop() on SMP

Message ID 20120727214024.GA10249@mudshark.cambridge.arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Will Deacon July 27, 2012, 9:40 p.m. UTC
Hi Russell,

On Fri, Jul 27, 2012 at 10:06:37PM +0100, Russell King - ARM Linux wrote:
> On Fri, Jul 27, 2012 at 09:44:47PM +0100, Will Deacon wrote:
> > I did comment on this one:
> > 
> >   http://thread.gmane.org/gmane.linux.kernel/1303115
> > 
> > and I really think we should fix the cause of the problem, rather than
> > point patching this instance of it.
> 
> What do you think needs fixing there?

Well, we certainly need to fix the NULL dereference and the original patch
does do that. I just think it might be nicer to remove the possibility of a
NULL dereference instead.

> We support booting a kernel on systems with or without SMP support, even
> with a SMP kernel.  When the kernel is booted on such a system, it is
> undefined whether smp_cross_call() is a valid function pointer.

So let's define it to point at a dummy function which explodes with a BUG if
the cpumask passed in isn't empty. That allows SMP kernels to do things like
`cross call to all other cores' without having to worry about whether there
are any other cores or not.

> In any case, when we have only one CPU online in the system, it is
> pointless even calling smp_cross_call().

Pointless, but also error-prone and requiring explicit cpumask checks at
each call-site.

> That is why I explicitly suggested this solution.  This is the solution
> _I_ want, because it is the most sane solution all round.

Adding a dummy implementation is straightforward [ok, this is untested]:




If you still prefer checking at the call-site then the original patch will
certainly work. Otherwise, I'm happy to submit the above after some testing.

Will

Comments

Russell King - ARM Linux July 28, 2012, 10:08 a.m. UTC | #1
On Fri, Jul 27, 2012 at 10:40:24PM +0100, Will Deacon wrote:
> On Fri, Jul 27, 2012 at 10:06:37PM +0100, Russell King - ARM Linux wrote:
> > We support booting a kernel on systems with or without SMP support, even
> > with a SMP kernel.  When the kernel is booted on such a system, it is
> > undefined whether smp_cross_call() is a valid function pointer.
> 
> So let's define it to point at a dummy function which explodes with a BUG if
> the cpumask passed in isn't empty. That allows SMP kernels to do things like
> `cross call to all other cores' without having to worry about whether there
> are any other cores or not.

We should not be even attempting to do any cross calls when there aren't
any other CPUs in the system - that's rather the point of leaving it as
a NULL pointer so it does explode on such systems.

. the scheduler won't call smp_send_reschedule() when there are no other
  CPUs in the system.
. timer ticks won't be broadcast to other CPUs if there are no other CPUs
  in the system.
. function calls will not be issued to other CPUs if there are no other
  CPUs in the system.

There is only one case where this doesn't happen, and that's the shutdown
path.

For instance, smp_call_function*() all check that the target CPUs are
marked online before the arch code is requested to issue an IPI to the
target CPU.

The only place which is missing this check is smp_send_stop().

Now, we could make machine_shutdown() do this instead:

	if (is_smp())
		smp_send_stop();

but why bother calling into smp_send_stop(), and ultimately end up with
the _only_ site which calls smp_cross_call() with an empty CPU mask when
we can avoid the call when the CPU mask is empty - and prevent this
special case of smp_cross_call() being called with an empty CPU mask
ever occuring.
Will Deacon July 28, 2012, 12:11 p.m. UTC | #2
On Sat, Jul 28, 2012 at 11:08:31AM +0100, Russell King - ARM Linux wrote:
> On Fri, Jul 27, 2012 at 10:40:24PM +0100, Will Deacon wrote:
> > On Fri, Jul 27, 2012 at 10:06:37PM +0100, Russell King - ARM Linux wrote:
> > > We support booting a kernel on systems with or without SMP support, even
> > > with a SMP kernel.  When the kernel is booted on such a system, it is
> > > undefined whether smp_cross_call() is a valid function pointer.
> > 
> > So let's define it to point at a dummy function which explodes with a BUG if
> > the cpumask passed in isn't empty. That allows SMP kernels to do things like
> > `cross call to all other cores' without having to worry about whether there
> > are any other cores or not.
> 
> We should not be even attempting to do any cross calls when there aren't
> any other CPUs in the system - that's rather the point of leaving it as
> a NULL pointer so it does explode on such systems.
> 
> . the scheduler won't call smp_send_reschedule() when there are no other
>   CPUs in the system.
> . timer ticks won't be broadcast to other CPUs if there are no other CPUs
>   in the system.
> . function calls will not be issued to other CPUs if there are no other
>   CPUs in the system.
> 
> There is only one case where this doesn't happen, and that's the shutdown
> path.

Ok, that's a pretty compelling argument you've got there :)

> For instance, smp_call_function*() all check that the target CPUs are
> marked online before the arch code is requested to issue an IPI to the
> target CPU.

Yes, thanks for making the case for the original patch. Consistency is the
most important thing with these APIs, so I'll go full circle and offer my
ack for the original patch:

Acked-by: Will Deacon <will.deacon@arm.com>

The next question is: who's putting this into the patch system?

Will
Javier Martinez Canillas July 28, 2012, 2:26 p.m. UTC | #3
On Sat, Jul 28, 2012 at 2:11 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Sat, Jul 28, 2012 at 11:08:31AM +0100, Russell King - ARM Linux wrote:
>> On Fri, Jul 27, 2012 at 10:40:24PM +0100, Will Deacon wrote:
>> > On Fri, Jul 27, 2012 at 10:06:37PM +0100, Russell King - ARM Linux wrote:
>> > > We support booting a kernel on systems with or without SMP support, even
>> > > with a SMP kernel.  When the kernel is booted on such a system, it is
>> > > undefined whether smp_cross_call() is a valid function pointer.
>> >
>> > So let's define it to point at a dummy function which explodes with a BUG if
>> > the cpumask passed in isn't empty. That allows SMP kernels to do things like
>> > `cross call to all other cores' without having to worry about whether there
>> > are any other cores or not.
>>
>> We should not be even attempting to do any cross calls when there aren't
>> any other CPUs in the system - that's rather the point of leaving it as
>> a NULL pointer so it does explode on such systems.
>>
>> . the scheduler won't call smp_send_reschedule() when there are no other
>>   CPUs in the system.
>> . timer ticks won't be broadcast to other CPUs if there are no other CPUs
>>   in the system.
>> . function calls will not be issued to other CPUs if there are no other
>>   CPUs in the system.
>>
>> There is only one case where this doesn't happen, and that's the shutdown
>> path.
>
> Ok, that's a pretty compelling argument you've got there :)
>
>> For instance, smp_call_function*() all check that the target CPUs are
>> marked online before the arch code is requested to issue an IPI to the
>> target CPU.
>
> Yes, thanks for making the case for the original patch. Consistency is the
> most important thing with these APIs, so I'll go full circle and offer my
> ack for the original patch:
>
> Acked-by: Will Deacon <will.deacon@arm.com>
>
> The next question is: who's putting this into the patch system?
>
> Will

Hi,

I just put the patch with Will's Acked-by on the patch system. It  was
accepted as patch 7480/1 [1].

The patch is on top of today's linux-next (next-20120727).

Thanks a lot and best regards,
Javier

[1] http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=7480/1
Russell King - ARM Linux July 29, 2012, 9:31 p.m. UTC | #4
On Sat, Jul 28, 2012 at 04:26:28PM +0200, Javier Martinez Canillas wrote:
> On Sat, Jul 28, 2012 at 2:11 PM, Will Deacon <will.deacon@arm.com> wrote:
> > Acked-by: Will Deacon <will.deacon@arm.com>
> >
> > The next question is: who's putting this into the patch system?
> >
> > Will
> 
> Hi,
> 
> I just put the patch with Will's Acked-by on the patch system. It  was
> accepted as patch 7480/1 [1].
> 
> The patch is on top of today's linux-next (next-20120727).
> 
> Thanks a lot and best regards,
> Javier
> 
> [1] http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=7480/1

Isn't this a candidate for stable kernels?
Javier Martinez Canillas July 30, 2012, 9:22 a.m. UTC | #5
On Sun, Jul 29, 2012 at 11:31 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, Jul 28, 2012 at 04:26:28PM +0200, Javier Martinez Canillas wrote:
>> On Sat, Jul 28, 2012 at 2:11 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > Acked-by: Will Deacon <will.deacon@arm.com>
>> >
>> > The next question is: who's putting this into the patch system?
>> >
>> > Will
>>
>> Hi,
>>
>> I just put the patch with Will's Acked-by on the patch system. It  was
>> accepted as patch 7480/1 [1].
>>
>> The patch is on top of today's linux-next (next-20120727).
>>
>> Thanks a lot and best regards,
>> Javier
>>
>> [1] http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=7480/1
>
> Isn't this a candidate for stable kernels?

Yes, I forgot it, sorry for that.

Please let me know if I need to resubmit the patch cc'ing stable.

Best regards,
Javier
Russell King - ARM Linux July 30, 2012, 2:35 p.m. UTC | #6
On Mon, Jul 30, 2012 at 11:22:51AM +0200, Javier Martinez Canillas wrote:
> Yes, I forgot it, sorry for that.
> 
> Please let me know if I need to resubmit the patch cc'ing stable.

I can just add it to the commit.
Javier Martinez Canillas July 30, 2012, 2:51 p.m. UTC | #7
On Mon, Jul 30, 2012 at 4:35 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jul 30, 2012 at 11:22:51AM +0200, Javier Martinez Canillas wrote:
>> Yes, I forgot it, sorry for that.
>>
>> Please let me know if I need to resubmit the patch cc'ing stable.
>
> I can just add it to the commit.

Perfect, thanks a lot and sorry for missing that.

Best regards,
Javier
diff mbox

Patch

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 2c7217d..ffa411f 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -329,7 +329,13 @@  void __init smp_prepare_cpus(unsigned int max_cpus)
        }
 }
 
-static void (*smp_cross_call)(const struct cpumask *, unsigned int);
+static void dummy_smp_cross_call(const struct cpumask *mask, unsigned int irq)
+{
+       BUG_ON(!cpumask_empty(mask));
+}
+
+static void (*smp_cross_call)(const struct cpumask *, unsigned int) =
+       dummy_smp_cross_call;
 
 void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int))
 {