hi Vlad,
On Mon, 2019-02-18 at 16:52 +0000, Vlad Buslov wrote:
> Hi Davide,
>
> I really like the idea of putting all action validation code in single
> place, instead of spreading it between act API and specific actions init
> code.
sure, the previous validation code was in tcf_action_add_1(), and I
attempted to fix the problems (well, it's the same problem showing up in
3 different ways) without adding the same code everywhere.
Sadly, we need to handle correctly the situations where
tcf_action_check_ctrlact() returns an error, and I'm seeing that we
can't fix every action with the same exact code everywhere (to avoid
leaking IDRs, or memory, or both).
> See my comment regarding minor problem with using
> tcf_action_set_ctrlact() on current net-next below.
> > +void tcf_action_set_ctrlact(struct tc_action *p, int action,
> > + struct tcf_chain *goto_chain)
> > +{
> > + struct tcf_chain *old;
> > +
> > + old = xchg(&p->goto_chain, goto_chain);
> > + if (old)
> > + tcf_chain_put_by_act(old);
>
> This is blocking in current net-next because tcf_chain_put_by_act()
> obtains block->lock mutex, so you can't call it while holding tcf_lock
> spinlock like you do in following patches. Its not a big problem but
> I guess you will have to just inline this code into all init handlers
> instead of tcf_action_set_ctrlact() function.
Argh! you are right. Thanks a lot for spotting this.
Ok, let's kill tcf_action_set_ctrlact() and swap 'newchain' with
a->goto_chain in each action init(), with a->tcfa_lock taken. I will
then call tcf_chain_put_by_act() in the init() handler, after the
spinlock is released.
> BTW is atomic xchg strictly necessary if you hold tcf_lock while
> re-assigning goto_chain pointer?
No, it's not necessary. The new value of goto_chain is used only when the
new value of tcfa_action is updated: so, if we just do the assignment of
goto_chain and tcfa_action together with the spinlock taken, at least we
are improving the
current code.
Thanks!
--
davide
