From: Shan Wei <[email protected]>
Signed-off-by: Shan Wei <[email protected]>
---
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:
> --- 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.
Christoph Lameter said, at 2012/11/1 1:39:
> On Wed, 31 Oct 2012, Shan Wei wrote:
>
>> --- 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 <[email protected]> wrote:
> 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.
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 <[email protected]> wrote:
> > 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.
>
> 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.