On Thu, Nov 9, 2017 at 12:45 AM, Manish Kurup <[email protected]> wrote: > 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:
>>> @@ -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
