On Mon 09 Nov 2020 at 16:50, Marcelo Ricardo Leitner
<marcelo.leit...@gmail.com> wrote:
> 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.
Hmmm okay.
>
> If act_frag is confusing, then maybe act_mirred_frag? It is a mirred
> plugin now, after all.
Would be even more confusing to me since the act_frag module code is
only directly accessed from act_ct and not act_mirred :)
Anyway, I don't have a strong opinion regarding this. Just wanted to
understand the motivation.
>
> ...
>> > +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.
Wouldn't it be enough to just call synchronize_rcu() in
tcf_clear_xmit_hook() after setting tcf_xmit_hook to NULL? act_ct module
removal should be very rare, so synchronously waiting for rcu grace
period to complete is probably okay.
>
> 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?
Yes, good idea.
>
>>
>> > + return tcf_xmit_hook(skb, xmit);
>> > + else
>> > + return xmit(skb);
>> > +}
>> > +EXPORT_SYMBOL_GPL(tcf_dev_queue_xmit);
