[linux-yocto] [PATCH 16/17] kernel/smp: Allow smp_call_function_single() to be called with a function that doesn't return.

[Bruce Ashfield]

On 14-07-08 10:22 AM, Charlie Paul wrote:
> From: John Jacques <john.jacques at lsi.com>
>
> When we do a reset (core, not chip or system), the core that is doing the reset has to
> tell all the other cores to reset.  To do this, we call smp_call_function_single().
> In this case the function specified in smp_call_function_single() doesn't return.
> The "wait" parameter (third argument, described as '@wait: If true, wait until
> function has completed on other CPUs.') doesn't work as described without the patch.

We need more explanation here. Why doesn't it work ? Deadlock ?
Something else ?

>
> Signen-off-by: John Jacques <john.jacques at lsi.com>
> ---
>   kernel/smp.c |   24 +++++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/smp.c b/kernel/smp.c
> index d5f3238..040b2b1 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -17,14 +17,14 @@
>   static struct {
>   	struct list_head	queue;
>   	raw_spinlock_t		lock;
> -} call_function __cacheline_aligned_in_smp =
> -	{
> +} call_function __cacheline_aligned_in_smp = {

Cosmetic change.

>   		.queue		= LIST_HEAD_INIT(call_function.queue),
>   		.lock		= __RAW_SPIN_LOCK_UNLOCKED(call_function.lock),
>   	};
>
>   enum {
>   	CSD_FLAG_LOCK		= 0x01,
> +	CSD_FLAG_NOWAIT		= 0x02,
>   };
>
>   struct call_function_data {
> @@ -268,6 +268,8 @@ void generic_smp_call_function_single_interrupt(void)
>
>   	while (!list_empty(&list)) {
>   		struct call_single_data *data;
> +		void (*func)(void *);
> +		void *info;
>
>   		data = list_entry(list.next, struct call_single_data, list);
>   		list_del(&data->list);
> @@ -278,12 +280,21 @@ void generic_smp_call_function_single_interrupt(void)
>   		 * so save them away before making the call:
>   		 */
>   		data_flags = data->flags;
> -
> -		data->func(data->info);
> +		func = data->func;
> +		info = data->info;

Why create these local variables ? func is only used once, and
info in that very same call. Hardly worth modifying the original
code for the change. Keeping the patch footprint small is a
virtue.

>
>   		/*
> +		 * Unlock before calling func so that func never has
> +		 * to return.
> +		 *
>   		 * Unlocked CSDs are valid through generic_exec_single():
>   		 */
> +		if ((data_flags & CSD_FLAG_LOCK) &&
> +		    (data_flags & CSD_FLAG_NOWAIT))

To confirm that this is not changing semantics, you now must be both
lock and no wait to unlock, but is "no wait" the default ? i.e. I
don't see how an existing caller that sets the lock flag, can be
guaranteed to also be 'no wait'.


> +			csd_unlock(data);
> +
> +		func(info);
> +
>   		if (data_flags & CSD_FLAG_LOCK)
>   			csd_unlock(data);
>   	}
> @@ -337,6 +348,9 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
>
>   			csd_lock(data);
>
> +			if (!wait)
> +				data->flags |= CSD_FLAG_NOWAIT;
> +
>   			data->func = func;
>   			data->info = info;
>   			generic_exec_single(cpu, data, wait);
> @@ -672,7 +686,7 @@ EXPORT_SYMBOL(nr_cpu_ids);
>   /* An arch may set nr_cpu_ids earlier if needed, so this would be redundant */
>   void __init setup_nr_cpu_ids(void)
>   {
> -	nr_cpu_ids = find_last_bit(cpumask_bits(cpu_possible_mask),NR_CPUS) + 1;
> +	nr_cpu_ids = find_last_bit(cpumask_bits(cpu_possible_mask), NR_CPUS) + 1;

Cosmetic change that should be dropped.

Bruce

>   }
>
>   /* Called by boot processor to activate the rest. */
>