Re: [PATCH 05/10] nohz: New tick dependency mask

[Chris Metcalf <cmetcalf@ezchip.com>]

On 07/23/2015 12:42 PM, Frederic Weisbecker wrote:
> +unsigned long __tick_nohz_set_tick_dependency(enum tick_dependency_bit bit,
> +					      unsigned long *dep)
> +{
> +	unsigned long prev;
> +	unsigned long old = *dep;
> +	unsigned long mask = BIT_MASK(bit);
> +
> +	while ((prev = cmpxchg(dep, old, old | mask)) != old) {
> +		old = prev;
> +		cpu_relax();
> +	}
> +
> +	return prev;
> +}

Why not use set_bit() here?  It is suitably atomic.

> +		/*
> +		* We need the IPIs to be sent from sane process context.
> +		* The posix cpu timers are always set with irqs disabled.
> +		*/

The block comment indentation is not quite right here.

> +void tick_nohz_set_tick_dependency_cpu(enum tick_dependency_bit bit, int cpu)
> +{
> +	unsigned long prev;
> +	struct tick_sched *ts;
> +
> +	ts = per_cpu_ptr(&tick_cpu_sched, cpu);
> +
> +	prev = __tick_nohz_set_tick_dependency(bit, &ts->tick_dependency);
> +	if (!prev)
> +		tick_nohz_full_kick_cpu(cpu);
> +}

I could imagine arguments for a WARN_ON() if cpu == smp_processor_id() 
here, since then you should be using the _thiscpu() variant.
Or, you could transparently call the _thiscpu() variant in that case.
I think some comment explaining why the approach you chose is better
than those alternatives would be helpful here, perhaps.

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com