[PATCH][net-next] openvswitch: change the data type of error status to atomic_long_t

[PATCH][net-next] openvswitch: change the data type of error status to atomic_long_t

[roy.qing.li]

From: Li RongQing <roy.qing.li@gmail.com>

Change the date type of error status from u64 to atomic_long_t, and use atomic
operation, then remove the lock which is used to protect the error status.

The operation of atomic maybe faster than spin lock.

Cc: Pravin Shelar <pshelar@nicira.com>
Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
---
 net/openvswitch/vport.c |   25 ++++++++-----------------
 net/openvswitch/vport.h |   10 ++++------
 2 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index 6d8f2ec..f7e63f9 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -148,8 +148,6 @@ struct vport *ovs_vport_alloc(int priv_size, const struct vport_ops *ops,
 		return ERR_PTR(-ENOMEM);
 	}
 
-	spin_lock_init(&vport->stats_lock);
-
 	return vport;
 }
 
@@ -268,14 +266,10 @@ void ovs_vport_get_stats(struct vport *vport, struct ovs_vport_stats *stats)
 	 * netdev-stats can be directly read over netlink-ioctl.
 	 */
 
-	spin_lock_bh(&vport->stats_lock);
-
-	stats->rx_errors	= vport->err_stats.rx_errors;
-	stats->tx_errors	= vport->err_stats.tx_errors;
-	stats->tx_dropped	= vport->err_stats.tx_dropped;
-	stats->rx_dropped	= vport->err_stats.rx_dropped;
-
-	spin_unlock_bh(&vport->stats_lock);
+	stats->rx_errors  = atomic_long_read(&vport->err_stats.rx_errors);
+	stats->tx_errors  = atomic_long_read(&vport->err_stats.tx_errors);
+	stats->tx_dropped = atomic_long_read(&vport->err_stats.tx_dropped);
+	stats->rx_dropped = atomic_long_read(&vport->err_stats.rx_dropped);
 
 	for_each_possible_cpu(i) {
 		const struct pcpu_sw_netstats *percpu_stats;
@@ -495,27 +489,24 @@ int ovs_vport_send(struct vport *vport, struct sk_buff *skb)
 static void ovs_vport_record_error(struct vport *vport,
 				   enum vport_err_type err_type)
 {
-	spin_lock(&vport->stats_lock);
-
 	switch (err_type) {
 	case VPORT_E_RX_DROPPED:
-		vport->err_stats.rx_dropped++;
+		atomic_long_inc(&vport->err_stats.rx_dropped);
 		break;
 
 	case VPORT_E_RX_ERROR:
-		vport->err_stats.rx_errors++;
+		atomic_long_inc(&vport->err_stats.rx_errors);
 		break;
 
 	case VPORT_E_TX_DROPPED:
-		vport->err_stats.tx_dropped++;
+		atomic_long_inc(&vport->err_stats.tx_dropped);
 		break;
 
 	case VPORT_E_TX_ERROR:
-		vport->err_stats.tx_errors++;
+		atomic_long_inc(&vport->err_stats.tx_errors);
 		break;
 	}
 
-	spin_unlock(&vport->stats_lock);
 }
 
 static void free_vport_rcu(struct rcu_head *rcu)
diff --git a/net/openvswitch/vport.h b/net/openvswitch/vport.h
index 35f89d8..0d95b9f 100644
--- a/net/openvswitch/vport.h
+++ b/net/openvswitch/vport.h
@@ -62,10 +62,10 @@ int ovs_vport_send(struct vport *, struct sk_buff *);
 /* The following definitions are for implementers of vport devices: */
 
 struct vport_err_stats {
-	u64 rx_dropped;
-	u64 rx_errors;
-	u64 tx_dropped;
-	u64 tx_errors;
+	atomic_long_t rx_dropped;
+	atomic_long_t rx_errors;
+	atomic_long_t tx_dropped;
+	atomic_long_t tx_errors;
 };
 /**
  * struct vport_portids - array of netlink portids of a vport.
@@ -93,7 +93,6 @@ struct vport_portids {
  * @dp_hash_node: Element in @datapath->ports hash table in datapath.c.
  * @ops: Class structure.
  * @percpu_stats: Points to per-CPU statistics used and maintained by vport
- * @stats_lock: Protects @err_stats;
  * @err_stats: Points to error statistics used and maintained by vport
  */
 struct vport {
@@ -108,7 +107,6 @@ struct vport {
 
 	struct pcpu_sw_netstats __percpu *percpu_stats;
 
-	spinlock_t stats_lock;
 	struct vport_err_stats err_stats;
 };
 
-- 
1.7.10.4

Re: [PATCH][net-next] openvswitch: change the data type of error status to atomic_long_t

[Pravin Shelar]

On Sat, Sep 6, 2014 at 4:06 AM,  <roy.qing.li@gmail.com> wrote:
> From: Li RongQing <roy.qing.li@gmail.com>
>
> Change the date type of error status from u64 to atomic_long_t, and use atomic
> operation, then remove the lock which is used to protect the error status.
>
> The operation of atomic maybe faster than spin lock.

What is reason for this change?

>
> Cc: Pravin Shelar <pshelar@nicira.com>
> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
> ---
>  net/openvswitch/vport.c |   25 ++++++++-----------------
>  net/openvswitch/vport.h |   10 ++++------
>  2 files changed, 12 insertions(+), 23 deletions(-)
>
> diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
> index 6d8f2ec..f7e63f9 100644
> --- a/net/openvswitch/vport.c
> +++ b/net/openvswitch/vport.c
> @@ -148,8 +148,6 @@ struct vport *ovs_vport_alloc(int priv_size, const struct vport_ops *ops,
>                 return ERR_PTR(-ENOMEM);
>         }
>
> -       spin_lock_init(&vport->stats_lock);
> -
>         return vport;
>  }
>
> @@ -268,14 +266,10 @@ void ovs_vport_get_stats(struct vport *vport, struct ovs_vport_stats *stats)
>          * netdev-stats can be directly read over netlink-ioctl.
>          */
>
> -       spin_lock_bh(&vport->stats_lock);
> -
> -       stats->rx_errors        = vport->err_stats.rx_errors;
> -       stats->tx_errors        = vport->err_stats.tx_errors;
> -       stats->tx_dropped       = vport->err_stats.tx_dropped;
> -       stats->rx_dropped       = vport->err_stats.rx_dropped;
> -
> -       spin_unlock_bh(&vport->stats_lock);
> +       stats->rx_errors  = atomic_long_read(&vport->err_stats.rx_errors);
> +       stats->tx_errors  = atomic_long_read(&vport->err_stats.tx_errors);
> +       stats->tx_dropped = atomic_long_read(&vport->err_stats.tx_dropped);
> +       stats->rx_dropped = atomic_long_read(&vport->err_stats.rx_dropped);
>
>         for_each_possible_cpu(i) {
>                 const struct pcpu_sw_netstats *percpu_stats;
> @@ -495,27 +489,24 @@ int ovs_vport_send(struct vport *vport, struct sk_buff *skb)
>  static void ovs_vport_record_error(struct vport *vport,
>                                    enum vport_err_type err_type)
>  {
> -       spin_lock(&vport->stats_lock);
> -
>         switch (err_type) {
>         case VPORT_E_RX_DROPPED:
> -               vport->err_stats.rx_dropped++;
> +               atomic_long_inc(&vport->err_stats.rx_dropped);
>                 break;
>
>         case VPORT_E_RX_ERROR:
> -               vport->err_stats.rx_errors++;
> +               atomic_long_inc(&vport->err_stats.rx_errors);
>                 break;
>
>         case VPORT_E_TX_DROPPED:
> -               vport->err_stats.tx_dropped++;
> +               atomic_long_inc(&vport->err_stats.tx_dropped);
>                 break;
>
>         case VPORT_E_TX_ERROR:
> -               vport->err_stats.tx_errors++;
> +               atomic_long_inc(&vport->err_stats.tx_errors);
>                 break;
>         }
>
> -       spin_unlock(&vport->stats_lock);
>  }
>
>  static void free_vport_rcu(struct rcu_head *rcu)
> diff --git a/net/openvswitch/vport.h b/net/openvswitch/vport.h
> index 35f89d8..0d95b9f 100644
> --- a/net/openvswitch/vport.h
> +++ b/net/openvswitch/vport.h
> @@ -62,10 +62,10 @@ int ovs_vport_send(struct vport *, struct sk_buff *);
>  /* The following definitions are for implementers of vport devices: */
>
>  struct vport_err_stats {
> -       u64 rx_dropped;
> -       u64 rx_errors;
> -       u64 tx_dropped;
> -       u64 tx_errors;
> +       atomic_long_t rx_dropped;
> +       atomic_long_t rx_errors;
> +       atomic_long_t tx_dropped;
> +       atomic_long_t tx_errors;
>  };
>  /**
>   * struct vport_portids - array of netlink portids of a vport.
> @@ -93,7 +93,6 @@ struct vport_portids {
>   * @dp_hash_node: Element in @datapath->ports hash table in datapath.c.
>   * @ops: Class structure.
>   * @percpu_stats: Points to per-CPU statistics used and maintained by vport
> - * @stats_lock: Protects @err_stats;
>   * @err_stats: Points to error statistics used and maintained by vport
>   */
>  struct vport {
> @@ -108,7 +107,6 @@ struct vport {
>
>         struct pcpu_sw_netstats __percpu *percpu_stats;
>
> -       spinlock_t stats_lock;
>         struct vport_err_stats err_stats;
>  };
>
> --
> 1.7.10.4
>

Re: [PATCH][net-next] openvswitch: change the data type of error status to atomic_long_t

[Li RongQing]

On Sun, Sep 7, 2014 at 11:44 AM, Pravin Shelar <pshelar@nicira.com> wrote:
>> The operation of atomic maybe faster than spin lock.
>
> What is reason for this change?

1.  The operation of atomic maybe faster than spin lock
2.  I did not find that tx_dropped/tx_error/.. is protected by spin
lock under net dir,
sometime tx_dropped is atomic_long_t; sometime it is percpu variable;
sometime it is
u64,but does not need to protect.

-Roy

Re: [PATCH][net-next] openvswitch: change the data type of error status to atomic_long_t

[David Miller]

From: Li RongQing <roy.qing.li@gmail.com>
Date: Sun, 7 Sep 2014 17:24:11 +0800

> 2.  I did not find that tx_dropped/tx_error/.. is protected by spin
> lock under net dir,
> sometime tx_dropped is atomic_long_t; sometime it is percpu variable;
> sometime it is
> u64,but does not need to protect.

If it is only modified in ->ndo_start_xmit() then it is protected by
the per-queue TX lock, as ->ndo_start_xmit() is always invoked with
it held (except in LLTX drivers of course).

Re: [PATCH][net-next] openvswitch: change the data type of error status to atomic_long_t

[Li RongQing]

On Mon, Sep 8, 2014 at 7:17 AM, David Miller <davem@davemloft.net> wrote:
> From: Li RongQing <roy.qing.li@gmail.com>
> Date: Sun, 7 Sep 2014 17:24:11 +0800
>
>> 2.  I did not find that tx_dropped/tx_error/.. is protected by spin
>> lock under net dir,
>> sometime tx_dropped is atomic_long_t; sometime it is percpu variable;
>> sometime it is
>> u64,but does not need to protect.
>
> If it is only modified in ->ndo_start_xmit() then it is protected by
> the per-queue TX lock, as ->ndo_start_xmit() is always invoked with
> it held (except in LLTX drivers of course).


But reading tx_dropped is in process context, and maybe break by interrupt or
soft interrupt,  and no lock protect, or not the same lock protect.

-Roy

Re: [PATCH][net-next] openvswitch: change the data type of error status to atomic_long_t

[Cong Wang]

On Sun, Sep 7, 2014 at 2:24 AM, Li RongQing <roy.qing.li@gmail.com> wrote:
> On Sun, Sep 7, 2014 at 11:44 AM, Pravin Shelar <pshelar@nicira.com> wrote:
>>> The operation of atomic maybe faster than spin lock.
>>
>> What is reason for this change?
>
> 1.  The operation of atomic maybe faster than spin lock
> 2.  I did not find that tx_dropped/tx_error/.. is protected by spin
> lock under net dir,
> sometime tx_dropped is atomic_long_t; sometime it is percpu variable;
> sometime it is
> u64,but does not need to protect.

I didn't dig the history of the code, why not use u64_stats_sync though?

Re: [PATCH][net-next] openvswitch: change the data type of error status to atomic_long_t

[Pravin Shelar]

On Sun, Sep 7, 2014 at 2:24 AM, Li RongQing <roy.qing.li@gmail.com> wrote:
> On Sun, Sep 7, 2014 at 11:44 AM, Pravin Shelar <pshelar@nicira.com> wrote:
>>> The operation of atomic maybe faster than spin lock.
>>
>> What is reason for this change?
>
> 1.  The operation of atomic maybe faster than spin lock
> 2.  I did not find that tx_dropped/tx_error/.. is protected by spin
> lock under net dir,
> sometime tx_dropped is atomic_long_t; sometime it is percpu variable;
> sometime it is
> u64,but does not need to protect.
>

These are error counter and the access is not performance sensitive
code. So I do not see obvious need to optimize it. Do you have any
performance number for this patch?

Re: [PATCH][net-next] openvswitch: change the data type of error status to atomic_long_t

[Li RongQing]

On Tue, Sep 9, 2014 at 6:26 AM, Pravin Shelar <pshelar@nicira.com> wrote:
> On Sun, Sep 7, 2014 at 2:24 AM, Li RongQing <roy.qing.li@gmail.com> wrote:
>> On Sun, Sep 7, 2014 at 11:44 AM, Pravin Shelar <pshelar@nicira.com> wrote:
>>>> The operation of atomic maybe faster than spin lock.
>>>
>>> What is reason for this change?
>>
>> 1.  The operation of atomic maybe faster than spin lock
>> 2.  I did not find that tx_dropped/tx_error/.. is protected by spin
>> lock under net dir,
>> sometime tx_dropped is atomic_long_t; sometime it is percpu variable;
>> sometime it is
>> u64,but does not need to protect.
>>
>
> These are error counter and the access is not performance sensitive
> code. So I do not see obvious need to optimize it. Do you have any
> performance number for this patch?


I have no performance number, and did not know how to get the performance
number, since I did not know how to trigger the error packet continually.

But I think  atomic is suitable for this condition, it maybe
over-skill to use a spin
lock to protect a single variable, and using atomic can save a spin lock space.


-Roy

Re: [PATCH][net-next] openvswitch: change the data type of error status to atomic_long_t

[David Miller]

From: roy.qing.li@gmail.com
Date: Sat,  6 Sep 2014 19:06:11 +0800

> From: Li RongQing <roy.qing.li@gmail.com>
> 
> Change the date type of error status from u64 to atomic_long_t, and use atomic
> operation, then remove the lock which is used to protect the error status.
> 
> The operation of atomic maybe faster than spin lock.
> 
> Cc: Pravin Shelar <pshelar@nicira.com>
> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>

I think in the final analysis this is a good change, it shrinks a
datastructure by eliminating a spinlock, and the performance impact
is a non-argument since these are not happening in performance
critical paths.

So I am going to apply this, thanks.