lkml.org 
[lkml]   [2008]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/10] Add generic helpers for arch IPI function calls
Jens Axboe wrote:
> +/**
> + * __smp_call_function_single(): Run a function on another CPU
> + * @cpu: The CPU to run on.
> + * @data: Pre-allocated and setup data structure
> + *
> + * Like smp_call_function_single(), but allow caller to pass in a pre-allocated
> + * data structure. Useful for embedding @data inside other structures, for
> + * instance.
> + *
> + */
> +void __smp_call_function_single(int cpu, struct call_single_data *data)
>

Does this have any users? Proposed users?

> +{
> + /* Can deadlock when called with interrupts disabled */
> + WARN_ON((data->flags & CSD_FLAG_WAIT) && irqs_disabled());
>

Can it be reasonably be used without FLAG_WAIT set? If it doesn't have
FLAG_WAIT, it must have FLAG_ALLOC set, but it can't have FLAG_ALLOC if
the csd structure is embedded in something else.

> +
> + generic_exec_single(cpu, data);
> +}
> +
> +/**
> + * smp_call_function_mask(): Run a function on a set of other CPUs.
> + * @mask: The set of cpus to run on.
> + * @func: The function to run. This must be fast and non-blocking.
> + * @info: An arbitrary pointer to pass to the function.
> + * @wait: If true, wait (atomically) until function has completed on other CPUs.
> + *
> + * Returns 0 on success, else a negative status code.
> + *
> + * If @wait is true, then returns once @func has returned.
> + *
> + * You must not call this function with disabled interrupts or from a
> + * hardware interrupt handler or from a bottom half handler.
> + */
> +int smp_call_function_mask(cpumask_t mask, void (*func)(void *), void *info,
> + int wait)
> +{
> + struct call_function_data d;
> + struct call_function_data *data = NULL;
> + cpumask_t allbutself;
> + unsigned long flags;
> + int cpu, num_cpus;
> +
> + /* Can deadlock when called with interrupts disabled */
> + WARN_ON(irqs_disabled());
> +
> + cpu = smp_processor_id();
>

What prevents us from changing cpus? Are all callers supposed to
disable preemption? If so, should we WARN for it?

> + allbutself = cpu_online_map;
> + cpu_clear(cpu, allbutself);
> + cpus_and(mask, mask, allbutself);
> + num_cpus = cpus_weight(mask);
>

Ditto: What prevents the online map changing under our feet? Is
disabling preemption enough?

> +
> + /*
> + * If zero CPUs, return. If just a single CPU, turn this request
> + * into a targetted single call instead since it's faster.
> + */
> + if (!num_cpus)
> + return 0;
> + else if (num_cpus == 1) {
> + cpu = first_cpu(mask);
> + return smp_call_function_single(cpu, func, info, 0, wait);
> + }
>

It's weirdly inconsistent that smp_call_function_mask will ignore the
current cpu if its in the mask, but smp_call_function_single will call
the function locally if the specified cpu is the current one. I know
this is a hold-over from the old code, but could we finally fix it now?
I guess it's still reasonable for smp_call_function to call on
everywhere but current cpu, but it can do its own mask manipulation to
sort that out.

> +
> + if (!wait) {
> + data = kmalloc(sizeof(*data), GFP_ATOMIC);
> + if (data)
> + data->csd.flags = CSD_FLAG_ALLOC;
> + }
> + if (!data) {
> + data = &d;
> + data->csd.flags = CSD_FLAG_WAIT;
> + }
> +
> + spin_lock_init(&data->lock);
> + data->csd.func = func;
> + data->csd.info = info;
> + data->refs = num_cpus;
> +
> + /*
> + * need to see above stores before the cpumask is valid for the CPU
> + */
> + smp_wmb();
> + data->cpumask = mask;
> +
> + spin_lock_irqsave(&call_function_lock, flags);
> + list_add_tail_rcu(&data->csd.list, &call_function_queue);
> + spin_unlock_irqrestore(&call_function_lock, flags);
> +
> + /* Send a message to all CPUs in the map */
> + arch_send_call_function_ipi(mask);
> +
> + /* optionally wait for the CPUs to complete */
> + if (wait)
>
Should this test for data->csd.flags & CDS_FLAG_WAIT? Or should the "if
(!data)" path above also set "wait"?

> + csd_flag_wait(&data->csd);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(smp_call_function_mask);
> +
> +/**
> + * smp_call_function(): Run a function on all other CPUs.
> + * @func: The function to run. This must be fast and non-blocking.
> + * @info: An arbitrary pointer to pass to the function.
> + * @natomic: Unused
> + * @wait: If true, wait (atomically) until function has completed on other CPUs.
> + *
> + * Returns 0 on success, else a negative status code.
> + *
> + * If @wait is true, then returns once @func has returned; otherwise
> + * it returns just before the target cpu calls @func.
> + *
> + * You must not call this function with disabled interrupts or from a
> + * hardware interrupt handler or from a bottom half handler.
> + */
> +int smp_call_function(void (*func)(void *), void *info, int natomic, int wait)
> +{
> + int ret;
> +
> + preempt_disable();
> + ret = smp_call_function_mask(cpu_online_map, func, info, wait);
> + preempt_enable();
> + return ret;
> +}
> +EXPORT_SYMBOL(smp_call_function);
>



\
 
 \ /
  Last update: 2008-07-06 19:23    [W:0.231 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site