| Message ID | 1264432935-10453-8-git-send-email-tj@kernel.org |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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? > A cast (using __typeof__) combined with an address space override? -hpa -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > Is there a patch that shows per_cpu() macro changes in the batch? Sorry I forgot to write about this. It's in the percpu tree. git://git.kernel.org/pub/scm/linux/kernel/git/tj/percpu.git for-next The related commits are 545695fb41da117928ab946067a42d9e15fd009d and e0fdb0e050eae331046385643618f12452aa7e73. Thanks.
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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. :-(
Thanks.
On Tue, Jan 26, 2010 at 03:01:14AM +0100, Frederic Weisbecker wrote: > 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; The above is perhaps a bit confusing. To be more clear, in the first case you only cast the pointer to the type, which gives you a pointer valid in kernel space to data valid in percpu space. The second case gives you something valid in kernel space for both. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On 01/26/2010 10:06 AM, H. Peter Anvin wrote: >> 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? Hmmm.... the argument gotta be an lvalue. Let me try just in case. Nope, it doesn't work. Thanks.
Hello, On 01/26/2010 11:01 AM, Frederic Weisbecker wrote: > 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))) arch/x86/kernel/cpu/common.c:1149:20: warning: cast to non-scalar arch/x86/kernel/cpu/common.c:1149:20: error: strange non-value function or array CC arch/x86/kernel/cpu/common.o arch/x86/kernel/cpu/common.c: In function 'cpu_init': arch/x86/kernel/cpu/common.c:1149: error: cast specifies array type Can't cast that way. :-(
On 01/26/2010 11:04 AM, Al Viro wrote: > 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... Here it is. # define RELOC_HIDE(ptr, off) \ ({ unsigned long __ptr; \ __ptr = (unsigned long) (ptr); \ (typeof(ptr)) (__ptr + (off)); }) #define __verify_pcpu_ptr(ptr) do { \ const void __percpu *__vpp_verify = (typeof(ptr))NULL; \ (void)__vpp_verify; \ } while (0) /* Weird cast keeps both GCC and sparse happy. */ #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))) Thanks.
On Tue, Jan 26, 2010 at 11:13:59AM +0900, Tejun Heo wrote: > Hello, > > On 01/26/2010 11:01 AM, Frederic Weisbecker wrote: > > 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))) > > arch/x86/kernel/cpu/common.c:1149:20: warning: cast to non-scalar > arch/x86/kernel/cpu/common.c:1149:20: error: strange non-value function or array > CC arch/x86/kernel/cpu/common.o > arch/x86/kernel/cpu/common.c: In function 'cpu_init': > arch/x86/kernel/cpu/common.c:1149: error: cast specifies array type > > Can't cast that way. :-( What about this? It doesn't use direct cast to scalar but should create a pointer type to kernel space datas: #define kernel_space_t(var) \ (typeof(var) __kernel __force) #define SHIFT_PERCPU_PTR(__p, __offset) ({ \ __verify_pcpu_ptr((__p)); \ RELOC_HIDE((typeof(*(kernel_space_t(var)) __kernel __force *)(__p), (__offset)); \ }) #define per_cpu(var, cpu) \ (*SHIFT_PERCPU_PTR(&(var), per_cpu_offset(cpu))) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 26, 2010 at 03:18:48AM +0100, Frederic Weisbecker wrote: > On Tue, Jan 26, 2010 at 11:13:59AM +0900, Tejun Heo wrote: > > Hello, > > > > On 01/26/2010 11:01 AM, Frederic Weisbecker wrote: > > > 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))) > > > > arch/x86/kernel/cpu/common.c:1149:20: warning: cast to non-scalar > > arch/x86/kernel/cpu/common.c:1149:20: error: strange non-value function or array > > CC arch/x86/kernel/cpu/common.o > > arch/x86/kernel/cpu/common.c: In function 'cpu_init': > > arch/x86/kernel/cpu/common.c:1149: error: cast specifies array type > > > > Can't cast that way. :-( > > > What about this? It doesn't use direct cast to scalar but should > create a pointer type to kernel space datas: > > > #define kernel_space_t(var) \ > (typeof(var) __kernel __force) Should be typeof(*var) > > #define SHIFT_PERCPU_PTR(__p, __offset) ({ \ > __verify_pcpu_ptr((__p)); \ > RELOC_HIDE((typeof(*(kernel_space_t(var)) __kernel __force *)(__p), (__offset)); \ > }) > > #define per_cpu(var, cpu) \ > (*SHIFT_PERCPU_PTR(&(var), per_cpu_offset(cpu))) > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 26, 2010 at 11:16:42AM +0900, Tejun Heo wrote: > # define RELOC_HIDE(ptr, off) \ > ({ unsigned long __ptr; \ > __ptr = (unsigned long) (ptr); \ > (typeof(ptr)) (__ptr + (off)); }) > > #define __verify_pcpu_ptr(ptr) do { \ > const void __percpu *__vpp_verify = (typeof(ptr))NULL; \ > (void)__vpp_verify; \ > } while (0) > > /* Weird cast keeps both GCC and sparse happy. */ > #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))) Eh... You are leaving that noderef in place in case of array. And _that_ is not an address space, so casts to AS 0 won't do you any good. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/26/2010 11:22 AM, Frederic Weisbecker wrote: >> What about this? It doesn't use direct cast to scalar but should >> create a pointer type to kernel space datas: >> >> >> #define kernel_space_t(var) \ >> (typeof(var) __kernel __force) > > > Should be typeof(*var) > >> >> #define SHIFT_PERCPU_PTR(__p, __offset) ({ \ >> __verify_pcpu_ptr((__p)); \ >> RELOC_HIDE((typeof(*(kernel_space_t(var)) __kernel __force *)(__p), (__offset)); \ >> }) Can you balance ()'s here too?
On Tue, Jan 26, 2010 at 11:34:40AM +0900, Tejun Heo wrote: > On 01/26/2010 11:22 AM, Frederic Weisbecker wrote: > >> What about this? It doesn't use direct cast to scalar but should > >> create a pointer type to kernel space datas: > >> > >> > >> #define kernel_space_t(var) \ > >> (typeof(var) __kernel __force) > > > > > > Should be typeof(*var) > > > >> > >> #define SHIFT_PERCPU_PTR(__p, __offset) ({ \ > >> __verify_pcpu_ptr((__p)); \ > >> RELOC_HIDE((typeof(*(kernel_space_t(var)) __kernel __force *)(__p), (__offset)); \ > >> }) > > Can you balance ()'s here too? #define kernel_space_t(var) \ (typeof(*(var)) __kernel __force) /* Weird cast keeps both GCC and sparse happy. */ #define SHIFT_PERCPU_PTR(__p, __offset) ({ \ __verify_pcpu_ptr((__p)); \ RELOC_HIDE((typeof(*(kernel_space_t(__p))) __kernel __force *)(__p), (__offset)); \ }) #define per_cpu(var, cpu) \ (*SHIFT_PERCPU_PTR(&(var), per_cpu_offset(cpu))); No guarantee that will build. I should pull your tree and install sparse (yeah, shame on me, I've never installed it). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On 01/26/2010 11:32 AM, Al Viro wrote: > On Tue, Jan 26, 2010 at 11:16:42AM +0900, Tejun Heo wrote: > >> # define RELOC_HIDE(ptr, off) \ >> ({ unsigned long __ptr; \ >> __ptr = (unsigned long) (ptr); \ >> (typeof(ptr)) (__ptr + (off)); }) >> >> #define __verify_pcpu_ptr(ptr) do { \ >> const void __percpu *__vpp_verify = (typeof(ptr))NULL; \ >> (void)__vpp_verify; \ >> } while (0) >> >> /* Weird cast keeps both GCC and sparse happy. */ >> #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))) > > Eh... You are leaving that noderef in place in case of array. And _that_ > is not an address space, so casts to AS 0 won't do you any good. Any ideas on how to fix it? Thanks.
On 01/26/2010 11:35 AM, Frederic Weisbecker wrote: > No guarantee that will build. I should pull your tree and install > sparse (yeah, shame on me, I've never installed it). Nope, it doesn't. Please pull from the following tree to receive the whole thing. The definitions in question are in include/asm-generic/percpu.h. git://git.kernel.org/pub/scm/linux/kernel/git/tj/percpu.git percpu-sparse-review After installing sparse, make C=2 arch/x86/kernel/cpu/common.o should be enough. Thanks.
On Tue, Jan 26, 2010 at 11:43:56AM +0900, Tejun Heo wrote: > > Eh... You are leaving that noderef in place in case of array. And _that_ > > is not an address space, so casts to AS 0 won't do you any good. > > Any ideas on how to fix it? BTW, before we go any further, which warnings are you getting from sparse and which version of sparse are you using? noderef is one thing; address_space mess is a different story. The version I have here steps into the former, but not the latter; what are you seeing? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On 01/26/2010 11:48 AM, Al Viro wrote: > On Tue, Jan 26, 2010 at 11:43:56AM +0900, Tejun Heo wrote: > >>> Eh... You are leaving that noderef in place in case of array. And _that_ >>> is not an address space, so casts to AS 0 won't do you any good. >> >> Any ideas on how to fix it? > > BTW, before we go any further, which warnings are you getting from sparse > and which version of sparse are you using? > > noderef is one thing; address_space mess is a different story. The version > I have here steps into the former, but not the latter; what are you seeing? Oops, I too am seeing the noderef thing not the address space warning. char *estacks = per_cpu(exception_stacks, cpu); I get arch/x86/kernel/cpu/common.c:1149:19: warning: incorrect type in initializer (different modifiers) arch/x86/kernel/cpu/common.c:1149:19: expected char *estacks arch/x86/kernel/cpu/common.c:1149:19: got char [noderef] *<noident> CC arch/x86/kernel/cpu/common.o $ rpm -qi sparse Name : sparse Relocations: (not relocatable) Version : 0.4.1.git1 Vendor: openSUSE Release : 3.2 Build Date: Sat 24 Oct 2009 11:58:16 AM KST Thanks.
On Tue, Jan 26, 2010 at 12:10:51PM +0900, Tejun Heo wrote: > Hello, > > On 01/26/2010 11:48 AM, Al Viro wrote: > > On Tue, Jan 26, 2010 at 11:43:56AM +0900, Tejun Heo wrote: > > > >>> Eh... You are leaving that noderef in place in case of array. And _that_ > >>> is not an address space, so casts to AS 0 won't do you any good. > >> > >> Any ideas on how to fix it? > > > > BTW, before we go any further, which warnings are you getting from sparse > > and which version of sparse are you using? > > > > noderef is one thing; address_space mess is a different story. The version > > I have here steps into the former, but not the latter; what are you seeing? > > Oops, I too am seeing the noderef thing not the address space warning. OK... So all messing around __kernel __force is actually a red herring. Frankly, for now I'd keep it as in your patch. Yes, including workarounds in these few places. Longer term... We probably want to implement __attribute__((qualify(...)))/__attribute__((unqualify(...))), revert __typeof__ for AS/noderef to what gcc is doing for normal qualifiers (i.e. "if p is int const *, typeof(*p) v gives const int") go for explicit __unqualify((address_space,noderef)) in there. Playing interesting games with arrays for unqualify (i.e. creating parallel chains of type nodes all way down to the place where original qualifier had been applied). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h index 41235c9..f462d3d 100644 --- a/include/linux/hw_breakpoint.h +++ b/include/linux/hw_breakpoint.h @@ -66,14 +66,14 @@ register_wide_hw_breakpoint_cpu(struct perf_event_attr *attr, perf_overflow_handler_t triggered, int cpu); -extern struct perf_event ** +extern struct perf_event * __percpu * register_wide_hw_breakpoint(struct perf_event_attr *attr, perf_overflow_handler_t triggered); extern int register_perf_hw_breakpoint(struct perf_event *bp); extern int __register_perf_hw_breakpoint(struct perf_event *bp); extern void unregister_hw_breakpoint(struct perf_event *bp); -extern void unregister_wide_hw_breakpoint(struct perf_event **cpu_events); +extern void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events); extern int reserve_bp_slot(struct perf_event *bp); extern void release_bp_slot(struct perf_event *bp); @@ -98,7 +98,7 @@ static inline struct perf_event * register_wide_hw_breakpoint_cpu(struct perf_event_attr *attr, perf_overflow_handler_t triggered, int cpu) { return NULL; } -static inline struct perf_event ** +static inline struct perf_event * __percpu * register_wide_hw_breakpoint(struct perf_event_attr *attr, perf_overflow_handler_t triggered) { return NULL; } static inline int @@ -107,7 +107,7 @@ static inline int __register_perf_hw_breakpoint(struct perf_event *bp) { return -ENOSYS; } static inline void unregister_hw_breakpoint(struct perf_event *bp) { } static inline void -unregister_wide_hw_breakpoint(struct perf_event **cpu_events) { } +unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events) { } static inline int reserve_bp_slot(struct perf_event *bp) {return -ENOSYS; } static inline void release_bp_slot(struct perf_event *bp) { } diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c index 50dbd59..11dae6d 100644 --- a/kernel/hw_breakpoint.c +++ b/kernel/hw_breakpoint.c @@ -74,7 +74,7 @@ static DEFINE_MUTEX(nr_bp_mutex); static unsigned int max_task_bp_pinned(int cpu) { int i; - unsigned int *tsk_pinned = per_cpu(nr_task_bp_pinned, cpu); + unsigned int *tsk_pinned = &per_cpu(nr_task_bp_pinned[0], cpu); for (i = HBP_NUM -1; i >= 0; i--) { if (tsk_pinned[i] > 0) @@ -163,7 +163,7 @@ static void toggle_bp_task_slot(struct task_struct *tsk, int cpu, bool enable) count = task_bp_pinned(tsk); - tsk_pinned = per_cpu(nr_task_bp_pinned, cpu); + tsk_pinned = &per_cpu(nr_task_bp_pinned[0], cpu); if (enable) { tsk_pinned[count]++; if (count > 0) @@ -377,17 +377,17 @@ EXPORT_SYMBOL_GPL(unregister_hw_breakpoint); * * @return a set of per_cpu pointers to perf events */ -struct perf_event ** +struct perf_event * __percpu * register_wide_hw_breakpoint(struct perf_event_attr *attr, perf_overflow_handler_t triggered) { - struct perf_event **cpu_events, **pevent, *bp; + struct perf_event * __percpu *cpu_events, **pevent, *bp; long err; int cpu; cpu_events = alloc_percpu(typeof(*cpu_events)); if (!cpu_events) - return ERR_PTR(-ENOMEM); + return (void __percpu __force *)ERR_PTR(-ENOMEM); get_online_cpus(); for_each_online_cpu(cpu) { @@ -415,7 +415,7 @@ fail: put_online_cpus(); free_percpu(cpu_events); - return ERR_PTR(err); + return (void __percpu __force *)ERR_PTR(err); } EXPORT_SYMBOL_GPL(register_wide_hw_breakpoint); @@ -423,7 +423,7 @@ EXPORT_SYMBOL_GPL(register_wide_hw_breakpoint); * unregister_wide_hw_breakpoint - unregister a wide breakpoint in the kernel * @cpu_events: the per cpu set of events to unregister */ -void unregister_wide_hw_breakpoint(struct perf_event **cpu_events) +void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events) { int cpu; struct perf_event **pevent; diff --git a/samples/hw_breakpoint/data_breakpoint.c b/samples/hw_breakpoint/data_breakpoint.c index c69cbe9..bd0f337 100644 --- a/samples/hw_breakpoint/data_breakpoint.c +++ b/samples/hw_breakpoint/data_breakpoint.c @@ -34,7 +34,7 @@ #include <linux/perf_event.h> #include <linux/hw_breakpoint.h> -struct perf_event **sample_hbp; +struct perf_event * __percpu *sample_hbp; static char ksym_name[KSYM_NAME_LEN] = "pid_max"; module_param_string(ksym, ksym_name, KSYM_NAME_LEN, S_IRUGO); @@ -61,8 +61,8 @@ static int __init hw_break_module_init(void) attr.bp_type = HW_BREAKPOINT_W | HW_BREAKPOINT_R; sample_hbp = register_wide_hw_breakpoint(&attr, sample_hbp_handler); - if (IS_ERR(sample_hbp)) { - ret = PTR_ERR(sample_hbp); + if (IS_ERR((void __force *)sample_hbp)) { + ret = PTR_ERR((void __force *)sample_hbp); goto fail; }
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. Signed-off-by: Tejun Heo <tj@kernel.org> Cc: Frederic Weisbecker <fweisbec@gmail.com> --- include/linux/hw_breakpoint.h | 8 ++++---- kernel/hw_breakpoint.c | 14 +++++++------- samples/hw_breakpoint/data_breakpoint.c | 6 +++--- 3 files changed, 14 insertions(+), 14 deletions(-)