Hi Or, On Wed, Nov 8, 2017 at 9:20 AM, Or Gerlitz <[email protected]> wrote: > On Wed, Nov 8, 2017 at 9:03 PM, Manish Kurup <[email protected]> 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 <[email protected]> >> Acked-by: Jiri Pirko <[email protected]> >> Signed-off-by: Manish Kurup <[email protected]> >> --- >> 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
