Re: [PATCH net] net/sched: act_mirred: fix fragment the packet after defrag in act_ct

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

On Fri, Jul 03, 2020 at 06:19:51PM +0800, wenxu wrote:
> 
> On 7/3/2020 8:47 AM, Marcelo Ricardo Leitner wrote:
> > 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@gmail.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@ucloud.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@ucloud.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. ;-)
> agree, Maybe a act_output_ct action is better?

Ahm, sorry, that wasn't really my intention here. I meant that I don't
see an issue with mirred learning how to fragment it and, with that,
do its action "well" (as subjective as the term can be).

> >
> > Thanks,
> >   Marcelo
> >