net: openvswitch: use this_cpu_ptr per-cpu helper
Subsystems (3)
PATCH 4/9 net: openvswitch: use this_cpu_ptr per-cpu helper
From: Shan Wei <[email protected]>
--- net/openvswitch/datapath.c | 4 ++-- net/openvswitch/vport.c | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-)
net/openvswitch/datapath.c +2 -2
@@ -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++;
net/openvswitch/vport.c +2 -3
@@ -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++;
Signature · 1 line
1.7.1
net/openvswitch/datapath.c void ovs_dp_process_received_packet(struct vport *p, struct sk_buff *skb) · line 211
Quoted 8 lines · Shan Wei
net/openvswitch/datapath.c +1
-1@@ -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.
Christoph Lameter said, at 2012/11/1 1:39:
Quoted 8 lines · Shan Wei
Quoted 7 lines
net/openvswitch/datapath.c +0
-0@@ -208,7 +208,7 @@ void ovs_dp_process_received_packet(struct vport *p, struct sk_buff *skb)int error;int key_len;
Quoted 10 lines
net/openvswitch/datapath.c void ovs_dp_process_received_packet(struct vport *p, struct sk_buff *skb) · line 211
Quoted 2 lines
net/openvswitch/datapath.c +1
-1- 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_xxxops 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()
Quoted 5 lines · Shan Wei
But for different field in same per-cpu variable, how to guarantee
n_missedandn_hitare 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.
Quoted 8 lines
In addition, following usage of
per_cpu_ptrcan be replaced bythis_cpu_read.cpu=get_cpu()
....*per_cpu_ptr(p,cpu)
....
....put_cpu()
Right.
A message that is not in the archive (sent off-list or not yet received): 0000013abc646055-30441bd6-241f-436a-a356-9cd462da66ce-000000-p/GC64/[email protected]
2012/11/1 Christoph Lameter <[email protected]>
Quoted 4 lines
Quoted 2 lines
In addition, following usage of
per_cpu_ptrcan be replaced by
this_cpu_read.Quoted 1 line
Quoted 10 lines
Quoted 6 lines
cpu=get_cpu()
....*per_cpu_ptr(p,cpu)
....
....put_cpu()Right.
Thanks very much,
I will resubmit new patchset.
Quoted 10 lines · Christoph Lameter
Quoted 5 lines · Shan Wei
But for different field in same per-cpu variable, how to guarantee
n_missedandn_hitare 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_xxxops.
Preemption is disabled in all of the places where writes are done and all of the reads are from foreign CPUs.
Quoted 9 lines · Jesse Gross
Quoted 8 lines · Christoph Lameter
Quoted 5 lines · Shan Wei
But for different field in same per-cpu variable, how to guarantee
n_missedandn_hitare from same cpu?this_cpu_read(dp->stats_percpu->n_missed);
[processor changed]this_cpu_read(dp->stats_percpu->n_hit);
Quoted 5 lines
Quoted 2 lines
What does current guarantee that? If it is guaranteed then you can use the
__this_cpu_xxxops.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.