Clarifying confusion of our variable placement rules caused by cleanup.h

Clarifying confusion of our variable placement rules caused by cleanup.h

[James Bottomley]

This patch, introduced by Dan Williams (cc'd):

https://lore.kernel.org/all/171097196970.1011049.9726486429680041876.stgit@dwillia2-xfh.jf.intel.com/

There was some discussion at the time (I missed it because I'm not on
any of the lists it touched) and people expected to be opinionated were
cc'd but none of it really touched on the problem that we now have
inconsistent rules for variable and initializer placement.

Before this change, we always adhered to K&R rules, subsequently
codified in C89, to place variable declarations at the top of the block

The problem specifically is this added comment in cleanup.h:

>  * That bug is fixed by changing init() to call guard() and define +
>  * initialize @obj in this order::
>  *
>  *	guard(mutex)(&lock);
>  *	struct object *obj __free(remove_free) = alloc_add();

Which is recommending mixing declarations and code contrary to our
prior rule.  I note the rule against mixing variables and code was
relaxed in the C99 standard (and in a lot of other languages), but
we've never formally changed our coding rules.

I'm not saying we have to stick with C89, just that if we change
adherence to it, we should do so globally and document it because
having incosistency for __free vs other variables really isn't a good
idea.

So which should we do?  It does seem the dependency ordering problem of
__free annotations means that there are situations where we can't
adhere to the variables at the top of the block rule, but are we going
to relax this globally or simply advise noting the dependency in a
comment but keep the rule (thus discouraging the mixing of code and
declarations except where absolutely necessary)?

The other problem is that we've traditionally not initialized automatic
variables specifically to avoid the initialization taking unnecessary
space in the data/bss of the kernel.  However immediately below the
quote above, there's this

>  *
>  * Given that the "__free(...) = NULL" pattern for variables defined at
>  * the top of the function poses this potential interdependency problem
>  * the recommendation is to always define and assign variables in one
>  * statement and not group variable definitions at the top of the
>  * function when __free() is used.

Which advises always initializing __free variables (in addition to
mixing code and declarations).  My reading of our compilers and
checkers is that actually this is unnecessary: there is a danger that a
return before initialization will result in a free being called on an
uninitialized pointer, but that the checkers and compilers can
correctly detect this (at least as correctly as they usually detect
uninitialized variables) so, again, requiring __free variables always
to be initiallised is an unnecessary deviation from our usual coding
habits.

The danger of the above is that one way of avoiding the data/bss space
problem is to move the declaration into the code directly where the
allocation is (as the rule relaxation above says you could).  Again,
I'm not saying we shouldn't do this, just that we should do it
consistently and mindfully rather than allowing it to leak in like
this.

So, again, I think we should debate whether and how much we're changing
the coding rules and do so globally in the coding-style document rather
than keeping the change in the cleanup.h file.

For myself I do find some value in the C89 declarations at the
beginning of the block for readability, so I'm happy to relax the
mixing rule to cases where it's strictly necessary and require
documenting in the comment what the necessity is.  However, I do think
we should, absent ordering problems, keep __free variables
uninitialised and at the top of the block given we can detect any
problem (and thus keep this rule absolutely for non-__free variables
where there's no ordering issues).  But, again, I'm less attached to
this position than I am to the consistency one: I really think it's a
bad idea to change the rules for one class of variables but not for
another, so whatever we do, we should do it for everything and if that
means relaxing the rule mixing code and declarations for everthing, I
can live with that.

Regards,

James

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Bart Van Assche]

On 11/18/25 8:39 AM, James Bottomley wrote:
> The problem specifically is this added comment in cleanup.h:
> 
>>   * That bug is fixed by changing init() to call guard() and define +
>>   * initialize @obj in this order::
>>   *
>>   *	guard(mutex)(&lock);
>>   *	struct object *obj __free(remove_free) = alloc_add();
> 
> Which is recommending mixing declarations and code contrary to our
> prior rule.  I note the rule against mixing variables and code was
> relaxed in the C99 standard (and in a lot of other languages), but
> we've never formally changed our coding rules.
> 
> I'm not saying we have to stick with C89, just that if we change
> adherence to it, we should do so globally and document it because
> having incosistency for __free vs other variables really isn't a good
> idea.
A related question is whether or not to allow declarations in the
initialization expression of for-statements. Although some maintainers 
reject patches that use this C99 feature, apparently this feature is
already used extensively:

$ git grep -nH 'for (int ' | grep -vE '^Documentation/|^tools/' | wc -l
    1239

Thanks,

Bart.

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Linus Torvalds]

On Tue, 18 Nov 2025 at 09:25, Bart Van Assche <bvanassche@acm.org> wrote:
>
> A related question is whether or not to allow declarations in the
> initialization expression of for-statements.

Absolutely. It's *such* an improvement to the C syntax to be able to do

        for (int i = 0; i < x; i++)

both from a syntax and a variable lifetime rule.

It is also very much a "beginning of scope" syntax, even if the scope
isn't limited by a "{ }" grouping.

We already have that syntax being fairly widespread, doing a quick
grep for it shows over 3k uses of this ("int" being the most common,
but we've got other iterator types being used too).

So this is not even a question of "whether". It is already widely used.

The whole "declare variables in the middle of code" should still be
mostly frowned upon.

But it's practically required for cleanup situations - you really do
want the initialization to pair with the cleanup or you end up with
crazy code that might need dummy initialization that makes no sense.

And I think that is basically the only valid reason for it. The old
"declare at the top of scope" rule still holds true for normal
variable declarations so that you don't have to look for the types.

*MOST* of the cleanup cases are hopefully then abstracted out behind
various guard macros etc, where the declaration not only goes together
with the cleanup information, it's actually also a part of the whole
scoping rules for cleanup.

But the whole "only declare at the top" really doesn't work well for
automatic cleanup. I do see some people still adhering to that rule,
but it really can result in odd looking code. You end up with things
like this:

        struct x509_parse_context *ctx __free(kfree) = NULL;
        ... other code ...
        ctx = kzalloc(sizeof(struct x509_parse_context), GFP_KERNEL);

where you have now split up the whole "this is allocated by kmalloc,
and free'd by kfree" into two different sections that are not next to
each other.

And while I still don't love the "declare variables in the middle of
random code" and I think we're better off with the old rule of
generally declaring things at the top of scope, I really do think that
it's better to keep the freeing-vs-allocation logic together.

Side note: there are other situations where we might just want to
admit that sometimes it's better to declare at the point where the
first variable assignment is done. In particular, when using automatic
types, the type obviously comes from the assignment. So you have a
similar situation wrt the whole declaration location: if you use an
auto type, you can't declare things separately from the assignment -
and the assignment might not work at the top of a scope.

So I do suspect that I'll just have to get used to assignments in the
middle of code in general, but I feel we want to limit it to the cases
where there is a real reason for why the declaration needs to be in a
particular place.

Example automatic type thing: something like this

    #define kmalloc_type(type,gpf) (type *)kmalloc(sizeof(type),gpf)

    __auto_type x = kmalloc_type(struct mystruct);

simply doesn't work if you don't allow the declaration in the middle
of code, because assignments invariably will sometimes be after other
code.

Now, we currently don't use __auto_type very much outside of macros
(and there we often use "typeof(x)" instead for historical compiler
reasons), but I suspect we probably should.  There's a patch floating
around that makes it more convenient with a

   #define auto __auto_type

because the historical C 'auto' keyword has been so completely and
utterly useless.

                  Linus

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Bart Van Assche]

On 11/18/25 10:38 AM, Linus Torvalds wrote:
> Now, we currently don't use __auto_type very much outside of macros
> (and there we often use "typeof(x)" instead for historical compiler
> reasons), but I suspect we probably should.  There's a patch floating
> around that makes it more convenient with a
> 
>     #define auto __auto_type
> 
> because the historical C 'auto' keyword has been so completely and
> utterly useless.

In a C++ style guide I found the following advice for type deduction:

"Use type deduction only if it makes the code clearer to readers who
aren't familiar with the project, or if it makes the code safer. Do not
use it merely to avoid the inconvenience of writing an explicit type."

However, I'm not sure whether this guidance also makes sense for C 
kernel code. See also
https://google.github.io/styleguide/cppguide.html#Type_deduction

Thanks,

Bart.

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[H. Peter Anvin]

On 2025-11-18 10:38, Linus Torvalds wrote:
> 
> Now, we currently don't use __auto_type very much outside of macros
> (and there we often use "typeof(x)" instead for historical compiler
> reasons), but I suspect we probably should.  There's a patch floating
> around that makes it more convenient with a
> 
>    #define auto __auto_type
> 
> because the historical C 'auto' keyword has been so completely and
> utterly useless.
> 

Indeed, and this matches C++ and C23.

The only thing about it was that it got to be fuzzy the best way to upstream it.

Do you want me to send you the patchset during the merge window?

	-hpa

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Linus Torvalds]

On Tue, 18 Nov 2025 at 11:05, Bart Van Assche <bvanassche@acm.org> wrote:
>
> In a C++ style guide I found the following advice for type deduction:
>
> "Use type deduction only if it makes the code clearer to readers who
> aren't familiar with the project, or if it makes the code safer. Do not
> use it merely to avoid the inconvenience of writing an explicit type."

I don't think that's a bad rule, no. I don't think we should encourage
people to switch to automatic types just because they can, but I do
think there are situations where it makes sense and makes the code
simpler.

As mentioned, most of our automatic types currently are hidden in
helper macros. I think it typically works best in those, where you
effectively make them type-agnostic.

But I do think it also makes sense in various allocation scenarios,
where just repeating the same type multiple times adds no real upside.

It's not exactly uncommon to have code like this:

    struct xyz *abc = kzalloc(sizeof(struct xyz), GPF_KERNEL);

and I don't think there's any actual *value* in stating that "struct
xyz" twice (or in stating the sizeof()).

Again: I don't think we should  *push* people to do this, but I think
it's a reasonable thing to allow. And it's a situation where having
the declaration in the middle of the code really does make sense.

                  Linus

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[H. Peter Anvin]

On 2025-11-18 11:04, Bart Van Assche wrote:
> On 11/18/25 10:38 AM, Linus Torvalds wrote:
>> Now, we currently don't use __auto_type very much outside of macros
>> (and there we often use "typeof(x)" instead for historical compiler
>> reasons), but I suspect we probably should.  There's a patch floating
>> around that makes it more convenient with a
>>
>>     #define auto __auto_type
>>
>> because the historical C 'auto' keyword has been so completely and
>> utterly useless.
> 
> In a C++ style guide I found the following advice for type deduction:
> 
> "Use type deduction only if it makes the code clearer to readers who
> aren't familiar with the project, or if it makes the code safer. Do not
> use it merely to avoid the inconvenience of writing an explicit type."
> 
> However, I'm not sure whether this guidance also makes sense for C kernel
> code. See also
> https://google.github.io/styleguide/cppguide.html#Type_deduction
> 

The "makes code clearer or safer" seems like a good idea to me.

Notably the following constructs, mostly used in macros:

	typeof(x) _x = (x);
	typeof(z) _y = (typeof(z)) (y);

... are really quite dangerous because it is very easy to mistakenly put the
wrong variable inside the typeof().

	-hpa

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Linus Torvalds]

On Tue, 18 Nov 2025 at 11:11, H. Peter Anvin <hpa@zytor.com> wrote:
>
> Do you want me to send you the patchset during the merge window?

Sure. I don't think it's a high priority, but I do think it's an
improvement and allows people to write clearer code.

I assume all the conversions got acked?

            Linus

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Steven Rostedt]

On Tue, 18 Nov 2025 10:38:20 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

>         struct x509_parse_context *ctx __free(kfree) = NULL;
>         ... other code ...
>         ctx = kzalloc(sizeof(struct x509_parse_context), GFP_KERNEL);
> 
> where you have now split up the whole "this is allocated by kmalloc,
> and free'd by kfree" into two different sections that are not next to
> each other.

I've been doing the above, and was even going to recommend it to James. But
if it is preferred to declare the __free() variables where they are
allocated, I'd be much happier.

I think the code could also be better optimized? I haven't run an objcopy to
confirm but now early exits do not require calling the __free() function on
NULL pointers.

Most of my code allocates near the top where I don't find this a problem,
but I do have a few places of:

	struct foo *var __free(kfree) = NULL;

	if (ret < 0)
		return -ERROR;

	[ several more error exits ]

	var = kmalloc(..);

-- Steve

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[H. Peter Anvin]

On 2025-11-18 11:16, Linus Torvalds wrote:
> On Tue, 18 Nov 2025 at 11:11, H. Peter Anvin <hpa@zytor.com> wrote:
>>
>> Do you want me to send you the patchset during the merge window?
> 
> Sure. I don't think it's a high priority, but I do think it's an
> improvement and allows people to write clearer code.
> 
> I assume all the conversions got acked?
> 

I think so, and most importantly, the only one which wasn't mechanical (at the
maintainer's request.)

However, as long as the actual definition patch gets in there any subsequent
conversions can also go through maintainer trees if they prefer.

	-hpa

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Linus Torvalds]

On Tue, 18 Nov 2025 at 11:16, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> I've been doing the above, and was even going to recommend it to James. But
> if it is preferred to declare the __free() variables where they are
> allocated, I'd be much happier.

I'm not going to make some hard rule that "it's preferred", but I
think it's simpler and clearer, and I would not want to discourage it.

That said, I *do* want to discourage the rash of mindless conversions.
I do think th is is a situation where people should pick the more
readable version when writing code, *not* a "let's convert existing
code that isn't being otherwise modified" situation.

And your example of

>        struct foo *var __free(kfree) = NULL;
>
>        if (ret < 0)
>                return -ERROR;
>
>        [ several more error exits ]
>
>        var = kmalloc(..);

is exactly where I do think that just moving the declaration down does
actually make code sufficiently better that it then outweighs the "now
you have the variable declarations in random places".

Because I really feel like the whole "__free(kfree) = NULL" thing
doesn't make sense on its own. It really only makes sense when paired
with the "kmalloc()". And _that_ is why they go together.

But again: I don't want to make this some kind of hard rule, and I
think it should be done judiciously and with taste, not some kind of
crazy conversion thing.

              Linus

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[H. Peter Anvin]

On 2025-11-18 08:39, James Bottomley wrote:
> 
> For myself I do find some value in the C89 declarations at the
> beginning of the block for readability, so I'm happy to relax the
> mixing rule to cases where it's strictly necessary and require
> documenting in the comment what the necessity is.  However, I do think
> we should, absent ordering problems, keep __free variables
> uninitialised and at the top of the block given we can detect any
> problem (and thus keep this rule absolutely for non-__free variables
> where there's no ordering issues).  But, again, I'm less attached to
> this position than I am to the consistency one: I really think it's a
> bad idea to change the rules for one class of variables but not for
> another, so whatever we do, we should do it for everything and if that
> means relaxing the rule mixing code and declarations for everthing, I
> can live with that.
> 

To me, a major win with pushing declarations down to first initialization or
thereabouts is that it implicitly reduces the scope of a variable (without
needing to create new blocks.)  This can sometimes catch some pretty serious
errors.

	-hpa

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Steven Rostedt]

On Tue, 18 Nov 2025 11:22:27 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> But again: I don't want to make this some kind of hard rule, and I
> think it should be done judiciously and with taste, not some kind of
> crazy conversion thing.

For the few places that do what that example shows, I may update them, as I
think it does make the code look better.

I do have several places that have something like this:

        struct ring_buffer_per_cpu *cpu_buffer __free(kfree) = NULL;
        struct ring_buffer_cpu_meta *meta;
        struct buffer_page *bpage;
        struct page *page;
        int ret;

        cpu_buffer = kzalloc_node(ALIGN(sizeof(*cpu_buffer), cache_line_size()),
                                  GFP_KERNEL, cpu_to_node(cpu));
        if (!cpu_buffer)
                return NULL;

Where the allocation happens right after the declaration. I think I did it
this way because the full line goes over 80 characters, and breaks up the
declaration.

        struct ring_buffer_per_cpu *cpu_buffer __free(kfree) = 
				kzalloc_node(ALIGN(sizeof(*cpu_buffer), cache_line_size()),
                                		  GFP_KERNEL, cpu_to_node(cpu));
        struct ring_buffer_cpu_meta *meta;
        struct buffer_page *bpage;
        struct page *page;
        int ret;

        if (!cpu_buffer)
                return NULL;

Doesn't look nice. I wonder since its the first allocation, if doing:

        struct ring_buffer_cpu_meta *meta;
        struct buffer_page *bpage;
        struct page *page;
        int ret;

        struct ring_buffer_per_cpu *cpu_buffer __free(kfree) = 
		 kzalloc_node(ALIGN(sizeof(*cpu_buffer), cache_line_size()),
				    GFP_KERNEL, cpu_to_node(cpu));
        if (!cpu_buffer)
                return NULL;

Would be acceptable? The "cpu_buffer" is declared right after the declaration,
but the space after the declaration also makes it easier to read than:

        struct ring_buffer_cpu_meta *meta;
        struct buffer_page *bpage;
        struct page *page;
        int ret;
        struct ring_buffer_per_cpu *cpu_buffer __free(kfree) = 
		 kzalloc_node(ALIGN(sizeof(*cpu_buffer), cache_line_size()),
				    GFP_KERNEL, cpu_to_node(cpu));

        if (!cpu_buffer)
                return NULL;


-- Steve

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[James Bottomley]

On Tue, 2025-11-18 at 14:17 -0500, Steven Rostedt wrote:
> On Tue, 18 Nov 2025 10:38:20 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> >         struct x509_parse_context *ctx __free(kfree) = NULL;
> >         ... other code ...
> >         ctx = kzalloc(sizeof(struct x509_parse_context),
> > GFP_KERNEL);
> > 
> > where you have now split up the whole "this is allocated by
> > kmalloc, and free'd by kfree" into two different sections that are
> > not next to each other.

Yes, I get that; effectively you're declaring an unindented variable
scope.  However, there will be cases where you want an explicit free
(so not a __free variable) and under the rule you propose that should
be allowable as tastefully done.

I've also got to say that the NULL initialization above should be
unnecessary given we can compile time detect if there's potential exit
uninitialized, but I can see that simply declaring where allocated
would avoid having to move the allocation if you did want to introduce
an exit above it in new code.

> I've been doing the above, and was even going to recommend it to
> James. But if it is preferred to declare the __free() variables where
> they are allocated, I'd be much happier.
> 
> I think the code could also be better optimized? I haven't run an
> objcopy to confirm but now early exits do not require calling the
> __free() function on NULL pointers.

Yes, I can confirm that (at least from reading the docs not actually
from disassembling the code).

> 
> Most of my code allocates near the top where I don't find this a
> problem, but I do have a few places of:
> 
> 	struct foo *var __free(kfree) = NULL;
> 
> 	if (ret < 0)
> 		return -ERROR;
> 
> 	[ several more error exits ]
> 
> 	var = kmalloc(..);

Agree: moving the declaration avoids the unnecessary check on exit
(although I'd also presume a good compiler can optimize this away).

There is one last case that hasn't been discussed, which is where you
deliberately introduce an extra scope block for the free and
allocation, so in your example above it would look like

	if (ret < )
		return -ERROR

	[ several more error exits ]

	{
		struct foo *var __free(kfree) = kmalloc(...)

		[...]
	}

I think adding an explicit scope block with no other purpose than to
define the use range of a variable can be beneficial ... with the
obvious caveat that doing it too often causes too much indentation.

It can also help with the entangled lock and free described in the
cleanup.h documentation if the block is paired with a scoped guard.

Regards,

James

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Linus Torvalds]

On Tue, 18 Nov 2025 at 11:55, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Doesn't look nice. I wonder since its the first allocation, if doing:
>
>         struct ring_buffer_cpu_meta *meta;
>         struct buffer_page *bpage;
>         struct page *page;
>         int ret;
>
>         struct ring_buffer_per_cpu *cpu_buffer __free(kfree) =
>                  kzalloc_node(ALIGN(sizeof(*cpu_buffer), cache_line_size()),
>                                     GFP_KERNEL, cpu_to_node(cpu));
>         if (!cpu_buffer)
>                 return NULL;
>
> Would be acceptable?

So no, I do not think this is something we want to do, because that
thing is just stupidly complicated and as such I think it *wants* to
be broken up into multiple pieces.

But this is also literally *why* I was talking up that automatic type
thing, because I think that together with a few helper macros, we
*can* make cases that would otherwise look like the above horror-show
actually work really nicely.

Now, I think that your crazy case that wants to do alignment etc may
never be a good example of this, but for the simpler case of "I just
want a normal allocation for this" a couple of helper macros would
make it quite nice.

Because in the simpler - and I suspect *much* more common - cases, you
could easily end up with something simpler like

       auto cpu_buffer __free(kfree) = kmalloc_type(struct ring_buffer_per_cpu);

and at *that* point I think it's nice. But please - not your horror-show.

Because if you need three lines to make one allocation be legible,
just separate it out.

                 Linus

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[James Bottomley]

On Tue, 2025-11-18 at 11:23 -0800, H. Peter Anvin wrote:
> On 2025-11-18 08:39, James Bottomley wrote:
> > 
> > For myself I do find some value in the C89 declarations at the
> > beginning of the block for readability, so I'm happy to relax the
> > mixing rule to cases where it's strictly necessary and require
> > documenting in the comment what the necessity is.  However, I do
> > think we should, absent ordering problems, keep __free variables
> > uninitialised and at the top of the block given we can detect any
> > problem (and thus keep this rule absolutely for non-__free
> > variables where there's no ordering issues).  But, again, I'm less
> > attached to this position than I am to the consistency one: I
> > really think it's a bad idea to change the rules for one class of
> > variables but not for another, so whatever we do, we should do it
> > for everything and if that means relaxing the rule mixing code and
> > declarations for everthing, 
> > can live with that.
> > 
> 
> To me, a major win with pushing declarations down to first
> initialization or thereabouts is that it implicitly reduces the scope
> of a variable (without needing to create new blocks.)  This can
> sometimes catch some pretty serious errors.

I do somewhat agree with that.  However, I do also think it can be
clearer if you do deliberately create a scope block simply to demarcate
the variable lifetime within the code ... and if you can't do that
because of over indenting then it might be a sign the code needs to be
split up a bit more.

Regards,

James

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Linus Torvalds]

On Tue, 18 Nov 2025 at 12:21, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Tue, 2025-11-18 at 14:17 -0500, Steven Rostedt wrote:
> > I think the code could also be better optimized? I haven't run an
> > objcopy to confirm but now early exits do not require calling the
> > __free() function on NULL pointers.
>
> Yes, I can confirm that (at least from reading the docs not actually
> from disassembling the code).

Actually, I _have_ been disassembling some of that code, and most of
the time the compiler is actually good at eliding those things and not
calling kfree() with a NULL pointer.

Now, the reason for that is actually that we spent some effort on this
in <linux/cleanup.h> (and by "we" I mean mostly PeterZ & co with me
being involved in the discussions).

So you'll see those destructor functions being inline functions with
things like that

   DEFINE_FREE(kfree, void *, if (_T) kfree(_T))

where that "if (_T)" being integral to having the compiler able to see
inline that "oh, it's statically NULL at this stage, I don't need to
call any external function".

But yes, sometimes having the declaration later can simplify this all
for the compiler too. But the *primary* thing should be about making
the code itself legible and maintainable to humans.

          Linus

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Al Viro]

On Tue, Nov 18, 2025 at 11:14:21AM -0800, Linus Torvalds wrote:

> It's not exactly uncommon to have code like this:
> 
>     struct xyz *abc = kzalloc(sizeof(struct xyz), GPF_KERNEL);
> 
> and I don't think there's any actual *value* in stating that "struct
> xyz" twice (or in stating the sizeof()).

Depends...  "Need to find all places where we have struct xyz instances
allocated" does happen on code audit; making it harder to find...

Pet peeve: I really wish we had a decent way to say that pointers to
this particular type are _not_ to be converted without an explicit
(and searchable) cast...   Tricks like wrapping pointer into a struct
do work to an extent, but not when you need to dereference it in a lot
of places ;-/

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Steven Rostedt]

On Tue, 18 Nov 2025 15:21:10 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> There is one last case that hasn't been discussed, which is where you
> deliberately introduce an extra scope block for the free and
> allocation, so in your example above it would look like
> 
> 	if (ret < )
> 		return -ERROR
> 
> 	[ several more error exits ]
> 
> 	{
> 		struct foo *var __free(kfree) = kmalloc(...)
> 
> 		[...]
> 	}

I'm not too big on explicit blocks for just declarations. Especially, in my
case where the var is used until the end:

	{
		struct foo *var __free(kfree) = kmalloc(...)

		[...]

		return func(..., var);
	}

It seems a bit strange to have the final return of a function from within
an explicit scope block. Especially since the beginning is just for error
conditions, and the meat of the function is now in that explicit block.

-- Steve

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Linus Torvalds]

On Tue, 18 Nov 2025 at 12:23, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Now, I think that your crazy case that wants to do alignment etc may
> never be a good example of this, but for the simpler case of "I just
> want a normal allocation for this" a couple of helper macros would
> make it quite nice.

Actually, if we made some "kmalloc_type()" helper macro also do
__alignof__ in addition to the sizeef() , and _if_ the type itself
already has the proper alignment information, it probably would work
fine for most cases.

Your particular example is still too specialized, though, with that
whole "I want a particular node" and a "I want the dynamic cacheline
alignment" rather than "current node" and "static alignment based on
type".

So I think *that* particular case is always specialized enough that
there won't be some simple helper macro to make it more readable, and
as a result you're actually better off just splitting it out, even if
it then results in some duplication, ie just doing

        struct ring_buffer_per_cpu *cpu_buffer __free(kfree);

        cpu_buffer = kzalloc_node(...);

as separate things (but probably next to each other, so that the
"__free(kfree)" part makes sense because the allocation is right
there).

But hey, you could also just make your own alloc/free wrapper
functions, and try to make it more legible that way.

Just a simple

    struct ring_buffer_per_cpu *cpu_buffer_alloc()
    { return kzalloc_node(...); }

would then make that otherwise nasty allocation then become just

    auto cpu_buffer __free(kfree) = cpu_buffer_alloc();

and suddenly it all looks pretty simple. No?

But please do _not_ spread one complex thing over three lines. Split
it up *somehow* to make it easier to read.

Either by just splitting it up into multiple parts, or maybe like the
above with a helper wrapper allocator or whatever.

Helper wrappers that are just used once or twice can still be nice
readability improvements just because they make each part be easier to
read on its own.

              Linus

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[James Bottomley]

On Tue, 2025-11-18 at 15:51 -0500, Steven Rostedt wrote:
> On Tue, 18 Nov 2025 15:21:10 -0500
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > There is one last case that hasn't been discussed, which is where
> > you deliberately introduce an extra scope block for the free and
> > allocation, so in your example above it would look like
> > 
> > 	if (ret < )
> > 		return -ERROR
> > 
> > 	[ several more error exits ]
> > 
> > 	{
> > 		struct foo *var __free(kfree) = kmalloc(...)
> > 
> > 		[...]
> > 	}
> 
> I'm not too big on explicit blocks for just declarations. Especially,
> in my case where the var is used until the end:

Well I don't do it often, but there are times when the scope of a
variable is definitely a lot less than the scope of the code block.

I see this as being analagous to when you should use a guard vs a
scoped guard.

> 
> 	{
> 		struct foo *var __free(kfree) = kmalloc(...)
> 
> 		[...]
> 
> 		return func(..., var);
> 	}
> 
> It seems a bit strange to have the final return of a function from
> within an explicit scope block.

Well, you did that ... the return could equally well have been outside
the block.  However, I do think additional scoped blocks for variables
looks most readable when the scope of the variable is less than the
code on both sides.  If the variable doesn't go out of scope until the
final return, I can see an argument for just doing an interior
declaration.

Regards,

James

>  Especially since the beginning is just for error conditions, and the
> meat of the function is now in that explicit block.
> 
> -- Steve
> 

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Steven Rostedt]

On Tue, 18 Nov 2025 16:10:00 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> > 
> > 	{
> > 		struct foo *var __free(kfree) = kmalloc(...)
> > 
> > 		[...]
> > 
> > 		return func(..., var);
> > 	}
> > 
> > It seems a bit strange to have the final return of a function from
> > within an explicit scope block.  
> 
> Well, you did that ... the return could equally well have been outside
> the block.  However, I do think additional scoped blocks for variables
> looks most readable when the scope of the variable is less than the
> code on both sides.  If the variable doesn't go out of scope until the
> final return, I can see an argument for just doing an interior
> declaration.

I guess you mean by adding a ret value?

	{
		struct foo *var __free(kfree) = kmalloc(...)

		[...]

		ret = func(..., var);
	}

	return ret;

As the var that is passed to the function that this function is retuning
(tail call) is only scoped inside the brackets. But anyway, I don't plan on
changing the code in question here.

I do quite often use the scoped_guard() as that does document exactly what
the guard is protecting.

-- Steve

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[James Bottomley]

On Tue, 2025-11-18 at 17:34 -0500, Steven Rostedt wrote:
> On Tue, 18 Nov 2025 16:10:00 -0500
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > > 
> > > 	{
> > > 		struct foo *var __free(kfree) = kmalloc(...)
> > > 
> > > 		[...]
> > > 
> > > 		return func(..., var);
> > > 	}
> > > 
> > > It seems a bit strange to have the final return of a function
> > > from within an explicit scope block.  
> > 
> > Well, you did that ... the return could equally well have been
> > outside the block.  However, I do think additional scoped blocks
> > for variables looks most readable when the scope of the variable is
> > less than the code on both sides.  If the variable doesn't go out
> > of scope until the final return, I can see an argument for just
> > doing an interior declaration.
> 
> I guess you mean by adding a ret value?

Well yes, that was the difference.

> 	{
> 		struct foo *var __free(kfree) = kmalloc(...)
> 
> 		[...]
> 
> 		ret = func(..., var);
> 	}
> 
> 	return ret;
> 
> As the var that is passed to the function that this function is
> retuning (tail call) is only scoped inside the brackets. But anyway,
> I don't plan on changing the code in question here.
> 
> I do quite often use the scoped_guard() as that does document exactly
> what the guard is protecting.

But how would that be different from a declaration scope with the
declarations at the top?  In many ways that's precisely what for (int
i=0 ...) is except we don't have a generic way of doing it as a block
prefix statement for a bunch of variables.

Regards,

James

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Jani Nikula]

On Tue, 18 Nov 2025, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> For myself I do find some value in the C89 declarations at the
> beginning of the block for readability, so I'm happy to relax the
> mixing rule to cases where it's strictly necessary and require
> documenting in the comment what the necessity is.

I think I've seen an increase in patches using non-pointer const local
variables. No metrics, just a gut feeling.

	const int foo = bar + 5;

I haven't really decided whether I like that or not, and subsequently I
have neither encouraged or discouraged that usage. I don't think we have
any style guidance on that either.

Anyway, more const usage like that would also benefit from declaration
and initialization at a later point when the initializer value is
available, if it's not at the beginning.

BR,
Jani.


-- 
Jani Nikula, Intel

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Alexey Dobriyan]

On Tue, Nov 18, 2025 at 11:39:26AM -0500, James Bottomley wrote:

> So which should we do?

The best way to understand that C89 style of declaring in the beginning
of the function is pointless rule is to write some code in a language
which doesn't enforce it. You should see that nothing bad happens.

It increases bug rate due to increased variable scope allowing typos.

It bloats LOC -- in many cases declaration and initializer can fit
into a single line.

It prevents adding "const" qualifier if necessary.

Pressing PageUp and PageDown when adding new variable is pointless
busywork and distracts, breaks the tempo(flow?) so to speak.

C89 style provokes substyles(!) which makes adding new variables even
more obnoxious: some subsystems have(had?) a rule saying that declarations
(with initializers) must be sorted by length, so not only programmer has
to PageUp to the beginning of the block, but then aim carefully and
insert new declaration.

None of this is necessary (or possible) if the rule says "declare as low
as possible".

There was variation of this type of nonsense with headers (not only it has
to be sorted alphabetically but by length too!)

There is no practical difference between code and declarations:
declarations can have initializers which can be arbitrary complex,
just like "real" code. So the only difference is superficial.


C89 declaration style is pointless and dumb, no wonder other programming
languages dumped it (or never had), it should be simply discarded.

It will also make Linux slightly less white crow to newcomers
(C++ doesn't have this rule after all).

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Stephen Hemminger]

On Tue, 25 Nov 2025 17:25:19 +0300
Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Tue, Nov 18, 2025 at 11:39:26AM -0500, James Bottomley wrote:
> 
> > So which should we do?  
> 
> The best way to understand that C89 style of declaring in the beginning
> of the function is pointless rule is to write some code in a language
> which doesn't enforce it. You should see that nothing bad happens.
> 
> It increases bug rate due to increased variable scope allowing typos.
> 
> It bloats LOC -- in many cases declaration and initializer can fit
> into a single line.
> 
> It prevents adding "const" qualifier if necessary.
> 
> Pressing PageUp and PageDown when adding new variable is pointless
> busywork and distracts, breaks the tempo(flow?) so to speak.
> 
> C89 style provokes substyles(!) which makes adding new variables even
> more obnoxious: some subsystems have(had?) a rule saying that declarations
> (with initializers) must be sorted by length, so not only programmer has
> to PageUp to the beginning of the block, but then aim carefully and
> insert new declaration.
> 
> None of this is necessary (or possible) if the rule says "declare as low
> as possible".
> 
> There was variation of this type of nonsense with headers (not only it has
> to be sorted alphabetically but by length too!)
> 
> There is no practical difference between code and declarations:
> declarations can have initializers which can be arbitrary complex,
> just like "real" code. So the only difference is superficial.
> 
> 
> C89 declaration style is pointless and dumb, no wonder other programming
> languages dumped it (or never had), it should be simply discarded.
> 
> It will also make Linux slightly less white crow to newcomers
> (C++ doesn't have this rule after all).
> 

Agree with everything you said.
But I don't want to see patches that are just to rearrange existing
code to move declarations around. So yes, but no more churn please.

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[Steven Rostedt]

On Tue, 25 Nov 2025 17:25:19 +0300
Alexey Dobriyan <adobriyan@gmail.com> wrote:

> C89 style provokes substyles(!) which makes adding new variables even
> more obnoxious: some subsystems have(had?) a rule saying that declarations
> (with initializers) must be sorted by length, so not only programmer has
> to PageUp to the beginning of the block, but then aim carefully and
> insert new declaration.

As one of the subsystem maintainers that enforce the "order by length"
rule, I'm also for making more exceptions to the c89 method. The reason we
do the "order by length" is for aesthetic reasons, as nicer looking code is
easier to read. If there's a rule to have all declarations at the top, at
least make it pretty!

But yeah, perhaps if we didn't have a strict enforcement of declaring
everything at the top, we wouldn't have bugs like this:

  https://lore.kernel.org/all/20251125032630.8746-3-piliu@redhat.com/

-- Steve

Re: Clarifying confusion of our variable placement rules caused by cleanup.h

[H. Peter Anvin]

On November 25, 2025 6:25:19 AM PST, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>On Tue, Nov 18, 2025 at 11:39:26AM -0500, James Bottomley wrote:
>
>> So which should we do?
>
>The best way to understand that C89 style of declaring in the beginning
>of the function is pointless rule is to write some code in a language
>which doesn't enforce it. You should see that nothing bad happens.
>
>It increases bug rate due to increased variable scope allowing typos.
>
>It bloats LOC -- in many cases declaration and initializer can fit
>into a single line.
>
>It prevents adding "const" qualifier if necessary.
>
>Pressing PageUp and PageDown when adding new variable is pointless
>busywork and distracts, breaks the tempo(flow?) so to speak.
>
>C89 style provokes substyles(!) which makes adding new variables even
>more obnoxious: some subsystems have(had?) a rule saying that declarations
>(with initializers) must be sorted by length, so not only programmer has
>to PageUp to the beginning of the block, but then aim carefully and
>insert new declaration.
>
>None of this is necessary (or possible) if the rule says "declare as low
>as possible".
>
>There was variation of this type of nonsense with headers (not only it has
>to be sorted alphabetically but by length too!)
>
>There is no practical difference between code and declarations:
>declarations can have initializers which can be arbitrary complex,
>just like "real" code. So the only difference is superficial.
>
>
>C89 declaration style is pointless and dumb, no wonder other programming
>languages dumped it (or never had), it should be simply discarded.
>
>It will also make Linux slightly less white crow to newcomers
>(C++ doesn't have this rule after all).
>

Preventing the use of "const" is a big one.