On Sat, Nov 07, 2026 at 05:05:07PM +0000, David Laight wrote: > On Sat, 7 Nov 2026 16:27:39 +0100 > Joel Granados wrote: > > > On Mon, Nov 02, 2026 at 11:40:40PM +0800, Wen Yang wrote: > > > > > > > > > On 10/29/26 18:58, David Laight wrote: > > > > On Thu, 29 Oct 2026 00:34:39 +0800 > > > > wen.yang@linux.dev wrote: > > > > > > > > > From: Wen Yang > > > > > > > > > > struct ctl_table describes an entry with data, maxlen and, until this > > > > > series, two untyped misc pointers in extra1 and extra2. Those two pointers > > > > > have accumulated at least seven unrelated jobs: a fixed bound, a bound read > > > > > through a pointer on every call, an object or namespace pointer, a small > > > > > integer cookie encoded into the pointer, a lock signal, NULL standing for > > > > > "no bound", and a value the core itself casts to unsigned int while > > > > > validating a u8 range. Nothing in the entry says which job applies, so > > > > > readers infer it from the handler, and the core gets it wrong where it > > > > > treats extra1 as an unsigned int. > > <--- snip ---> > > > > > (~574 new min/max initializers) instead of pointing at constants. The > > > > > sysctl ABI is unchanged; the parport spintime minimum is corrected as > > > > > described above. > > > > > > > > > > The cost is 8 bytes per ctl_table entry on 64-bit (56 -> 64) and 4 bytes on > > > > > 32-bit, because priv is appended; the range read path gains no indirection > > > > > and the migrated entries no longer dereference a bound pointer. > > > > > > > > Can you remove the little-used 'priv' parameter by making the extra > > > > information just follow the table entry (nested structure) and have > > > > the relevant functions use container_of() to get the outer structure > > > > and extra values? > > > > > > > > > > > > > Hi David, Hi Joel, > > > > > > I reworked the context side of the series so that priv is gone entirely; > > > and the final tree has no per-entry context pointer. > > > A small helper recovers the enclosing object, but it needs the slot > > > index from the caller: > > > > > > #define sysctl_table_container(entry, type, member, index) ({ \ > > > static_assert(__same_type(*(entry), \ > > > typeof_member(type, member[0])), \ > > > "pointer type mismatch"); \ > > > (type *)((void *)(entry) - offsetof(type, member) - \ > > > (size_t)(index) * sizeof(*(entry))); \ > > > }) > > > > > > The reason plain container_of() is not enough is that the core keeps the > > > table as a contiguous array. For > > > > > > struct addrconf_sysctl_table { > > > struct addrconf_sysctl_ctx context; > > > struct ctl_table table[]; > > > }; > > > > > > the handler receives an interior entry, and > > > > > > container_of(ctl, struct addrconf_sysctl_table, table) > > > == (char *)outer + index * sizeof(struct ctl_table) > > > > > > for ctl == &outer->table[index]. It only returns the outer object for > > > index 0. Passing the index explicitly keeps the original registered > > > entry usable: > > > > > > static int addrconf_sysctl_forward(const struct ctl_table *ctl, ...) > > > { > > > struct addrconf_sysctl_table *t = > > > sysctl_table_container(ctl, struct addrconf_sysctl_table, > > > table, ADDRCONF_SYSCTL_FORWARDING); > > > const struct addrconf_sysctl_ctx *ctx = &t->context; > > > ... > > > } > > > > > > Shared handlers get one wrapper per slot; the common body does not need > > > to know its position: > > > > > > #define DEVINET_CONF_WRAPPER(attr) \ > > > static int devinet_conf_##attr(const struct ctl_table *ctl, ...) \ > > > { \ > > > struct devinet_sysctl_table *t = \ > > > sysctl_table_container(ctl, \ > > > struct devinet_sysctl_table, devinet_vars, \ > > > IPV4_DEVCONF_##attr - 1); \ > > > return devinet_conf_proc_common(ctl, &t->context, ...); \ > > > } > > > > Two questions before seeing the actual implementation of this. > > > > 1. This means that we will have to add more macros to the sysctl > > callers? There is a clear signal in [1] against overuse of macros. > > 2. I understand that you are doing this to avoid having priv. Is it to > > save memory? Yes. priv is a void* and we are also trying to get rid > > of these. But this changes removes the use of two void pointers and > > replaces it with just one. > > The idea behind removing the 'void *priv' is mostly to save memory. > There are a lot of systctl and hardly any need it. > > This solution does look nasty. > I suggested another where the size of the array entries was passed in > by the caller. That would mean changing all the proc handler signature. Right? > If that works it would allow the per-value function use container_of() > to get extra fields. Would this mean that the callers would have to have to keep track of the ctl_table offsets? in other words the sysctl indices and the caller's indices would have to be synced. right? This goes a bit against the second point in [1]. Which I'm still willing to live with, but worth avoiding if it can be done. Best [1] https://lore.kernel.org/atoDngZsxy3TWmO_@hobbes > That only really helps if only a few 'private' functions need it. > > David > > > > > Best > > > > [1] https://lore.kernel.org/CAHk-=wjUmLrJd4KM4xvJHUN4P0r8oSu5LyvG4JipnhxizBrE9Q@mail.gmail.com > > > > > > Single-entry tables simply use slot 0: > > > > > > struct devinet_forward_table *t = > > > sysctl_table_container(ctl, struct devinet_forward_table, table, > > > 0); > > > > > > > > > The series is 20 commits over 134 files, with no priv at any point. If > > > you would prefer a different shape for the helper or the wrappers, I am > > > happy to adjust it. > > > > > > > > > Thanks, > > > Wen > > > > > > > David > > > > >