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

[wenxu <wenxu@ucloud.cn>]

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?
>
> Thanks,
>   Marcelo
>