Messages in this thread | | | Date | Tue, 26 Jan 2010 09:48:45 +0900 | From | Tejun Heo <> | Subject | Re: [PATCH 7/8] percpu: add __percpu sparse annotations to hw_breakpoint |
| |
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
| |