Re: [PATCH 7/8] percpu: add __percpu sparse annotations to hw_breakpoint

[Tejun Heo <tj@kernel.org>]

Hello, Frederic.

On 01/26/2010 09:19 AM, Frederic Weisbecker wrote:
> On Tue, Jan 26, 2010 at 12:22:14AM +0900, Tejun Heo wrote:
>> Add __percpu sparse annotations to hw_breakpoint.
>>
>> These annotations are to make sparse consider percpu variables to be
>> in a different address space and warn if accessed without going
>> through percpu accessors.  This patch doesn't affect normal builds.
>>
>> per_cpu(nr_task_bp_pinned, cpu) is replaced with
>> &per_cpu(nr_task_bp_pinned[0], cpu).  This is the same to the compiler
>> but allows per_cpu() macro to correctly drop __percpu designation for
>> the returned pointer.
> 
> Ouch... It's unpleasant to see such workaround that messes up the
> code just to make sparse happy.
> 
> I guess __percpu is an address_space attribute? Is there no
> way to force the address space change directly from the
> per_cpu() macro?

Yeah, per_cpu() macro does that but when things get a bit complicated
with static percpu arrays.  In the above case, the variable is defined
as

  static DEFINE_PER_CPU(unsigned int, nr_task_bp_pinned[HBP_NUM]);

which gets translated to

  static __attribute__((noderef, address_space(3))) \
	 __attribute__((section(.data.percpu))) \
	 __typeof__(unsigned int) nr_task_bp_pinned[HBP_NUM];

The above tells sparse that the members of nr_task_bp_pinned array are
in address space 3 which is correct.  The problematic dereference was

  unsigned int *task_pinned = per_cpu(nr_task_bp_pinned, cpu)

per_cpu() macro changes the address space of the resulting address but
it does so assuming that the parameter it got passed is the one which
got declared to be in the percpu address space.  It casts
nr_task_bp_pinned itself, which to the sparse isn't in the percpu
address space, to the kernel address space.  So, the workaround is
basically to give per_cpu() macro the same thing that was defined.

This type of usage (define as array, dereference the array as address)
was the only place where I needed to work around to make address space
change explicit.  There are two places which needed this and hwbreak
was one.  The options were...

* Leave it alone.  We can live with a few additional sparse warnings.

* Make the proposed change.  It is slightly ugly but not cryptic or
  difficult.

* Somehow teach per_cpu() macro or sparse how to handle the above
  right.

I tried to improve per_cpu() macro but couldn't do it in any sane way.
Leaving it alone isn't too bad either but given that the workaround is
not horribly unreadable, I think it's best to use the slightly less
elegant form in the few places where they are needed.

Thanks.

-- 
tejun