Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n

Hi Jamie,

On Fri, Nov 18, 2022 at 2:50 AM Jamie Bainbridge
[off-list ref] wrote:

On Thu, 17 Nov 2022 at 08:15, Jakub Kicinski [off-list ref] wrote:

quoted

On Thu, 17 Nov 2022 08:39:43 +1100 Jamie Bainbridge wrote:

quoted

quoted

        if (v6) {
#ifdef v6
                expensive_call6();
#endif
        } else {
                expensive_call6();
        }

These should work, but I expect they cause a comparison which can't be
optimised out at compile time. This is probably why the first style
exists.

In this SYN flood codepath optimisation doesn't matter because we're
doing ratelimited logging anyway. But if we're breaking with existing
style, then wouldn't the others also have to change to this style? I
haven't reviewed all the other usage to tell if they're in an oft-used
fastpath where such a thing might matter.

I think the word style already implies subjectivity.

You are right. Looking further, there are many other ways
IF_ENABLED(CONFIG_IPV6) is used, including similar to the ways you
have suggested.

I don't mind Geert's original patch, but if you want a different
style, I like your suggestion with v4 first:

        if (v4) {
                expensive_call4();
#ifdef v6
        } else {
                expensive_call6();
#endif
        }

IMHO this is worse, as the #ifdef/#endif is spread across the two branches
of an if-conditional.

Hence this is usually written as:

            if (cond1) {
                    expensive_call1();
            }
    #ifdef cond2_enabled
           else {
                    expensive_call1();
            }
    #endif

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds