Messages in this thread | | | From | Vlad Buslov <> | Subject | Re: [EXT] Re: [v3,net-next 1/4] net: qos: introduce a gate control flow action | Date | Thu, 23 Apr 2020 14:14:39 +0300 |
| |
On Thu 23 Apr 2020 at 12:15, Po Liu <po.liu@nxp.com> wrote: > Hi Vlad Buslov, > >> > >> > +static enum hrtimer_restart gate_timer_func(struct hrtimer *timer) >> { >> > >> > + struct gate_action *gact = container_of(timer, struct >> gate_action, >> > >> > + hitimer); >> > >> > + struct tcf_gate_params *p = get_gate_param(gact); >> > >> > + struct tcfg_gate_entry *next; >> > >> > + ktime_t close_time, now; >> > >> > + >> > >> > + spin_lock(&gact->entry_lock); >> > >> > + >> > >> > + next = rcu_dereference_protected(gact->next_entry, >> > >> > + >> > >> > + lockdep_is_held(&gact->entry_lock)); >> > >> > + >> > >> > + /* cycle start, clear pending bit, clear total octets */ >> > >> > + gact->current_gate_status = next->gate_state ? >> > >> GATE_ACT_GATE_OPEN : 0; >> > >> > + gact->current_entry_octets = 0; >> > >> > + gact->current_max_octets = next->maxoctets; >> > >> > + >> > >> > + gact->current_close_time = ktime_add_ns(gact- >> > >current_close_time, >> > >> > + next->interval); >> > >> > + >> > >> > + close_time = gact->current_close_time; >> > >> > + >> > >> > + if (list_is_last(&next->list, &p->entries)) >> > >> > + next = list_first_entry(&p->entries, >> > >> > + struct tcfg_gate_entry, list); >> > >> > + else >> > >> > + next = list_next_entry(next, list); >> > >> > + >> > >> > + now = gate_get_time(gact); >> > >> > + >> > >> > + if (ktime_after(now, close_time)) { >> > >> > + ktime_t cycle, base; >> > >> > + u64 n; >> > >> > + >> > >> > + cycle = p->tcfg_cycletime; >> > >> > + base = ns_to_ktime(p->tcfg_basetime); >> > >> > + n = div64_u64(ktime_sub_ns(now, base), cycle); >> > >> > + close_time = ktime_add_ns(base, (n + 1) * cycle); >> > >> > + } >> > >> > + >> > >> > + rcu_assign_pointer(gact->next_entry, next); >> > >> > + spin_unlock(&gact->entry_lock); >> > >> >> > >> I have couple of question about synchronization here: >> > >> >> > >> - Why do you need next_entry to be rcu pointer? It is only assigned >> > >> here with entry_lock protection and in init code before action is >> > >> visible to concurrent users. I don't see any unlocked rcu-protected >> > >> readers here that could benefit from it. >> > >> >> > >> - Why create dedicated entry_lock instead of using already existing >> > >> per- action tcf_lock? >> > > >> > > Will try to use the tcf_lock for verification. > > I think I added entry_lock was that I can't get the tc_action common parameter in this timer function. If I insist to use the tcf_lock, I have to move the hrtimer to struct tcf_gate which has tc_action common. > What do you think?
Well, if you use tcf_lock instead of rcu to sync with fastpath, the you don't need to implement struct gate_action as standalone object pointed to by rcu pointer from base structure that includes tc_action common. All the necessary data can be included in tcf_gate structure directly and used from both timer and action fastpath. See pedit for example of action that doesn't use rcu for fastpath.
> >> > > The thoughts came from that the timer period arrived then check >> > > through the list and then update next time would take much more >> time. >> > > Action function would be busy when traffic. So use a separate lock >> > > here for >> > > >> > >> >> > >> > + >> > >> > + hrtimer_set_expires(&gact->hitimer, close_time); >> > >> > + >> > >> > + return HRTIMER_RESTART; >> > >> > +} >> > >> > + >> > >> > +static int tcf_gate_act(struct sk_buff *skb, const struct tc_action *a, >> > >> > + struct tcf_result *res) { >> > >> > + struct tcf_gate *g = to_gate(a); >> > >> > + struct gate_action *gact; >> > >> > + int action; >> > >> > + >> > >> > + tcf_lastuse_update(&g->tcf_tm); >> > >> > + bstats_cpu_update(this_cpu_ptr(g->common.cpu_bstats), skb); >> > >> > + >> > >> > + action = READ_ONCE(g->tcf_action); >> > >> > + rcu_read_lock(); >> > >> >> > >> Action fastpath is already rcu read lock protected, you don't need >> > >> to manually obtain it. >> > > >> > > Will be removed. >> > > >> > >> >> > >> > + gact = rcu_dereference_bh(g->actg); >> > >> > + if (unlikely(gact->current_gate_status & GATE_ACT_PENDING)) >> > >> > + { >> > >> >> > >> Can't current_gate_status be concurrently modified by timer callback? >> > >> This function doesn't use entry_lock to synchronize with timer. >> > > >> > > Will try tcf_lock either. >> > > >> > >> >> > >> > + rcu_read_unlock(); >> > >> > + return action; >> > >> > + } >> > >> > + >> > >> > + if (!(gact->current_gate_status & GATE_ACT_GATE_OPEN)) >> > >> >> > >> ...and here >> > >> >> > >> > + goto drop; >> > >> > + >> > >> > + if (gact->current_max_octets >= 0) { >> > >> > + gact->current_entry_octets += qdisc_pkt_len(skb); >> > >> > + if (gact->current_entry_octets > >> > >> > + gact->current_max_octets) { >> > >> >> > >> here also. >> > >> >> > >> > + >> > >> > + qstats_overlimit_inc(this_cpu_ptr(g->common.cpu_qstats)); >> > >> >> > >> Please use tcf_action_inc_overlimit_qstats() and other wrappers for >> > stats. >> > >> Otherwise it will crash if user passes >> > TCA_ACT_FLAGS_NO_PERCPU_STATS >> > >> flag. >> > > >> > > The tcf_action_inc_overlimit_qstats() can't show limit counts in tc >> > > show >> > command. Is there anything need to do? >> > >> > What do you mean? Internally tcf_action_inc_overlimit_qstats() just >> > calls qstats_overlimit_inc, if cpu_qstats percpu counter is not NULL: >> > >> > >> > if (likely(a->cpu_qstats)) { >> > qstats_overlimit_inc(this_cpu_ptr(a->cpu_qstats)); >> > return; >> > } >> > >> > Is there a subtle bug somewhere in this function? >> >> Sorry, I updated using the tcf_action_*, and the counting is ok. I moved >> back to the qstats_overlimit_inc() because tcf_action_* () include the >> spin_lock(&a->tcfa_lock). >> I would update to tcf_action_* () increate. >> >> > >> > > >> > > Br, >> > > Po Liu >> >> Thanks a lot. >> >> Br, >> Po Liu > > Thanks a lot. > > Br, > Po Liu
| |