Messages in this thread | | | From | Po Liu <> | Subject | RE: [EXT] Re: [v3,net-next 1/4] net: qos: introduce a gate control flow action | Date | Thu, 23 Apr 2020 08:32:55 +0000 |
| |
> -----Original Message----- > From: Vlad Buslov <vlad@buslov.dev> > Sent: 2020年4月23日 15:43 > To: Po Liu <po.liu@nxp.com> > Cc: Vlad Buslov <vlad@buslov.dev>; davem@davemloft.net; linux- > kernel@vger.kernel.org; netdev@vger.kernel.org; > vinicius.gomes@intel.com; Claudiu Manoil <claudiu.manoil@nxp.com>; > Vladimir Oltean <vladimir.oltean@nxp.com>; Alexandru Marginean > <alexandru.marginean@nxp.com>; michael.chan@broadcom.com; > vishal@chelsio.com; saeedm@mellanox.com; leon@kernel.org; > jiri@mellanox.com; idosch@mellanox.com; > alexandre.belloni@bootlin.com; UNGLinuxDriver@microchip.com; > kuba@kernel.org; jhs@mojatatu.com; xiyou.wangcong@gmail.com; > simon.horman@netronome.com; pablo@netfilter.org; > moshe@mellanox.com; m-karicheri2@ti.com; > andre.guedes@linux.intel.com; stephen@networkplumber.org > Subject: Re: [EXT] Re: [v3,net-next 1/4] net: qos: introduce a gate control > flow action > > Caution: EXT Email > > On Thu 23 Apr 2020 at 06:14, Po Liu <po.liu@nxp.com> wrote: > > Hi Vlad Buslov, > > > >> -----Original Message----- > >> From: Vlad Buslov <vlad@buslov.dev> > >> Sent: 2020年4月22日 21:23 > >> To: Po Liu <po.liu@nxp.com> > >> Cc: davem@davemloft.net; linux-kernel@vger.kernel.org; > >> netdev@vger.kernel.org; vinicius.gomes@intel.com; Claudiu Manoil > >> <claudiu.manoil@nxp.com>; Vladimir Oltean > <vladimir.oltean@nxp.com>; > >> Alexandru Marginean <alexandru.marginean@nxp.com>; > >> michael.chan@broadcom.com; vishal@chelsio.com; > saeedm@mellanox.com; > >> leon@kernel.org; jiri@mellanox.com; idosch@mellanox.com; > >> alexandre.belloni@bootlin.com; UNGLinuxDriver@microchip.com; > >> kuba@kernel.org; jhs@mojatatu.com; xiyou.wangcong@gmail.com; > >> simon.horman@netronome.com; pablo@netfilter.org; > moshe@mellanox.com; > >> m-karicheri2@ti.com; andre.guedes@linux.intel.com; > >> stephen@networkplumber.org > >> Subject: [EXT] Re: [v3,net-next 1/4] net: qos: introduce a gate > >> control flow action > >> > >> Caution: EXT Email > >> > >> Hi Po, > >> > >> On Wed 22 Apr 2020 at 05:48, Po Liu <Po.Liu@nxp.com> wrote: > >> > Introduce a ingress frame gate control flow action. > >> > Tc gate action does the work like this: > >> > Assume there is a gate allow specified ingress frames can be passed > >> > at specific time slot, and be dropped at specific time slot. Tc > >> > filter chooses the ingress frames, and tc gate action would specify > >> > what slot does these frames can be passed to device and what time > >> > slot would be dropped. > >> > Tc gate action would provide an entry list to tell how much time > >> > gate keep open and how much time gate keep state close. Gate > action > >> > also assign a start time to tell when the entry list start. Then > >> > driver would repeat the gate entry list cyclically. > >> > For the software simulation, gate action requires the user assign a > >> > time clock type. > >> > > >> > Below is the setting example in user space. Tc filter a stream > >> > source ip address is 192.168.0.20 and gate action own two time > >> > slots. One is last 200ms gate open let frame pass another is last > >> > 100ms gate close let frames dropped. When the frames have passed > >> > total frames over > >> > 8000000 bytes, frames will be dropped in one 200000000ns time slot. > >> > > >> >> tc qdisc add dev eth0 ingress > >> > > >> >> tc filter add dev eth0 parent ffff: protocol ip \ > >> > flower src_ip 192.168.0.20 \ > >> > action gate index 2 clockid CLOCK_TAI \ > >> > sched-entry open 200000000 -1 8000000 \ > >> > sched-entry close 100000000 -1 -1 > >> > > >> >> tc chain del dev eth0 ingress chain 0 > >> > > >> > "sched-entry" follow the name taprio style. Gate state is > >> > "open"/"close". Follow with period nanosecond. Then next item is > >> > internal priority value means which ingress queue should put. "-1" > >> > means wildcard. The last value optional specifies the maximum > >> > number of MSDU octets that are permitted to pass the gate during > >> > the specified time interval. > >> > Base-time is not set will be 0 as default, as result start time > >> > would be ((N + 1) * cycletime) which is the minimal of future time. > >> > > >> > Below example shows filtering a stream with destination mac address > >> > is > >> > 10:00:80:00:00:00 and ip type is ICMP, follow the action gate. The > >> > gate action would run with one close time slot which means always > >> > keep > >> close. > >> > The time cycle is total 200000000ns. The base-time would calculate by: > >> > > >> > 1357000000000 + (N + 1) * cycletime > >> > > >> > When the total value is the future time, it will be the start time. > >> > The cycletime here would be 200000000ns for this case. > >> > > >> >> tc filter add dev eth0 parent ffff: protocol ip \ > >> > flower skip_hw ip_proto icmp dst_mac 10:00:80:00:00:00 \ > >> > action gate index 12 base-time 1357000000000 \ > >> > sched-entry close 200000000 -1 -1 \ > >> > clockid CLOCK_TAI > >> > > >> > Signed-off-by: Po Liu <Po.Liu@nxp.com> > >> > --- > >> > include/net/tc_act/tc_gate.h | 54 +++ > >> > include/uapi/linux/pkt_cls.h | 1 + > >> > include/uapi/linux/tc_act/tc_gate.h | 47 ++ > >> > net/sched/Kconfig | 13 + > >> > net/sched/Makefile | 1 + > >> > net/sched/act_gate.c | 647 > ++++++++++++++++++++++++++++ > >> > 6 files changed, 763 insertions(+) create mode 100644 > >> > include/net/tc_act/tc_gate.h create mode 100644 > >> > include/uapi/linux/tc_act/tc_gate.h > >> > create mode 100644 net/sched/act_gate.c > >> > > >> > diff --git a/include/net/tc_act/tc_gate.h > >> > b/include/net/tc_act/tc_gate.h new file mode 100644 index > >> > 000000000000..b0ace55b2aaa > >> > --- /dev/null > >> > +++ b/include/net/tc_act/tc_gate.h > >> > @@ -0,0 +1,54 @@ > >> > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > >> > +/* Copyright 2020 NXP */ > >> > + > >> > +#ifndef __NET_TC_GATE_H > >> > +#define __NET_TC_GATE_H > >> > + > >> > +#include <net/act_api.h> > >> > +#include <linux/tc_act/tc_gate.h> > >> > + > >> > +struct tcfg_gate_entry { > >> > + int index; > >> > + u8 gate_state; > >> > + u32 interval; > >> > + s32 ipv; > >> > + s32 maxoctets; > >> > + struct list_head list; > >> > +}; > >> > + > >> > +struct tcf_gate_params { > >> > + s32 tcfg_priority; > >> > + u64 tcfg_basetime; > >> > + u64 tcfg_cycletime; > >> > + u64 tcfg_cycletime_ext; > >> > + u32 tcfg_flags; > >> > + s32 tcfg_clockid; > >> > + size_t num_entries; > >> > + struct list_head entries; > >> > +}; > >> > + > >> > +#define GATE_ACT_GATE_OPEN BIT(0) > >> > +#define GATE_ACT_PENDING BIT(1) > >> > +struct gate_action { > >> > + struct tcf_gate_params param; > >> > + spinlock_t entry_lock; > >> > + u8 current_gate_status; > >> > + ktime_t current_close_time; > >> > + u32 current_entry_octets; > >> > + s32 current_max_octets; > >> > + struct tcfg_gate_entry __rcu *next_entry; > >> > + struct hrtimer hitimer; > >> > + enum tk_offsets tk_offset; > >> > + struct rcu_head rcu; > >> > +}; > >> > + > >> > +struct tcf_gate { > >> > + struct tc_action common; > >> > + struct gate_action __rcu *actg; > >> > +}; > >> > +#define to_gate(a) ((struct tcf_gate *)a) > >> > + > >> > +#define get_gate_param(act) ((struct tcf_gate_params *)act) > >> > +#define > >> > +get_gate_action(p) ((struct gate_action *)p) > >> > + > >> > +#endif > >> > diff --git a/include/uapi/linux/pkt_cls.h > >> > b/include/uapi/linux/pkt_cls.h index 9f06d29cab70..fc672b232437 > >> 100644 > >> > --- a/include/uapi/linux/pkt_cls.h > >> > +++ b/include/uapi/linux/pkt_cls.h > >> > @@ -134,6 +134,7 @@ enum tca_id { > >> > TCA_ID_CTINFO, > >> > TCA_ID_MPLS, > >> > TCA_ID_CT, > >> > + TCA_ID_GATE, > >> > /* other actions go here */ > >> > __TCA_ID_MAX = 255 > >> > }; > >> > diff --git a/include/uapi/linux/tc_act/tc_gate.h > >> > b/include/uapi/linux/tc_act/tc_gate.h > >> > new file mode 100644 > >> > index 000000000000..f214b3a6d44f > >> > --- /dev/null > >> > +++ b/include/uapi/linux/tc_act/tc_gate.h > >> > @@ -0,0 +1,47 @@ > >> > +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */ > >> > +/* Copyright 2020 NXP */ > >> > + > >> > +#ifndef __LINUX_TC_GATE_H > >> > +#define __LINUX_TC_GATE_H > >> > + > >> > +#include <linux/pkt_cls.h> > >> > + > >> > +struct tc_gate { > >> > + tc_gen; > >> > +}; > >> > + > >> > +enum { > >> > + TCA_GATE_ENTRY_UNSPEC, > >> > + TCA_GATE_ENTRY_INDEX, > >> > + TCA_GATE_ENTRY_GATE, > >> > + TCA_GATE_ENTRY_INTERVAL, > >> > + TCA_GATE_ENTRY_IPV, > >> > + TCA_GATE_ENTRY_MAX_OCTETS, > >> > + __TCA_GATE_ENTRY_MAX, > >> > +}; > >> > +#define TCA_GATE_ENTRY_MAX (__TCA_GATE_ENTRY_MAX - 1) > >> > + > >> > +enum { > >> > + TCA_GATE_ONE_ENTRY_UNSPEC, > >> > + TCA_GATE_ONE_ENTRY, > >> > + __TCA_GATE_ONE_ENTRY_MAX, > >> > +}; > >> > +#define TCA_GATE_ONE_ENTRY_MAX > (__TCA_GATE_ONE_ENTRY_MAX > >> - 1) > >> > + > >> > +enum { > >> > + TCA_GATE_UNSPEC, > >> > + TCA_GATE_TM, > >> > + TCA_GATE_PARMS, > >> > + TCA_GATE_PAD, > >> > + TCA_GATE_PRIORITY, > >> > + TCA_GATE_ENTRY_LIST, > >> > + TCA_GATE_BASE_TIME, > >> > + TCA_GATE_CYCLE_TIME, > >> > + TCA_GATE_CYCLE_TIME_EXT, > >> > + TCA_GATE_FLAGS, > >> > + TCA_GATE_CLOCKID, > >> > + __TCA_GATE_MAX, > >> > +}; > >> > +#define TCA_GATE_MAX (__TCA_GATE_MAX - 1) > >> > + > >> > +#endif > >> > diff --git a/net/sched/Kconfig b/net/sched/Kconfig index > >> > bfbefb7bff9d..1314549c7567 100644 > >> > --- a/net/sched/Kconfig > >> > +++ b/net/sched/Kconfig > >> > @@ -981,6 +981,19 @@ config NET_ACT_CT > >> > To compile this code as a module, choose M here: the > >> > module will be called act_ct. > >> > > >> > +config NET_ACT_GATE > >> > + tristate "Frame gate entry list control tc action" > >> > + depends on NET_CLS_ACT > >> > + help > >> > + Say Y here to allow to control the ingress flow to be passed at > >> > + specific time slot and be dropped at other specific time slot by > >> > + the gate entry list. The manipulation will simulate the IEEE > >> > + 802.1Qci stream gate control behavior. > >> > + > >> > + If unsure, say N. > >> > + To compile this code as a module, choose M here: the > >> > + module will be called act_gate. > >> > + > >> > config NET_IFE_SKBMARK > >> > tristate "Support to encoding decoding skb mark on IFE action" > >> > depends on NET_ACT_IFE > >> > diff --git a/net/sched/Makefile b/net/sched/Makefile index > >> > 31c367a6cd09..66bbf9a98f9e 100644 > >> > --- a/net/sched/Makefile > >> > +++ b/net/sched/Makefile > >> > @@ -30,6 +30,7 @@ obj-$(CONFIG_NET_IFE_SKBPRIO) += > >> act_meta_skbprio.o > >> > obj-$(CONFIG_NET_IFE_SKBTCINDEX) += act_meta_skbtcindex.o > >> > obj-$(CONFIG_NET_ACT_TUNNEL_KEY)+= act_tunnel_key.o > >> > obj-$(CONFIG_NET_ACT_CT) += act_ct.o > >> > +obj-$(CONFIG_NET_ACT_GATE) += act_gate.o > >> > obj-$(CONFIG_NET_SCH_FIFO) += sch_fifo.o > >> > obj-$(CONFIG_NET_SCH_CBQ) += sch_cbq.o > >> > obj-$(CONFIG_NET_SCH_HTB) += sch_htb.o > >> > diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c new file > >> > mode > >> > 100644 index 000000000000..e932f402b4f1 > >> > --- /dev/null > >> > +++ b/net/sched/act_gate.c > >> > @@ -0,0 +1,647 @@ > >> > +// SPDX-License-Identifier: GPL-2.0-or-later > >> > +/* Copyright 2020 NXP */ > >> > + > >> > +#include <linux/module.h> > >> > +#include <linux/types.h> > >> > +#include <linux/kernel.h> > >> > +#include <linux/string.h> > >> > +#include <linux/errno.h> > >> > +#include <linux/skbuff.h> > >> > +#include <linux/rtnetlink.h> > >> > +#include <linux/init.h> > >> > +#include <linux/slab.h> > >> > +#include <net/act_api.h> > >> > +#include <net/netlink.h> > >> > +#include <net/pkt_cls.h> > >> > +#include <net/tc_act/tc_gate.h> > >> > + > >> > +static unsigned int gate_net_id; > >> > +static struct tc_action_ops act_gate_ops; > >> > + > >> > +static ktime_t gate_get_time(struct gate_action *gact) { > >> > + ktime_t mono = ktime_get(); > >> > + > >> > + switch (gact->tk_offset) { > >> > + case TK_OFFS_MAX: > >> > + return mono; > >> > + default: > >> > + return ktime_mono_to_any(mono, gact->tk_offset); > >> > + } > >> > + > >> > + return KTIME_MAX; > >> > +} > >> > + > >> > +static int gate_get_start_time(struct gate_action *gact, ktime_t > >> > +*start) { > >> > + struct tcf_gate_params *param = get_gate_param(gact); > >> > + ktime_t now, base, cycle; > >> > + u64 n; > >> > + > >> > + base = ns_to_ktime(param->tcfg_basetime); > >> > + now = gate_get_time(gact); > >> > + > >> > + if (ktime_after(base, now)) { > >> > + *start = base; > >> > + return 0; > >> > + } > >> > + > >> > + cycle = param->tcfg_cycletime; > >> > + > >> > + /* cycle time should not be zero */ > >> > + if (WARN_ON(!cycle)) > >> > + return -EFAULT; > >> > >> Looking at the init code it seems that this value can be set to 0 > >> directly from netlink packet without further validation, which would > >> allow user to trigger warning here. > > > > Yes, will avoid at ahead point. > > > >> > >> > + > >> > + n = div64_u64(ktime_sub_ns(now, base), cycle); > >> > + *start = ktime_add_ns(base, (n + 1) * cycle); > >> > + return 0; > >> > +} > >> > + > >> > +static void gate_start_timer(struct gate_action *gact, ktime_t > >> > +start) { > >> > + ktime_t expires; > >> > + > >> > + expires = hrtimer_get_expires(&gact->hitimer); > >> > + if (expires == 0) > >> > + expires = KTIME_MAX; > >> > + > >> > + start = min_t(ktime_t, start, expires); > >> > + > >> > + hrtimer_start(&gact->hitimer, start, HRTIMER_MODE_ABS); } > >> > + > >> > +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. > > 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
| |