diff mbox

[PERCPU] Remove & in front of this_cpu_ptr

Message ID 0000013dd1a300fb-1fbb26a9-77a7-4c24-95ff-f088309206d9-000000@email.amazonses.com
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Christoph Lameter (Ampere) April 3, 2013, 8:42 p.m. UTC 
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

Comments

Tejun Heo April 3, 2013, 9:24 p.m. UTC | #1 
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.
Eric Dumazet April 3, 2013, 9:29 p.m. UTC | #2 
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
Christoph Lameter (Ampere) April 4, 2013, 1:52 p.m. UTC | #3 
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
Tejun Heo April 4, 2013, 2 p.m. UTC | #4 
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.
Christoph Lameter (Ampere) April 4, 2013, 2:21 p.m. UTC | #5 
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
Tejun Heo April 4, 2013, 2:25 p.m. UTC | #6 
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.
Eric Dumazet April 4, 2013, 2:29 p.m. UTC | #7 
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
Christoph Lameter (Ampere) April 4, 2013, 3:02 p.m. UTC | #8 
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
diff mbox

Patch

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);
 }