[PATCH 4/9] net: openvswitch: use this_cpu_ptr per-cpu helper

From: Shan Wei <redacted>

Signed-off-by: Shan Wei <redacted>
---
 net/openvswitch/datapath.c |    4 ++--
 net/openvswitch/vport.c    |    5 ++---
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 4c4b62c..77d16a5 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c

@@ -208,7 +208,7 @@ void ovs_dp_process_received_packet(struct vport *p, struct sk_buff *skb)
 	int error;
 	int key_len;
 
-	stats = per_cpu_ptr(dp->stats_percpu, smp_processor_id());
+	stats = this_cpu_ptr(dp->stats_percpu);
 
 	/* Extract flow from 'skb' into 'key'. */
 	error = ovs_flow_extract(skb, p->port_no, &key, &key_len);

@@ -282,7 +282,7 @@ int ovs_dp_upcall(struct datapath *dp, struct sk_buff *skb,
 	return 0;
 
 err:
-	stats = per_cpu_ptr(dp->stats_percpu, smp_processor_id());
+	stats = this_cpu_ptr(dp->stats_percpu);
 
 	u64_stats_update_begin(&stats->sync);
 	stats->n_lost++;

diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index 03779e8..70af0be 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c

@@ -333,8 +333,7 @@ void ovs_vport_receive(struct vport *vport, struct sk_buff *skb)
 {
 	struct vport_percpu_stats *stats;
 
-	stats = per_cpu_ptr(vport->percpu_stats, smp_processor_id());
-
+	stats = this_cpu_ptr(vport->percpu_stats);
 	u64_stats_update_begin(&stats->sync);
 	stats->rx_packets++;
 	stats->rx_bytes += skb->len;

@@ -359,7 +358,7 @@ int ovs_vport_send(struct vport *vport, struct sk_buff *skb)
 	if (likely(sent)) {
 		struct vport_percpu_stats *stats;
 
-		stats = per_cpu_ptr(vport->percpu_stats, smp_processor_id());
+		stats = this_cpu_ptr(vport->percpu_stats);
 
 		u64_stats_update_begin(&stats->sync);
 		stats->tx_packets++;

-- 
1.7.1

On Wed, 31 Oct 2012, Shan Wei wrote:

Well this is an improvement and may be ok if the preemption is disabled at
this point. There is another possibility here to use this_cpu_read/add/inc
instead of determining the pointer to the local cpu first and then
performing operations on the fields. The pointer relocation with
this_cpu_xxx ops is implicit in the instructions and safe against changing
of processors. It would also save us the determination of a pointer to the
current cpus stats structure.

Christoph Lameter said, at 2012/11/1 1:39:

On Wed, 31 Oct 2012, Shan Wei wrote:

quoted

--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c

@@ -208,7 +208,7 @@ void ovs_dp_process_received_packet(struct vport *p, struct sk_buff *skb)
 	int error;
 	int key_len;

-	stats = per_cpu_ptr(dp->stats_percpu, smp_processor_id());
+	stats = this_cpu_ptr(dp->stats_percpu);

Well this is an improvement and may be ok if the preemption is disabled at
this point. There is another possibility here to use this_cpu_read/add/inc
instead of determining the pointer to the local cpu first and then
performing operations on the fields. The pointer relocation with
this_cpu_xxx ops is implicit in the instructions and safe against changing
of processors. It would also save us the determination of a pointer to the
current cpus stats structure.

yes, this_cpu_ptr just locate the point to current cpu per-cpu data domain.
and then operating [read/write/inc/sub] fields of this per-cpu variable
maybe on other cpu because task is rescheduled for preemption, interrupt.

But for different field in same per-cpu variable, how to guarantee n_missed
and n_hit are from same cpu? 
this_cpu_read(dp->stats_percpu->n_missed);
[processor changed]
this_cpu_read(dp->stats_percpu->n_hit);


In addition, following usage of per_cpu_ptr can be replaced by this_cpu_read.

cpu=get_cpu()
....
*per_cpu_ptr(p,cpu)
....
....
put_cpu()

On Thu, 1 Nov 2012, Shan Wei wrote:

But for different field in same per-cpu variable, how to guarantee n_missed
and n_hit are from same cpu?
this_cpu_read(dp->stats_percpu->n_missed);
[processor changed]
this_cpu_read(dp->stats_percpu->n_hit);

What does current guarantee that? If it is guaranteed then you can use the
__this_cpu_xxx ops.

In addition, following usage of per_cpu_ptr can be replaced by this_cpu_read.

cpu=get_cpu()
....
*per_cpu_ptr(p,cpu)
....
....
put_cpu()

Right.

On Thu, Nov 1, 2012 at 7:33 AM, Christoph Lameter [off-list ref] wrote:

On Thu, 1 Nov 2012, Shan Wei wrote:

quoted

But for different field in same per-cpu variable, how to guarantee n_missed
and n_hit are from same cpu?
this_cpu_read(dp->stats_percpu->n_missed);
[processor changed]
this_cpu_read(dp->stats_percpu->n_hit);

What does current guarantee that? If it is guaranteed then you can use the
__this_cpu_xxx ops.

Preemption is disabled in all of the places where writes are done and
all of the reads are from foreign CPUs.

On Thu, 1 Nov 2012, Jesse Gross wrote:

On Thu, Nov 1, 2012 at 7:33 AM, Christoph Lameter [off-list ref] wrote:

quoted

On Thu, 1 Nov 2012, Shan Wei wrote:

quoted

But for different field in same per-cpu variable, how to guarantee n_missed
and n_hit are from same cpu?
this_cpu_read(dp->stats_percpu->n_missed);
[processor changed]
this_cpu_read(dp->stats_percpu->n_hit);

What does current guarantee that? If it is guaranteed then you can use the
__this_cpu_xxx ops.

Preemption is disabled in all of the places where writes are done and
all of the reads are from foreign CPUs.

Since preemption is disabled no processor change can occur. So its safe to
use __this_cpu ops throughout and they will operate on the current per cpu
area.