Message ID | 1510142600-25834-1-git-send-email-manish.kurup@verizon.com |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
Series | net_sched actions: act_vlan now uses RCU | expand |
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?
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
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
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.
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;