Message ID | 0000013dd1a300fb-1fbb26a9-77a7-4c24-95ff-f088309206d9-000000@email.amazonses.com |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
Hello, Christoph. On Wed, Apr 03, 2013 at 08:42:33PM +0000, Christoph Lameter wrote: > Subject: percpu: Remove & in front of this_cpu_ptr > > Both > > this_cpu_ptr(&percpu_pointer->field) > > > [Add Offset in percpu pointer to the field offset in the struct > and then add to the local percpu base] > > as well as > > &this_cpu_ptr(percpu_pointer)->field > > [Add percpu variable offset to local percpu base to form an address > and then add the field offset to the address]. > > are correct. However, the latter looks a bit more complicated. > The first one is easier to understand. The second one may be > more difficult for the compiler to optimize as well. I don't know about this one. I actually prefer the latter in that the pointer being passed into this_cpu_ptr() is something which is the actual percpu pointer either from variable declaration or the allocator. Sure, they both are just different expressions of the same thing but the former requires an extra guarantee from percpu subsystem that the accessors would work for pointers which aren't the exact values defined or allocated. I'd much prefer unfiying things toward the latter than the former. Thanks.
On Wed, 2013-04-03 at 14:24 -0700, Tejun Heo wrote: > I don't know about this one. I actually prefer the latter in that the > pointer being passed into this_cpu_ptr() is something which is the > actual percpu pointer either from variable declaration or the > allocator. Sure, they both are just different expressions of the same > thing but the former requires an extra guarantee from percpu subsystem > that the accessors would work for pointers which aren't the exact > values defined or allocated. I'd much prefer unfiying things toward > the latter than the former. I agree with you, I prefer &this_cpu_ptr(percpu_pointer)->field The offset is added after getting the address of the (percpu) base object. -- 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 Wed, 3 Apr 2013, Eric Dumazet wrote: > I agree with you, I prefer &this_cpu_ptr(percpu_pointer)->field > > The offset is added after getting the address of the (percpu) base > object. There are two offsets being added! percpu_pointer is not a pointer but an offset. this_cpu_ptr creates a pointer from the percpu base of the current processor by adding the offset of the percpu variable. The offset calculation better be in the parenthesis. The method that I proposed is also conforming with the use of other this_cpu_ops. F.e. In order to do a read one would need to do x = this_cpu_read(percpu_pointer->field) x = this_cpu_read(percpu_pointer)->field does not work (and does not pass 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
Hello, Christoph. On Thu, Apr 04, 2013 at 01:52:00PM +0000, Christoph Lameter wrote: > The method that I proposed is also conforming with the use of other > this_cpu_ops. F.e. In order to do a read one would need to do > > x = this_cpu_read(percpu_pointer->field) > > > > > x = this_cpu_read(percpu_pointer)->field > > does not work (and does not pass sparse). Right, this is true, and we *do* wanna support this_cpu ops other than this_cpu_ptr on per-cpu struct fields. The usage is still somewhat unusual tho. Can we please add documentation in the comments too? Thanks.
On Thu, 4 Apr 2013, Tejun Heo wrote: > Right, this is true, and we *do* wanna support this_cpu ops other than > this_cpu_ptr on per-cpu struct fields. The usage is still somewhat > unusual tho. Can we please add documentation in the comments too? I posted a patch adding documentation yesterday and you took it. ??? Add comments where? -- 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, Apr 04, 2013 at 02:21:57PM +0000, Christoph Lameter wrote: > On Thu, 4 Apr 2013, Tejun Heo wrote: > > > Right, this is true, and we *do* wanna support this_cpu ops other than > > this_cpu_ptr on per-cpu struct fields. The usage is still somewhat > > unusual tho. Can we please add documentation in the comments too? > > I posted a patch adding documentation yesterday and you took it. > ??? > > Add comments where? I was thinking above this_cpu_*() ops. Let's make it as conspicious as reasonably possible. It's a similar problem with declaring per-cpu arrays - there are a couple ways to do it and there's no way to automatically reject the one which isn't preferred. I don't know. Maybe all we can do is periodic sweep through the source tree and fix up the "wrong" ones. Thanks.
On Thu, 2013-04-04 at 13:52 +0000, Christoph Lameter wrote: > On Wed, 3 Apr 2013, Eric Dumazet wrote: > > > I agree with you, I prefer &this_cpu_ptr(percpu_pointer)->field > > > > The offset is added after getting the address of the (percpu) base > > object. > > There are two offsets being added! I was speaking of the offsetof(struct ..., field), not on the 'offset' you think (the percpu one). Thats why I prefer &this_cpu_ptr(percpu_pointer)->field Its clearer for me, but thats a very minor issue. -- 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, 4 Apr 2013, Tejun Heo wrote: > I was thinking above this_cpu_*() ops. Let's make it as conspicious > as reasonably possible. It's a similar problem with declaring per-cpu > arrays - there are a couple ways to do it and there's no way to > automatically reject the one which isn't preferred. I don't know. > Maybe all we can do is periodic sweep through the source tree and fix > up the "wrong" ones. Both ways are working just fine. I'd like to use more of these though and would like to tighten things up a bit before doing sweeps through the kernel. -- 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
Index: linux/fs/gfs2/rgrp.c =================================================================== --- linux.orig/fs/gfs2/rgrp.c 2013-04-03 15:25:22.576562629 -0500 +++ linux/fs/gfs2/rgrp.c 2013-04-03 15:26:43.045773676 -0500 @@ -1726,7 +1726,7 @@ static bool gfs2_rgrp_congested(const st s64 var; preempt_disable(); - st = &this_cpu_ptr(sdp->sd_lkstats)->lkstats[LM_TYPE_RGRP]; + st = this_cpu_ptr(&sdp->sd_lkstats->lkstats[LM_TYPE_RGRP]); r_srttb = st->stats[GFS2_LKS_SRTTB]; r_dcount = st->stats[GFS2_LKS_DCOUNT]; var = st->stats[GFS2_LKS_SRTTVARB] + Index: linux/mm/page_alloc.c =================================================================== --- linux.orig/mm/page_alloc.c 2013-04-03 15:25:22.576562629 -0500 +++ linux/mm/page_alloc.c 2013-04-03 15:30:02.124769119 -0500 @@ -1342,7 +1342,7 @@ void free_hot_cold_page(struct page *pag migratetype = MIGRATE_MOVABLE; } - pcp = &this_cpu_ptr(zone->pageset)->pcp; + pcp = this_cpu_ptr(&zone->pageset->pcp); if (cold) list_add_tail(&page->lru, &pcp->lists[migratetype]); else @@ -1484,7 +1484,7 @@ again: struct list_head *list; local_irq_save(flags); - pcp = &this_cpu_ptr(zone->pageset)->pcp; + pcp = this_cpu_ptr(&zone->pageset->pcp); list = &pcp->lists[migratetype]; if (list_empty(list)) { pcp->count += rmqueue_bulk(zone, 0, Index: linux/net/core/flow.c =================================================================== --- linux.orig/net/core/flow.c 2013-04-03 15:25:22.576562629 -0500 +++ linux/net/core/flow.c 2013-04-03 15:26:43.045773676 -0500 @@ -328,7 +328,7 @@ static void flow_cache_flush_per_cpu(voi 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); }
Subject: percpu: Remove & in front of this_cpu_ptr Both this_cpu_ptr(&percpu_pointer->field) [Add Offset in percpu pointer to the field offset in the struct and then add to the local percpu base] as well as &this_cpu_ptr(percpu_pointer)->field [Add percpu variable offset to local percpu base to form an address and then add the field offset to the address]. are correct. However, the latter looks a bit more complicated. The first one is easier to understand. The second one may be more difficult for the compiler to optimize as well. Convert all of them to this_cpu_ptr(&percpu_pointer->field). Signed-off-by: Christoph Lameter <cl@linux.com> -- 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