| Message ID | 1364463761-32510-1-git-send-email-roy.qing.li@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2013-03-28 at 17:42 +0800, roy.qing.li@gmail.com wrote: > From: Li RongQing <roy.qing.li@gmail.com> > > flush_tasklet is not percpu var, and percpu is percpu var, and > this_cpu_ptr(&info->cache->percpu->flush_tasklet) > is not equal to > &this_cpu_ptr(info->cache->percpu)->flush_tasklet > > 1f743b076(use this_cpu_ptr per-cpu helper) introduced this bug. > > Signed-off-by: Li RongQing <roy.qing.li@gmail.com> > --- > net/core/flow.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/flow.c b/net/core/flow.c > index 7fae135..e8084b8 100644 > --- a/net/core/flow.c > +++ b/net/core/flow.c > @@ -346,7 +346,7 @@ static void flow_cache_flush_per_cpu(void *data) > struct flow_flush_info *info = data; > struct tasklet_struct *tasklet; > > - tasklet = this_cpu_ptr(&info->cache->percpu->flush_tasklet); > + tasklet = &this_cpu_ptr(info->cache->percpu)->flush_tasklet; > tasklet->data = (unsigned long)info; > tasklet_schedule(tasklet); > } Hi Any reason you dont Cc Shan Wei & Christoph Lameter ? Christoph, could this kind of error be detected by the compiler or sparse ? 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 Thu, 28 Mar 2013, Eric Dumazet wrote: > > flush_tasklet is not percpu var, and percpu is percpu var, and > > this_cpu_ptr(&info->cache->percpu->flush_tasklet) > > is not equal to > > &this_cpu_ptr(info->cache->percpu)->flush_tasklet &this_cpu_ptr is always an error since you are taking the addresss of an address. this_cpu_ptr(&structure) is the right way to get the address of the cpu instance for this cpu for a per cpu structure. > Christoph, could this kind of error be detected by the compiler or > sparse ? The per cpu variables are marked with __percpu. This should be detected by sparse. -- 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 Thu, 2013-03-28 at 14:38 +0000, Christoph Lameter wrote: > On Thu, 28 Mar 2013, Eric Dumazet wrote: > > > Christoph, could this kind of error be detected by the compiler or > > sparse ? > > The per cpu variables are marked with __percpu. This should be detected by > sparse. make C=2 net/core/flow.o CHECK net/core/flow.c No warning. -- 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 Thu, 28 Mar 2013, Eric Dumazet wrote: > On Thu, 2013-03-28 at 14:38 +0000, Christoph Lameter wrote: > > On Thu, 28 Mar 2013, Eric Dumazet wrote: > > > > > > Christoph, could this kind of error be detected by the compiler or > > > sparse ? > > > > The per cpu variables are marked with __percpu. This should be detected by > > sparse. > > make C=2 net/core/flow.o > > CHECK net/core/flow.c > > No warning. huh? this_cpu_ptr uses SHIFT_PERCPU_PTR #ifndef SHIFT_PERCPU_PTR /* 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)); \ }) #endif This would mean that __verify_pcpu_ptr is broken. -- 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
2013/3/28 Christoph Lameter <cl@linux.com>: > On Thu, 28 Mar 2013, Eric Dumazet wrote: > >> > flush_tasklet is not percpu var, and percpu is percpu var, and >> > this_cpu_ptr(&info->cache->percpu->flush_tasklet) >> > is not equal to >> > &this_cpu_ptr(info->cache->percpu)->flush_tasklet > > &this_cpu_ptr is always an error since you are taking the addresss of an > address. > &this_cpu_ptr()->flush_tasklet, "->" has high priority than "&" so the result is same as &(this_cpu_ptr()->flush_tasklet) it should not a issue. flush_tasklet is not a percpu var, it is a member of percpu var. -Roy > this_cpu_ptr(&structure) is the right way to get the address of the cpu > instance for this cpu for a per cpu structure. > >> Christoph, could this kind of error be detected by the compiler or >> sparse ? > > The per cpu variables are marked with __percpu. This should be detected by > sparse. > -- 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
From: roy.qing.li@gmail.com Date: Thu, 28 Mar 2013 17:42:41 +0800 > From: Li RongQing <roy.qing.li@gmail.com> > > flush_tasklet is not percpu var, and percpu is percpu var, and > this_cpu_ptr(&info->cache->percpu->flush_tasklet) > is not equal to > &this_cpu_ptr(info->cache->percpu)->flush_tasklet > > 1f743b076(use this_cpu_ptr per-cpu helper) introduced this bug. > > Signed-off-by: Li RongQing <roy.qing.li@gmail.com> 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
On Fri, 29 Mar 2013, RongQing Li wrote: > > &this_cpu_ptr is always an error since you are taking the addresss of an > > address. > > > > &this_cpu_ptr()->flush_tasklet, "->" has high priority than "&" > so the result is same as > &(this_cpu_ptr()->flush_tasklet) Ok. This is the same as this_cpu_read(xxx.flush_tasklet) Looks less confusing to me. > flush_tasklet is not a percpu var, it is a member of percpu var. Well then it would be best to use this_cpu_read() instead of this_cpu_ptr. It also will generate better code. -- 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, 2013-04-01 at 15:21 +0000, Christoph Lameter wrote: > On Fri, 29 Mar 2013, RongQing Li wrote: > > flush_tasklet is not a percpu var, it is a member of percpu var. > > Well then it would be best to use this_cpu_read() instead of this_cpu_ptr. > It also will generate better code. I believe we already had this discussion in the past. flush_tasklet is a structure, and we need its address, not read its content. You can not use this_cpu_read() to get its address, and following code is fine. tasklet = &this_cpu_ptr(info->cache->percpu)->flush_tasklet; Similar to this code in mm/page_alloc.c pcp = &this_cpu_ptr(zone->pageset)->pcp; -- 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, 1 Apr 2013, Eric Dumazet wrote: > On Mon, 2013-04-01 at 15:21 +0000, Christoph Lameter wrote: > > On Fri, 29 Mar 2013, RongQing Li wrote: > > > > flush_tasklet is not a percpu var, it is a member of percpu var. > > > > Well then it would be best to use this_cpu_read() instead of this_cpu_ptr. > > It also will generate better code. > > I believe we already had this discussion in the past. > > flush_tasklet is a structure, and we need its address, not read its > content. > > You can not use this_cpu_read() to get its address, and following > code is fine. > > tasklet = &this_cpu_ptr(info->cache->percpu)->flush_tasklet; that is confusing.. tasklet = this_cpu_ptr(&info->cache->percpu->flushtasklet) this_cpu_ptr performs an address relocation. The address then is the one of flushtasklet. > Similar to this code in mm/page_alloc.c > > pcp = &this_cpu_ptr(zone->pageset)->pcp; Yeah thats my (early) code using these features. pcp = this_cpu_ptr(&zone->pageset->pcp) I need to do a writeup on this one. -- 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/net/core/flow.c b/net/core/flow.c index 7fae135..e8084b8 100644 --- a/net/core/flow.c +++ b/net/core/flow.c @@ -346,7 +346,7 @@ static void flow_cache_flush_per_cpu(void *data) struct flow_flush_info *info = data; struct tasklet_struct *tasklet; - tasklet = this_cpu_ptr(&info->cache->percpu->flush_tasklet); + tasklet = &this_cpu_ptr(info->cache->percpu)->flush_tasklet; tasklet->data = (unsigned long)info; tasklet_schedule(tasklet); }