Re: [RFC PATCH v1 17/30] sysctl: net: use sysctl_field in SCTP sysctls

[Alexey Gladkov <legion@kernel.org>]

On Wed, Aug 26, 2026 at 01:29:00PM -0700, Linus Torvalds wrote:
> I detest this series.
> 
> Maybe it makes technical sense, but as long as it makes no human
> visual sense, I'm NAK'ing it as being garbage.

You are right that my approach added too many macros and made the tables
harder to read. Sorry.

> On Wed, 26 Aug 2026 at 12:43, Alexey Gladkov <legion@kernel.org> wrote:
> >
>  [..]
> 
> The whole series is full of complete illegible noise like this:
> 
> > +static const struct sysctl_field sctp_net_table[] = {
> > +       SYSCTL_FIELD_CUSTOM("rto_min", 0644, sizeof(unsigned int),
> > +                        sctp_rto_min_data, proc_sctp_do_rto_min),
> > +       SYSCTL_FIELD_CUSTOM("rto_max", 0644, sizeof(unsigned int),
> > +                        sctp_rto_max_data, proc_sctp_do_rto_max),
> > +       SYSCTL_FIELD_INT_MINMAX("pf_retrans", 0644, sctp_pf_retrans_data,
> > +                            SYSCTL_ZERO, sctp_ps_retrans_data),
> > +       SYSCTL_FIELD_INT_MINMAX("ps_retrans", 0644, sctp_ps_retrans_data,
> > +                            sctp_pf_retrans_data, sctp_ps_retrans_max_data),
> > +       SYSCTL_FIELD_STATIC_UINT_MINMAX("rto_initial", 0644,
> > +                                    sctp_rto_initial_data,
> > +                                    SYSCTL_UINT_ONE, &timer_max),
> [...]
> 
> where apparently the indentation has been decided by a rodent on crack
> who was given an Ouija board and instructed to ask his dead ancestors
> what indentation to use.
> 
> So no.
> 
> That kind of complete random code is simply not acceptable.
> 
> I don't know what the correct answer is, but it is *not* this series.
> It needs to be consistent and visually parseable by humans *without*
> asking your dead ancestors for help.
> 
> And it's not just the indentation. That SYSCTL_FIELD_CUSTOM() thing
> needs to be usable and able to do some minimal type checking - not
> just passed a random sizeof() in a random argumentt. I'd suggest
> passing the actual type, and then checking that the type *matches* the
> data pointer it is passed too.
> 
> The old code may be ugly too, and have various other warts, but at
> least it had somewhat legible and understandable initializers:
> 
>         {
>                 .procname       = "prsctp_enable",
>                 .data           = &init_net.sctp.prsctp_enable,
>                 .maxlen         = sizeof(int),
>                 .mode           = 0644,
>                 .proc_handler   = proc_dointvec,
>         },
> 
> is at least something that can be read by a human and those things had
> consistent whitespace rather than some quantum randomness.
> 
> And this is also just complete line noise that only makes the code worse:
> 
>     +#define SCTP_DATA(type, field)                                         \
>     +static type *sctp_ ## field ## _data(const struct sysctl_context *ctx) \
>     +{                                                                      \
>     +       return &ctx->ns.net_ns->sctp.field;                             \
>     +}
>     +
>     +#define SCTP_CUSTOM_DATA(field)
>              \
>     +static void *sctp_ ## field ## _data(const struct sysctl_context *ctx) \
>     +{                                                                      \
>     +       return &ctx->ns.net_ns->sctp.field;                             \
>     +}
>     +
>     +SCTP_CUSTOM_DATA(rto_min)
>     +SCTP_CUSTOM_DATA(rto_max)
>     +SCTP_DATA(int, pf_retrans)
>     +SCTP_DATA(int, ps_retrans)
>     +SCTP_DATA(unsigned int, rto_initial)
>     [...]
> 
> If we're doing these kinds of changes, the end result has to look
> *BETTER* than the thing it replaces, not worse.
> 
> Yes, a few of the patches did look better. But the majority of them
> only looked worse. Some of it should be easy to fix: use consistent
> whitespace, and use sane argument ordering.
> 
> But honestly, the old setup didn't *rely* on argument ordering, and
> used named initializers to make things more legible and robust.
> 
> So I suspect that should be what you should aim for in the new setup
> too, and that probably means "completely different approach".
> 
>                Linus
> 

I tried an alternative based on offsets. The following four patches add the
infrastructure and convert IPC, SCTP, and MPLS as examples.

The remaining macros are short wrappers around offsetof() and are only used for
compile-time type checking. If they still hurt readability, I can drop that
checking and use plain offsetof() instead.

Does this look better for you, or should I just stop?


Alexey Gladkov (4):
  sysctl: add typed field descriptors
  sysctl: ipc: use typed fields for IPC namespace sysctls
  sctp: use typed fields for per-net sysctls
  mpls: use typed fields for per-device sysctls

 fs/proc/proc_sysctl.c  | 282 ++++++++++++++++++++++++++---
 include/linux/sysctl.h | 134 +++++++++++++-
 ipc/ipc_sysctl.c       | 188 +++++++++----------
 net/mpls/af_mpls.c     |  66 ++++---
 net/sctp/sysctl.c      | 398 +++++++++++++++++++++--------------------
 5 files changed, 703 insertions(+), 365 deletions(-)

-- 
Rgrds, legion