* [PATCH net-next v10 1/3] act_vlan: Change stats update to use per-core stats
@ 2017-11-08 12:03 Manish Kurup
2017-11-08 14:20 ` Or Gerlitz
0 siblings, 1 reply; 5+ messages in thread
From: Manish Kurup @ 2017-11-08 12:03 UTC (permalink / raw)
To: jhs, xiyou.wangcong, jiri, davem, jakub.kicinski,
pieter.jansenvanvuuren, simon.horman, john.hurley, oss-drivers,
netdev
Cc: aring, mrv, kurup.manish, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v10 1/3] act_vlan: Change stats update to use per-core stats
2017-11-08 12:03 [PATCH net-next v10 1/3] act_vlan: Change stats update to use per-core stats Manish Kurup
@ 2017-11-08 14:20 ` Or Gerlitz
2017-11-08 15:45 ` Manish Kurup
0 siblings, 1 reply; 5+ messages in thread
From: Or Gerlitz @ 2017-11-08 14:20 UTC (permalink / raw)
To: Manish Kurup
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David Miller,
Jakub Kicinski, Pieter Jansen van Vuuren, Simon Horman,
John Hurley, oss-drivers, Linux Netdev List, aring, Roman Mashak,
Manish Kurup
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?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v10 1/3] act_vlan: Change stats update to use per-core stats
2017-11-08 14:20 ` Or Gerlitz
@ 2017-11-08 15:45 ` Manish Kurup
2017-11-08 21:40 ` Or Gerlitz
0 siblings, 1 reply; 5+ messages in thread
From: Manish Kurup @ 2017-11-08 15:45 UTC (permalink / raw)
To: Or Gerlitz
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David Miller,
Jakub Kicinski, Pieter Jansen van Vuuren, Simon Horman,
John Hurley, oss-drivers, Linux Netdev List, Alexander Aring,
Roman Mashak, 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v10 1/3] act_vlan: Change stats update to use per-core stats
2017-11-08 15:45 ` Manish Kurup
@ 2017-11-08 21:40 ` Or Gerlitz
2017-11-08 23:42 ` David Miller
0 siblings, 1 reply; 5+ messages in thread
From: Or Gerlitz @ 2017-11-08 21:40 UTC (permalink / raw)
To: Manish Kurup
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David Miller,
Jakub Kicinski, Pieter Jansen van Vuuren, Simon Horman,
John Hurley, oss-drivers, Linux Netdev List, Alexander Aring,
Roman Mashak, Manish Kurup
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v10 1/3] act_vlan: Change stats update to use per-core stats
2017-11-08 21:40 ` Or Gerlitz
@ 2017-11-08 23:42 ` David Miller
0 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2017-11-08 23:42 UTC (permalink / raw)
To: gerlitz.or
Cc: kurup.manish, jhs, xiyou.wangcong, jiri, jakub.kicinski,
pieter.jansenvanvuuren, simon.horman, john.hurley, oss-drivers,
netdev, aring, mrv, manish.kurup
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-08 23:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-08 12:03 [PATCH net-next v10 1/3] act_vlan: Change stats update to use per-core stats Manish Kurup
2017-11-08 14:20 ` Or Gerlitz
2017-11-08 15:45 ` Manish Kurup
2017-11-08 21:40 ` Or Gerlitz
2017-11-08 23:42 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.