[PATCH net-next v10 1/3] act_vlan: Change stats update to use per-core stats

[PATCH net-next v10 1/3] act_vlan: Change stats update to use per-core stats

[Manish Kurup]

The VLAN action maintains one set of stats across all cores, and uses a
spinlock to synchronize updates to it from the same. Changed this to use a
per-CPU stats context instead.
This change will result in better performance.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Manish Kurup <manish.kurup@verizon.com>
---
 net/sched/act_vlan.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 115fc33..8a35efe 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -30,9 +30,10 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
 	int err;
 	u16 tci;
 
-	spin_lock(&v->tcf_lock);
 	tcf_lastuse_update(&v->tcf_tm);
-	bstats_update(&v->tcf_bstats, skb);
+	bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
+
+	spin_lock(&v->tcf_lock);
 	action = v->tcf_action;
 
 	/* Ensure 'data' points at mac_header prior calling vlan manipulating
@@ -85,7 +86,8 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
 
 drop:
 	action = TC_ACT_SHOT;
-	v->tcf_qstats.drops++;
+	qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
+
 unlock:
 	if (skb_at_tc_ingress(skb))
 		skb_pull_rcsum(skb, skb->mac_len);
@@ -172,7 +174,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
 
 	if (!exists) {
 		ret = tcf_idr_create(tn, parm->index, est, a,
-				     &act_vlan_ops, bind, false);
+				     &act_vlan_ops, bind, true);
 		if (ret)
 			return ret;
 
-- 
2.7.4

Re: [PATCH net-next v10 1/3] act_vlan: Change stats update to use per-core stats

[Or Gerlitz]

On Wed, Nov 8, 2017 at 9:03 PM, Manish Kurup <kurup.manish@gmail.com> wrote:
> The VLAN action maintains one set of stats across all cores, and uses a
> spinlock to synchronize updates to it from the same. Changed this to use a
> per-CPU stats context instead.
> This change will result in better performance.
>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Acked-by: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Manish Kurup <manish.kurup@verizon.com>
> ---
>  net/sched/act_vlan.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> index 115fc33..8a35efe 100644
> --- a/net/sched/act_vlan.c
> +++ b/net/sched/act_vlan.c
> @@ -30,9 +30,10 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
>         int err;
>         u16 tci;
>
> -       spin_lock(&v->tcf_lock);
>         tcf_lastuse_update(&v->tcf_tm);
> -       bstats_update(&v->tcf_bstats, skb);
> +       bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
> +
> +       spin_lock(&v->tcf_lock);
>         action = v->tcf_action;

(if this was asked && answered in earlier Vs, sorry for that, if not and I got
some small real problem here && you're @ netdev, maybe buy me Korean beer?)

before your changes the spin lock also protected the lastuse update call but
now it doesn't, why?

Re: [PATCH net-next v10 1/3] act_vlan: Change stats update to use per-core stats

[Manish Kurup]

Hi Or,

On Wed, Nov 8, 2017 at 9:20 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Wed, Nov 8, 2017 at 9:03 PM, Manish Kurup <kurup.manish@gmail.com> wrote:
>> The VLAN action maintains one set of stats across all cores, and uses a
>> spinlock to synchronize updates to it from the same. Changed this to use a
>> per-CPU stats context instead.
>> This change will result in better performance.
>>
>> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> Acked-by: Jiri Pirko <jiri@mellanox.com>
>> Signed-off-by: Manish Kurup <manish.kurup@verizon.com>
>> ---
>>  net/sched/act_vlan.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
>> index 115fc33..8a35efe 100644
>> --- a/net/sched/act_vlan.c
>> +++ b/net/sched/act_vlan.c
>> @@ -30,9 +30,10 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
>>         int err;
>>         u16 tci;
>>
>> -       spin_lock(&v->tcf_lock);
>>         tcf_lastuse_update(&v->tcf_tm);
>> -       bstats_update(&v->tcf_bstats, skb);
>> +       bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
>> +
>> +       spin_lock(&v->tcf_lock);
>>         action = v->tcf_action;
>
> (if this was asked && answered in earlier Vs, sorry for that, if not and I got
> some small real problem here && you're @ netdev, maybe buy me Korean beer?)
>
> before your changes the spin lock also protected the lastuse update call but
> now it doesn't, why?
Phase I of my changes, was to get rid of spin_locks, and convert the
stats to a per-cpu stats model to get better forwarding performance.
While doing this, I looked at a few 'model TC actions' within
net/sched (tcf_mirred for example). Neither of them protected the
tcf_lastuse_update(). I assumed that this was the case because this
was a 'display-only' field, and as long as it changed to a latest
timestamp based on packets received, it was OK.

I tested this using our suite of traffic tests, and verified that the
last-use field did update, and did not cause any other problems.

Do you envision any issues that could be caused due to this?

Thanks,

-Manish

Re: [PATCH net-next v10 1/3] act_vlan: Change stats update to use per-core stats

[Or Gerlitz]

On Thu, Nov 9, 2017 at 12:45 AM, Manish Kurup <kurup.manish@gmail.com> wrote:
> On Wed, Nov 8, 2017 at 9:20 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>> On Wed, Nov 8, 2017 at 9:03 PM, Manish Kurup <kurup.manish@gmail.com> wrote:

>>> @@ -30,9 +30,10 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
>>>         int err;
>>>         u16 tci;
>>>
>>> -       spin_lock(&v->tcf_lock);
>>>         tcf_lastuse_update(&v->tcf_tm);
>>> -       bstats_update(&v->tcf_bstats, skb);
>>> +       bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
>>> +
>>> +       spin_lock(&v->tcf_lock);
>>>         action = v->tcf_action;
>>

>> before your changes the spin lock also protected the lastuse update call but
>> now it doesn't, why?

> Phase I of my changes, was to get rid of spin_locks, and convert the
> stats to a per-cpu stats model to get better forwarding performance.
> While doing this, I looked at a few 'model TC actions' within
> net/sched (tcf_mirred for example). Neither of them protected the
> tcf_lastuse_update(). I assumed that this was the case because this
> was a 'display-only' field, and as long as it changed to a latest
> timestamp based on packets received, it was OK.

this is really late in the review cycle so lets not stop for that but
if for some  reason there's V11 - would be good to put a comment on
that in the change log

Re: [PATCH net-next v10 1/3] act_vlan: Change stats update to use per-core stats

[David Miller]

From: Or Gerlitz <gerlitz.or@gmail.com>
Date: Thu, 9 Nov 2017 06:40:06 +0900

> On Thu, Nov 9, 2017 at 12:45 AM, Manish Kurup <kurup.manish@gmail.com> wrote:
>> On Wed, Nov 8, 2017 at 9:20 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>> On Wed, Nov 8, 2017 at 9:03 PM, Manish Kurup <kurup.manish@gmail.com> wrote:
> 
>>>> @@ -30,9 +30,10 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
>>>>         int err;
>>>>         u16 tci;
>>>>
>>>> -       spin_lock(&v->tcf_lock);
>>>>         tcf_lastuse_update(&v->tcf_tm);
>>>> -       bstats_update(&v->tcf_bstats, skb);
>>>> +       bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
>>>> +
>>>> +       spin_lock(&v->tcf_lock);
>>>>         action = v->tcf_action;
>>>
> 
>>> before your changes the spin lock also protected the lastuse update call but
>>> now it doesn't, why?
> 
>> Phase I of my changes, was to get rid of spin_locks, and convert the
>> stats to a per-cpu stats model to get better forwarding performance.
>> While doing this, I looked at a few 'model TC actions' within
>> net/sched (tcf_mirred for example). Neither of them protected the
>> tcf_lastuse_update(). I assumed that this was the case because this
>> was a 'display-only' field, and as long as it changed to a latest
>> timestamp based on packets received, it was OK.
> 
> this is really late in the review cycle so lets not stop for that but
> if for some  reason there's V11 - would be good to put a comment on
> that in the change log

I think the async update of this lastuse value should be fine.