Re: [PATCH v5 net-next 3/3] net/sched: act_frag: add implict packet fragment support.

[Vlad Buslov <vladbu@nvidia.com>]

On Mon 09 Nov 2020 at 16:50, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:
> On Mon, Nov 09, 2020 at 03:24:37PM +0200, Vlad Buslov wrote:
>> On Sun 08 Nov 2020 at 01:30, wenxu@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);