From: Or Gerlitz <gerlitz...@gmail.com> Date: Thu, 9 Nov 2017 06:40:06 +0900
> On Thu, Nov 9, 2017 at 12:45 AM, Manish Kurup <kurup.man...@gmail.com> wrote: >> On Wed, Nov 8, 2017 at 9:20 AM, Or Gerlitz <gerlitz...@gmail.com> wrote: >>> On Wed, Nov 8, 2017 at 9:03 PM, Manish Kurup <kurup.man...@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.
