On Mon, Nov 09, 2020 at 03:24:37PM +0200, Vlad Buslov wrote:
> On Sun 08 Nov 2020 at 01:30, we...@ucloud.cn wrote:
...
> > @@ -974,9 +974,22 @@ config NET_ACT_TUNNEL_KEY
> > To compile this code as a module, choose M here: the
> > module will be called act_tunnel_key.
> >
> > +config NET_ACT_FRAG
> > + tristate "Packet fragmentation"
> > + depends on NET_CLS_ACT
> > + help
> > + Say Y here to allow fragmenting big packets when outputting
> > + with the mirred action.
> > +
> > + If unsure, say N.
> > +
> > + To compile this code as a module, choose M here: the
> > + module will be called act_frag.
> > +
>
> Just wondering, what is the motivation for putting the frag code into
> standalone module? It doesn't implement usual act_* interface and is not
> user-configurable. To me it looks like functionality that belongs to
> act_api. Am I missing something?
It's the way we found so far for not "polluting" mirred/tc with L3
functionality, per Cong's feedbacks on previous attempts. As for why
not act_api, this is not some code that other actions can just re-use
and that file is already quite big, so I thought act_frag would be
better to keep it isolated/contained.
If act_frag is confusing, then maybe act_mirred_frag? It is a mirred
plugin now, after all.
...
> > +int tcf_set_xmit_hook(int (*xmit_hook)(struct sk_buff *skb,
> > + int (*xmit)(struct sk_buff *skb)))
> > +{
> > + if (!tcf_xmit_hook_enabled())
> > + xchg(&tcf_xmit_hook, xmit_hook);
>
> Marcelo, why did you suggest to use atomic operations to change
> tcf_xmit_hook variable? It is not obvious to me after reading the code.
I thought as a minimal way to not have problems on module removal, but
your comment below proves it is not right/enough. :-)
>
> > + else if (xmit_hook != tcf_xmit_hook)
> > + return -EBUSY;
> > +
> > + tcf_inc_xmit_hook();
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(tcf_set_xmit_hook);
> > +
> > +void tcf_clear_xmit_hook(void)
> > +{
> > + tcf_dec_xmit_hook();
> > +
> > + if (!tcf_xmit_hook_enabled())
> > + xchg(&tcf_xmit_hook, NULL);
> > +}
> > +EXPORT_SYMBOL_GPL(tcf_clear_xmit_hook);
> > +
> > +int tcf_dev_queue_xmit(struct sk_buff *skb, int (*xmit)(struct sk_buff
> > *skb))
> > +{
> > + if (tcf_xmit_hook_enabled())
>
> Okay, so what happens here if tcf_xmit_hook is disabled concurrently? If
> we get here from some rule that doesn't involve act_ct but uses
> act_mirred and act_ct is concurrently removed decrementing last
> reference to static branch and setting tcf_xmit_hook to NULL?
Yeah.. good point. Thinking further now, what about using RCU for the
hook? AFAICT it can cover the synchronization needed when clearing the
pointer, tcf_set_xmit_hook() should do a module_get() and
tcf_clear_xmit_hook() can delay a module_put(act_frag) as needed with
call_rcu.
I see tcf_mirred_act is already calling rcu_dereference_bh(), so
it's already protected by rcu read here and calling tcf_xmit_hook()
with xmit pointer should be fine. WDYT?
>
> > + return tcf_xmit_hook(skb, xmit);
> > + else
> > + return xmit(skb);
> > +}
> > +EXPORT_SYMBOL_GPL(tcf_dev_queue_xmit);
