percpu: add __percpu sparse annotations to hw_breakpoint
in [Kernel]
|
From: Frederic Weisbecker on 25 Jan 2010 19:20 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? Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 25 Jan 2010 20:10 On Tue, Jan 26, 2010 at 09:48:45AM +0900, Tejun Heo wrote: > 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. Ok. Well, sorry I must be missing something obvious, but is it impossible to make per_cpu(var, cpu) returning something cast in: (typeof(var) __force) Or I guess you did that already and it is not working with static arrays, or? Is there a patch that shows per_cpu() macro changes in the batch? Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 25 Jan 2010 20:20 On Mon, Jan 25, 2010 at 05:06:37PM -0800, H. Peter Anvin wrote: > On 01/25/2010 04:19 PM, 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? > > > > Hmm... thinking more about it, we should be able to just move the & and > [0] into the per_cpu() macro, addressing the situation, or does that > cause problems elsewhere? > > -hpa That would work only with arrays. per_cpu() can access either pointers or direct values. Well that can be worked around with fake casts, but I would except the (typeof(x) __force) to work and then offer a more elegant solution. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 25 Jan 2010 21:10 On Tue, Jan 26, 2010 at 10:19:04AM +0900, Tejun Heo wrote: > Hello, > > On 01/26/2010 10:02 AM, Frederic Weisbecker wrote: > > Well, sorry I must be missing something obvious, but is it impossible > > to make per_cpu(var, cpu) returning something cast in: > > > > (typeof(var) __force) > > > > Or I guess you did that already and it is not working with static > > arrays, or? > > Yeap, the definition looks like > > #define SHIFT_PERCPU_PTR(__p, __offset) ({ \ > __verify_pcpu_ptr((__p)); \ > RELOC_HIDE((typeof(*(__p)) __kernel __force *)(__p), (__offset)); \ > }) > > #define per_cpu(var, cpu) \ > (*SHIFT_PERCPU_PTR(&(var), per_cpu_offset(cpu))) > > but it just ends up putting the __force at the wrong layer. It seems > that (typeof(var) __kernel __force) tell sparse var is in the kernel > address space but not its members. So, may be it considers you are applying the address space overriding to the pointer to the type and not to the type itself. Consider: int __percpu i; What you do above *might* be considered as if SHIFT_PERCPU_PTR returns something of a type: int * __percpu i; So the pointer is in the normal address space, but its content is in __percpu address space. What if you do this: #define SHIFT_PERCPU_PTR(__p, __offset) ({ \ __verify_pcpu_ptr((__p)); \ RELOC_HIDE((__p), (__offset)); \ }) #define per_cpu(var, cpu) \ (typeof(var) __kernel __force)(*SHIFT_PERCPU_PTR(&(var), per_cpu_offset(cpu))) This should work because &(var) should be dereferencable directly, since it is not of type "__force t" but of type "*__force t" And you're not doing anymore this: *(int * __kernel __force) i; but *(int __kernel __force *) i; That all might make no sense, I'm just trying to think like a backend so it might sound like I should just take more sleep and just shut up... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Al Viro on 25 Jan 2010 21:10 On Tue, Jan 26, 2010 at 11:06:10AM +0900, Tejun Heo wrote: > Hello, > > On 01/26/2010 10:02 AM, H. Peter Anvin wrote: > > A cast (using __typeof__) combined with an address space override? > > That still puts the address specification at the wrong level. The > problem is that the __typeof__ would be an array type which itself > doesn't have address space set but its members are in address space 3. > So, you need to get *inside* the array type def to change that. :-( Could you post the actual definitions in one piece? Would be easier to discuss what's going on... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|
Next
|
Last
Pages: 1 2 3 Prev: pci: Update pci_set_vga_state to call arch functions Next: scripts/kallsyms build warning fix |