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

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

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.

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);