Date:   Thu, 2 Jul 2020 21:47:27 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     wenxu <wenxu@...oud.cn>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net/sched: act_mirred: fix fragment the packet after
 defrag in act_ct

On Thu, Jul 02, 2020 at 02:39:07PM -0700, Cong Wang wrote:
> On Thu, Jul 2, 2020 at 10:32 AM Marcelo Ricardo Leitner
> <marcelo.leitner@...il.com> wrote:
> >
> > On Thu, Jul 02, 2020 at 05:36:38PM +0800, wenxu wrote:
> > >
> > > On 7/2/2020 1:33 AM, Cong Wang wrote:
> > > > On Wed, Jul 1, 2020 at 1:21 AM wenxu <wenxu@...oud.cn> wrote:
> > > >>
> > > >> On 7/1/2020 2:21 PM, wenxu wrote:
> > > >>> On 7/1/2020 2:12 PM, Cong Wang wrote:
> > > >>>> On Tue, Jun 30, 2020 at 11:03 PM wenxu <wenxu@...oud.cn> wrote:
> > > >>>>> Only forward packet case need do fragment again and there is no need do defrag explicit.
> > > >>>> Same question: why act_mirred? You have to explain why act_mirred
> > > >>>> has the responsibility to do this job.
> > > >>> The fragment behavior only depends on the mtu of the device sent in act_mirred. Only in
> > > >>>
> > > >>> the act_mirred can decides whether do the fragment or not.
> > > >> Hi cong,
> > > >>
> > > >>
> > > >> I still think this should be resolved in the act_mirred.  Maybe it is not matter with a "responsibility"
> > > >>
> > > >> Did you have some other suggestion to solve this problem?
> > > > Like I said, why not introduce a new action to handle fragment/defragment?
> > > >
> > > > With that, you can still pipe it to act_ct and act_mirred to achieve
> > > > the same goal.
> > >
> > > Thanks.  Consider about the act_fagment, There are two problem for this.
> > >
> > >
> > > The frag action will put the ressemble skb to more than one packets. How these packets
> > >
> > > go through the following tc filter or chain?
> >
> > One idea is to listificate it, but I don't see how it can work. For
> > example, it can be quite an issue when jumping chains, as the match
> > would have to work on the list as well.
> 
> Why is this an issue? We already use goto action for use cases like
> vlan pop/push. The header can be changed all the time, reclassification
> is necessary.

Hmm I'm not sure you got what I meant. That's operating on the very
same skb... I meant that the pipe would handle a list of skbs (like in
netif_receive_skb_list). So when we have a goto action with such skb,
it would have to either break this list and reclassify each skb
individually, or the classification would have to do it. Either way,
it adds more complexity not just to the code but to the user as well
and ends up doing more processing (in case of fragments or not) than
if it knew how to output such packets properly. Or am I just
over-complicating it?

Or, instead of the explicit "frag" action, make act_ct re-fragment it.
It would need to, somehow, push multiple skbs down the remaining
action pipe. It boils down to the above as well.

> 
> >
> > >
> > >
> > > When should use the act_fragament the action,  always before the act_mirred?
> >
> > Which can be messy if you consider chains like: "mirred, push vlan,
> > mirred" or so. "frag, mirred, defrag, push vlan, frag, mirred".
> 
> So you mean we should have a giant act_do_everything?

Not at all, but

> 
> "Do one thing do it well" is exactly the philosophy of designing tc
> actions, if you are against this, you are too late in the game.

in this case a act_output_it_well could do it. ;-)

Thanks,
  Marcelo

Powered by blists - more mailing lists