On Tue, Aug 25, 2020 at 02:07:43PM +0800, we...@ucloud.cn wrote: ... > +static LIST_HEAD(ct_output_list); > +static DEFINE_SPINLOCK(ct_output_list_lock); > + > +#define CT_OUTPUT_RECURSION_LIMIT 4 > +static DEFINE_PER_CPU(unsigned int, ct_output_rec_level);
Wenxu, first of all, thanks for doing this. Hopefully this helps to show how much duplicated code this means. Later on, any bug that we find on mirrer, we also need to fix in act_ct_output, which is not good. Currently act_ct is the only one doing defrag and leading to this need, but that may change in the future. The action here, AFAICT, has nothing in specific to conntrack. It is "just" re-fragmenting packets. The only specific reference to nf/ct I could notice is for the v6ops, to have access to ip6_fragment(), which can also be done via struct ipv6_stub (once added there). That said, it shouldn't be named after conntrack, to avoid future confusions. I still don't understand Cong's argument for not having this on act_mirred because TC is L2. That's actually not right. TC hooks at L2 but deals with L3 and L4 (after all, it does static NAT, mungles L4 headers and classifies based on virtually anything) since beginning, and this is just another case. What I can understand, is that this feature shouldn't be enabled by default on mirred. So that we are sure that users opting-in know what they are doing. It can have a "l3" flag, to enable L3 semantics, and that's it. Code re-used, no performance drawback for pure L2 users (it can even be protected by a static_key. Once a l3-enabled mirred is loaded, enable it), user knows what to expect and no confusion on which action to use. Marcelo
