RE: [patch net-next v2 2/4] net/sched: Introduce sample tc action

[Yotam Gigi]

>-----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.