On Wed, Sep 7, 2016 at 7:27 PM, Cong Wang <xiyou.wangc...@gmail.com> wrote: > On Wed, Sep 7, 2016 at 1:08 AM, Hadar Hen Zion <had...@mellanox.com> wrote: >> +struct tcf_tunnel_key_params { >> + struct rcu_head rcu; >> + int tcft_action; >> + int action; >> + struct metadata_dst *tcft_enc_metadata; >> +}; >> + >> +struct tcf_tunnel_key { >> + struct tc_action common; >> + struct tcf_tunnel_key_params __rcu *params; >> +}; >> + > ... > > This is unnecessary if we make the tc action API aware of RCU. > >> + >> +static void tunnel_key_release(struct tc_action *a, int bind) >> +{ >> + struct tcf_tunnel_key *t = to_tunnel_key(a); >> + struct tcf_tunnel_key_params *params; >> + >> + rcu_read_lock(); >> + params = rcu_dereference(t->params); >> + >> + if (params->tcft_action == TCA_TUNNEL_KEY_ACT_SET) >> + dst_release(¶ms->tcft_enc_metadata->dst); >> + >> + rcu_read_unlock(); >> + > > So you allocate memory for t->params in ->init() but not > release it here?
Right, I'll fix it in the next version. > > Also, ->cleanup() should be called with RTNL, no need to > take read lock here. RTNL lock isn't taken when cleanup is called. > > BTW, again you do NOT need to make it RCU, the whole > tc action API should be, as my patchset does, I will take care > of this as a part of my patchset. Eric is wasting your time on > this, with no benefits, the code will be replaced soon.