>-----Original Message----- >From: Yotam Gigi >Sent: Friday, January 27, 2017 8:09 AM >To: 'Cong Wang' <xiyou.wangc...@gmail.com>; Jiri Pirko <j...@resnulli.us> >Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>; David Miller ><da...@davemloft.net>; 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 > >>-----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.
Cong, after some thinking I think I don't really need the tcf_lock here. I don't really care if the truncate, trunc_size, rate and tcf_action are consistent among themselves - the only parameter that I care about is the psample_group pointer, and it is protected via RCU. As far as I see, there is no need to lock here. I do need to take the tcf_lock to protect the statistics update in the tcf_sample_act code, as far as I see. Am I missing something? > >I will send a fixup patch in the following days. Thanks! > >> >>> + >>> + if (ret == ACT_P_CREATED) >>> + tcf_hash_insert(tn, *a); >>> + return ret; >>> +} >>> + >> >> >>Thanks.
