>-----Original Message-----
>From: Cong Wang [mailto:xiyou.wangc...@gmail.com]
>Sent: Thursday, January 26, 2017 1:30 AM
>To: Jiri Pirko <j...@resnulli.us>
>Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>; David Miller
><da...@davemloft.net>; Yotam Gigi <yot...@mellanox.com>; Ido Schimmel
><ido...@mellanox.com>; Elad Raz <el...@mellanox.com>; Nogah Frankel
><nog...@mellanox.com>; Or Gerlitz <ogerl...@mellanox.com>; Jamal Hadi Salim
><j...@mojatatu.com>; geert+rene...@glider.be; Stephen Hemminger
><step...@networkplumber.org>; Guenter Roeck <li...@roeck-us.net>; Roopa
>Prabhu <ro...@cumulusnetworks.com>; John Fastabend
><john.fastab...@gmail.com>; Simon Horman <simon.hor...@netronome.com>;
>Roman Mashak <m...@mojatatu.com>
>Subject: Re: [patch net-next v2 2/4] net/sched: Introduce sample tc action
>
>On Mon, Jan 23, 2017 at 2:07 AM, Jiri Pirko <j...@resnulli.us> wrote:
>> +
>> +static int tcf_sample_init(struct net *net, struct nlattr *nla,
>> + struct nlattr *est, struct tc_action **a, int ovr,
>> + int bind)
>> +{
>> + struct tc_action_net *tn = net_generic(net, sample_net_id);
>> + struct nlattr *tb[TCA_SAMPLE_MAX + 1];
>> + struct psample_group *psample_group;
>> + struct tc_sample *parm;
>> + struct tcf_sample *s;
>> + bool exists = false;
>> + int ret;
>> +
>> + if (!nla)
>> + return -EINVAL;
>> + ret = nla_parse_nested(tb, TCA_SAMPLE_MAX, nla, sample_policy);
>> + if (ret < 0)
>> + return ret;
>> + if (!tb[TCA_SAMPLE_PARMS] || !tb[TCA_SAMPLE_RATE] ||
>> + !tb[TCA_SAMPLE_PSAMPLE_GROUP])
>> + return -EINVAL;
>> +
>> + parm = nla_data(tb[TCA_SAMPLE_PARMS]);
>> +
>> + exists = tcf_hash_check(tn, parm->index, a, bind);
>> + if (exists && bind)
>> + return 0;
>> +
>> + if (!exists) {
>> + ret = tcf_hash_create(tn, parm->index, est, a,
>> + &act_sample_ops, bind, false);
>> + if (ret)
>> + return ret;
>> + ret = ACT_P_CREATED;
>> + } else {
>> + tcf_hash_release(*a, bind);
>> + if (!ovr)
>> + return -EEXIST;
>> + }
>> + s = to_sample(*a);
>> +
>> + ASSERT_RTNL();
>
>Copy-n-paste from mirred aciton? This is not needed for you, mirred
>needs it because of target netdevice.
Ho, you are right. I will remove it.
>
>
>> + s->tcf_action = parm->action;
>> + s->rate = nla_get_u32(tb[TCA_SAMPLE_RATE]);
>> + s->psample_group_num =
>nla_get_u32(tb[TCA_SAMPLE_PSAMPLE_GROUP]);
>> + psample_group = psample_group_get(net, s->psample_group_num);
>> + if (!psample_group)
>> + return -ENOMEM;
>
>I don't think you can just return here, needs tcf_hash_cleanup() for create
>case, right?
Will fix.
>
>
>> + RCU_INIT_POINTER(s->psample_group, psample_group);
>> +
>> + if (tb[TCA_SAMPLE_TRUNC_SIZE]) {
>> + s->truncate = true;
>> + s->trunc_size = nla_get_u32(tb[TCA_SAMPLE_TRUNC_SIZE]);
>> + }
>
>
>Do you need tcf_lock here if RCU only protects ->psample_group??
>
You are right. I need to protect in case of update.
I will send a fixup patch in the following days. Thanks!
>
>> +
>> + if (ret == ACT_P_CREATED)
>> + tcf_hash_insert(tn, *a);
>> + return ret;
>> +}
>> +
>
>
>Thanks.
