On Wed, Mar 20, 2019 at 7:00 AM Davide Caratti <dcara...@redhat.com> wrote: > Changes since v1: > - reword the cover letter > - condense the extack message in case tc_action_check_ctrlact() is called > with invalid parameters. > - add tcf_action_set_ctrlact() to avoid code duplication an make the > RCU-ification of 'goto_chain' easier. > - fix errors in act_ife, act_simple, act_skbedit, and avoid useless 'goto > end' in act_connmark, thanks a lot to Vlad Buslov. > - avoid dereferencing 'goto_chain' in tcf_gact_goto_chain_index(), so > we don't have to care about the grace period there.
Hmm? goto_chain is dereferenced in tcf_action_goto_chain_exec() which is the fast path, which means you still have to care about RCU grace period, right? > - let actions respect the grace period when they release chains, thanks > to Cong Wang and Vlad Buslov. Hmm, could you be more specific on how RCU grace period is respected in your v2 patchset? I looked into patch 1 and one of the rest patches (act_vlan), for me it looks like you only updated free_tcf() since v1, but this change is effectively nothing but removing one wrapper. Let's look at the code in act_vlan: 222 spin_lock_bh(&v->tcf_lock); 223 goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch); 224 rcu_swap_protected(v->vlan_p, p, lockdep_is_held(&v->tcf_lock)); 225 spin_unlock_bh(&v->tcf_lock); 226 227 if (goto_ch) 228 tcf_chain_put_by_act(goto_ch); So, if we are replacing an existing action, goto_ch could be still accessed by RCU readers without taking any refcnt, so why tcf_chain_put_by_act() is safe against RCU readers? As we already discussed, its refcnt is not always non-zero, the target chain could be any chain. What am I missing in your v2? Thanks.
