lkml.org 
[lkml]   [2020]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [EXT] Re: [RFC,net-next 2/9] net: qos: introduce a gate control flow action
Date
Hi Cong,

> -----Original Message-----
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Sent: 2020年3月14日 2:37
> To: Po Liu <po.liu@nxp.com>
> Cc: David Miller <davem@davemloft.net>; LKML <linux-
> kernel@vger.kernel.org>; Linux Kernel Network Developers
> <netdev@vger.kernel.org>; Vinicius Costa Gomes
> <vinicius.gomes@intel.com>; Claudiu Manoil <claudiu.manoil@nxp.com>;
> Vladimir Oltean <vladimir.oltean@nxp.com>; Alexandru Marginean
> <alexandru.marginean@nxp.com>; Xiaoliang Yang
> <xiaoliang.yang_1@nxp.com>; Roy Zang <roy.zang@nxp.com>; Mingkai Hu
> <mingkai.hu@nxp.com>; Jerry Huang <jerry.huang@nxp.com>; Leo Li
> <leoyang.li@nxp.com>; michael.chan@broadcom.com; vishal@chelsio.com;
> Saeed Mahameed <saeedm@mellanox.com>; Leon Romanovsky
> <leon@kernel.org>; Jiri Pirko <jiri@mellanox.com>; Ido Schimmel
> <idosch@mellanox.com>; alexandre.belloni@bootlin.com;
> UNGLinuxDriver@microchip.com; Jakub Kicinski <kuba@kernel.org>; Jamal
> Hadi Salim <jhs@mojatatu.com>; John Hurley
> <john.hurley@netronome.com>; Simon Horman
> <simon.horman@netronome.com>; Pieter Jansen van Vuuren
> <pieter.jansenvanvuuren@netronome.com>; Pablo Neira Ayuso
> <pablo@netfilter.org>; moshe@mellanox.com;
> ivan.khoronzhuk@linaro.org; Murali Karicheri <m-karicheri2@ti.com>;
> andre.guedes@linux.intel.com; Jakub Kicinski
> <jakub.kicinski@netronome.com>
> Subject: [EXT] Re: [RFC,net-next 2/9] net: qos: introduce a gate control flow
> action
>
> Caution: EXT Email
>
> On Fri, Mar 6, 2020 at 5:14 AM Po Liu <Po.Liu@nxp.com> wrote:
> > +static int parse_gate_list(struct nlattr *list,
> > + struct tcf_gate_params *sched,
> > + struct netlink_ext_ack *extack) {
> > + struct nlattr *n;
> > + int err, rem;
> > + int i = 0;
> > +
> > + if (!list)
> > + return -EINVAL;
> > +
> > + nla_for_each_nested(n, list, rem) {
> > + struct tcfg_gate_entry *entry;
> > +
> > + if (nla_type(n) != TCA_GATE_ONE_ENTRY) {
> > + NL_SET_ERR_MSG(extack, "Attribute isn't type 'entry'");
> > + continue;
> > + }
> > +
> > + entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> > + if (!entry) {
> > + NL_SET_ERR_MSG(extack, "Not enough memory for
> entry");
> > + return -ENOMEM;
> > + }
>
> You need to free all previous allocated entries on this list when an error
> happens, right?

Yes, just as Vinicius has mentioned. I'll fix it next version.

>
>
> > +
> > + err = parse_gate_entry(n, entry, i, extack);
> > + if (err < 0) {
> > + kfree(entry);
> > + return err;
> > + }
> > +
> > + list_add_tail(&entry->list, &sched->entries);
> > + i++;
> > + }
> > +
> > + sched->num_entries = i;
> > +
> > + return i;
> > +}
> > +
> > +static int tcf_gate_init(struct net *net, struct nlattr *nla,
> > + struct nlattr *est, struct tc_action **a,
> > + int ovr, int bind, bool rtnl_held,
> > + struct tcf_proto *tp, u32 flags,
> > + struct netlink_ext_ack *extack) {
> > + struct tc_action_net *tn = net_generic(net, gate_net_id);
> > + enum tk_offsets tk_offset = TK_OFFS_TAI;
> > + struct nlattr *tb[TCA_GATE_MAX + 1];
> > + struct tcf_chain *goto_ch = NULL;
> > + struct tcfg_gate_entry *next;
> > + struct tcf_gate_params *p;
> > + struct gate_action *gact;
> > + s32 clockid = CLOCK_TAI;
> > + struct tc_gate *parm;
> > + struct tcf_gate *g;
> > + int ret = 0, err;
> > + u64 basetime = 0;
> > + u32 gflags = 0;
> > + s32 prio = -1;
> > + ktime_t start;
> > + u32 index;
> > +
> > + if (!nla)
> > + return -EINVAL;
> > +
> > + err = nla_parse_nested_deprecated(tb, TCA_GATE_MAX,
> > + nla, gate_policy, NULL);
> > + if (err < 0)
> > + return err;
> > +
> > + if (!tb[TCA_GATE_PARMS])
> > + return -EINVAL;
> > + parm = nla_data(tb[TCA_GATE_PARMS]);
> > + index = parm->index;
> > + err = tcf_idr_check_alloc(tn, &index, a, bind);
> > + if (err < 0)
> > + return err;
> > +
> > + if (err && bind)
> > + return 0;
> > +
> > + if (!err) {
> > + ret = tcf_idr_create_from_flags(tn, index, est, a,
> > + &act_gate_ops, bind, flags);
> > + if (ret) {
> > + tcf_idr_cleanup(tn, index);
> > + return ret;
> > + }
> > +
> > + ret = ACT_P_CREATED;
> > + } else if (!ovr) {
> > + tcf_idr_release(*a, bind);
> > + return -EEXIST;
> > + }
> > +
> > + if (tb[TCA_GATE_PRIORITY])
> > + prio = nla_get_s32(tb[TCA_GATE_PRIORITY]);
> > +
> > + if (tb[TCA_GATE_BASE_TIME])
> > + basetime = nla_get_u64(tb[TCA_GATE_BASE_TIME]);
> > +
> > + if (tb[TCA_GATE_FLAGS])
> > + gflags = nla_get_u32(tb[TCA_GATE_FLAGS]);
> > +
> > + if (tb[TCA_GATE_CLOCKID]) {
> > + clockid = nla_get_s32(tb[TCA_GATE_CLOCKID]);
> > + switch (clockid) {
> > + case CLOCK_REALTIME:
> > + tk_offset = TK_OFFS_REAL;
> > + break;
> > + case CLOCK_MONOTONIC:
> > + tk_offset = TK_OFFS_MAX;
> > + break;
> > + case CLOCK_BOOTTIME:
> > + tk_offset = TK_OFFS_BOOT;
> > + break;
> > + case CLOCK_TAI:
> > + tk_offset = TK_OFFS_TAI;
> > + break;
> > + default:
> > + NL_SET_ERR_MSG(extack, "Invalid 'clockid'");
> > + goto release_idr;
> > + }
> > + }
> > +
> > + err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
> > + if (err < 0)
> > + goto release_idr;
> > +
> > + g = to_gate(*a);
> > +
> > + gact = kzalloc(sizeof(*gact), GFP_KERNEL);
> > + if (!gact) {
> > + err = -ENOMEM;
> > + goto put_chain;
> > + }
> > +
> > + p = get_gate_param(gact);
> > +
> > + INIT_LIST_HEAD(&p->entries);
> > + if (tb[TCA_GATE_ENTRY_LIST]) {
> > + err = parse_gate_list(tb[TCA_GATE_ENTRY_LIST], p, extack);
> > + if (err < 0)
> > + goto release_mem;
> > + }
> > +
> > + if (tb[TCA_GATE_CYCLE_TIME]) {
> > + p->tcfg_cycletime = nla_get_u64(tb[TCA_GATE_CYCLE_TIME]);
> > + } else {
> > + struct tcfg_gate_entry *entry;
> > + ktime_t cycle = 0;
> > +
> > + list_for_each_entry(entry, &p->entries, list)
> > + cycle = ktime_add_ns(cycle, entry->interval);
> > + p->tcfg_cycletime = cycle;
> > + }
> > +
> > + if (tb[TCA_GATE_CYCLE_TIME_EXT])
> > + p->tcfg_cycletime_ext =
> > + nla_get_u64(tb[TCA_GATE_CYCLE_TIME_EXT]);
> > +
> > + p->tcfg_priority = prio;
> > + p->tcfg_basetime = basetime;
> > + p->tcfg_clockid = clockid;
> > + p->tcfg_flags = gflags;
> > +
> > + gact->tk_offset = tk_offset;
> > + spin_lock_init(&gact->entry_lock);
> > + hrtimer_init(&gact->hitimer, clockid, HRTIMER_MODE_ABS);
> > + gact->hitimer.function = gate_timer_func;
> > +
> > + err = gate_get_start_time(gact, &start);
> > + if (err < 0) {
> > + NL_SET_ERR_MSG(extack,
> > + "Internal error: failed get start time");
> > + goto release_mem;
> > + }
> > +
> > + gact->current_close_time = start;
> > + gact->current_gate_status = GATE_ACT_GATE_OPEN |
> > + GATE_ACT_PENDING;
> > +
> > + next = list_first_entry(&p->entries, struct tcfg_gate_entry, list);
> > + rcu_assign_pointer(gact->next_entry, next);
> > +
> > + gate_start_timer(gact, start);
> > +
> > + spin_lock_bh(&g->tcf_lock);
> > + goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> > + gact = rcu_replace_pointer(g->actg, gact,
> > + lockdep_is_held(&g->tcf_lock));
> > + spin_unlock_bh(&g->tcf_lock);
> > +
> > + if (goto_ch)
> > + tcf_chain_put_by_act(goto_ch);
> > + if (gact)
> > + kfree_rcu(gact, rcu);
> > +
> > + if (ret == ACT_P_CREATED)
> > + tcf_idr_insert(tn, *a);
> > + return ret;
> > +
> > +release_mem:
> > + kfree_rcu(gact, rcu);
>
> No need to bother RCU for an error path, right?

Yes, the kfree_rcu() change to kfree() is proper.

>
> Thanks.


Thanks!
Br,
Po Liu

\
 
 \ /
  Last update: 2020-03-15 02:40    [W:0.244 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site