[RFC] Use plan9 C extensions

[RFC] Use plan9 C extensions

[Matthew Wilcox]

While replacing idr_alloc_cyclic(), I've come across a situation in
which being able to depend on -fplan9-extensions is going to lead to
nicer code in the users.

For cyclic allocations, we need to keep a 'next' value somewhere.
The IDR had a moderately large root, so they didn't mind the cost to
all users of embedding the next value in the root.  For the XArray,
I care about increasing it from the current 16 bytes on 64-bit or
12 bytes on 32-bit.  If most users used it, I wouldn't care, but
only about 10% of the IDR users use the cyclic functionality.

What I currently have is this (random example):

-       struct idr      s2s_cp_stateids;
-       spinlock_t      s2s_cp_lock;
+       struct xarray   s2s_cp_stateids;
+       u32             s2s_cp_stateid_next;

...

+       if (xa_alloc_cyclic(&nn->s2s_cp_stateids,
+                               &copy->cp_stateid.si_opaque.so_id, copy,
+                               xa_u32_limit, &nn->s2s_cp_stateid_next,
+                               GFP_KERNEL))
                return 0;

What I'd really like to do is have a special data structure for the
cyclic users that embeds that "next" field, but I can pass to all the
regular xa_foo() functions.  Something like this:

struct cyclic_xarray {
	struct xarray;
	u32 next;
};

Then this user would do:

	struct cyclic_xarray s2s_cp_stateids;

	if (xa_alloc_cyclic(&nn->s2s_cp_stateids,
				&copy->cp_stateid.si_opaque.so_id, copy,
				xa_u32_limit, GFP_KERNEL))

(ie one fewer argument, as well as one fewer thing to track in their struct).

That's what -fplan9-extensions would allow us to do.  Without it,
we'd need to name that struct xarray and need extra syntactic sugar;
eg later when it calls xa_erase(), it'd look like this:

	xa_erase(&nn->s2s_cp_stateids, copy->cp_stateid.si_opaque.so_id);

instead of:

	xa_erase(&nn->s2s_cp_stateids.xa, copy->cp_stateid.si_opaque.so_id);

-fplan9-extensions is supported in gcc since 4.6 which is now our minimum
version.  This might annoy the people who want to get the kernel compiling
with LLVM though (and any other compilers?  is icc still a thing?).  Added
those people to the cc.

Re: [RFC] Use plan9 C extensions

[Nick Desaulniers]

+ Peter, Richard

On Wed, Jan 9, 2019 at 12:31 PM Matthew Wilcox <willy@infradead.org> wrote:
>
>
> While replacing idr_alloc_cyclic(), I've come across a situation in
> which being able to depend on -fplan9-extensions is going to lead to
> nicer code in the users.

Thanks for reaching out and for the heads up!  For more context for
folks following along, here's GCC's documentation on
`-fplan9-extensions`.
https://gcc.gnu.org/onlinedocs/gcc/Unnamed-Fields.html
(See bottom half of the page).

Additionally, here's an SO post where the code owner for Clang
(Richard, separate from LLVM) talks about some of the
`-fplan9-extensions`.
https://stackoverflow.com/questions/7060949/equivalent-to-fplan9-extensions-in-clang

Further, here's a patch that started implementing this in Clang to
support compiling Go with Clang back when Go was not implemented in Go
(self hosting):
https://reviews.llvm.org/D3853

I would say in general, additions of `-f` flags to a codebase should
be carefully reviewed.  The first thing that comes to mind for most
compiler vendors when developers ask about using them is "what are you
doing?"  Some are useful, but there are many that are dangerous and/or
problematic.

>
> For cyclic allocations, we need to keep a 'next' value somewhere.
> The IDR had a moderately large root, so they didn't mind the cost to
> all users of embedding the next value in the root.

This sounds like some kind of "intrusive data structure?"  Doesn't the
kernel have macro's for generic intrusive containers?  Would you be
adding similar macros for xarray (or is that irrelevant to your
question)?

> For the XArray,
> I care about increasing it from the current 16 bytes on 64-bit or
> 12 bytes on 32-bit.  If most users used it, I wouldn't care, but
> only about 10% of the IDR users use the cyclic functionality.
>
> What I currently have is this (random example):
>
> -       struct idr      s2s_cp_stateids;
> -       spinlock_t      s2s_cp_lock;
> +       struct xarray   s2s_cp_stateids;
> +       u32             s2s_cp_stateid_next;
>
> ...
>
> +       if (xa_alloc_cyclic(&nn->s2s_cp_stateids,
> +                               &copy->cp_stateid.si_opaque.so_id, copy,
> +                               xa_u32_limit, &nn->s2s_cp_stateid_next,
> +                               GFP_KERNEL))
>                 return 0;
>
> What I'd really like to do is have a special data structure for the
> cyclic users that embeds that "next" field, but I can pass to all the
> regular xa_foo() functions.  Something like this:
>
> struct cyclic_xarray {
>         struct xarray;
>         u32 next;
> };
>
> Then this user would do:
>
>         struct cyclic_xarray s2s_cp_stateids;
>
>         if (xa_alloc_cyclic(&nn->s2s_cp_stateids,
>                                 &copy->cp_stateid.si_opaque.so_id, copy,
>                                 xa_u32_limit, GFP_KERNEL))

Sorry, this doesn't look quite syntactically valid, so I'm having a
hard time following along.  Would you be able to show me a simple
reproducer in godbolt (https://godbolt.org/) with GCC what you're
trying to do (maybe with comments showing before/after)?  (Godbolt is
a godsend for developers to clearly communicate codegen issues with
compiler vendors, and its short links can easily be embedded in bug
reports and commit messages).

>
> (ie one fewer argument, as well as one fewer thing to track in their struct).

The difference in the second case from the first looks like the "one
fewer argument" is the address of another member of `nn`.  In general
cases of trying to minimize the number of parameters passed, rather
than pass 2 pointers to 2 members, I would curious if you could just
pass 1 pointer to parent struct (ie. `nn`) to `xa_foo()` and friends
and pull out the members there?

>
> That's what -fplan9-extensions would allow us to do.  Without it,
> we'd need to name that struct xarray and need extra syntactic sugar;
> eg later when it calls xa_erase(), it'd look like this:
>
>         xa_erase(&nn->s2s_cp_stateids, copy->cp_stateid.si_opaque.so_id);
>
> instead of:
>
>         xa_erase(&nn->s2s_cp_stateids.xa, copy->cp_stateid.si_opaque.so_id);

Also hard to spot the intended difference as there's no `xa` member in
any of your examples (I assume you meant `xarray`, but a godbolt link
would be appreciated to make sure I'm understanding the problem
correctly).

>
> -fplan9-extensions is supported in gcc since 4.6 which is now our minimum
> version.  This might annoy the people who want to get the kernel compiling
> with LLVM though (and any other compilers?  is icc still a thing?).  Added
> those people to the cc.

Thanks for including us (LLVM folks)!  It would be good that new
additions be made optional if possible if unsupported in Clang; but
that's not doable in this case.  From the documentation, it seems that
`-fplan9-extensions` enables a couple of things, and I think with a
clearer example of what you're trying to do, I would be better
equipped to comment on which part of the compiler flag you'd like to
make use of.

-- 
Thanks,
~Nick Desaulniers

Re: [RFC] Use plan9 C extensions

[Nick Desaulniers]

+ Richard for real this time.

On Wed, Jan 9, 2019 at 2:07 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> + Peter, Richard
>
> On Wed, Jan 9, 2019 at 12:31 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> >
> > While replacing idr_alloc_cyclic(), I've come across a situation in
> > which being able to depend on -fplan9-extensions is going to lead to
> > nicer code in the users.
>
> Thanks for reaching out and for the heads up!  For more context for
> folks following along, here's GCC's documentation on
> `-fplan9-extensions`.
> https://gcc.gnu.org/onlinedocs/gcc/Unnamed-Fields.html
> (See bottom half of the page).
>
> Additionally, here's an SO post where the code owner for Clang
> (Richard, separate from LLVM) talks about some of the
> `-fplan9-extensions`.
> https://stackoverflow.com/questions/7060949/equivalent-to-fplan9-extensions-in-clang
>
> Further, here's a patch that started implementing this in Clang to
> support compiling Go with Clang back when Go was not implemented in Go
> (self hosting):
> https://reviews.llvm.org/D3853
>
> I would say in general, additions of `-f` flags to a codebase should
> be carefully reviewed.  The first thing that comes to mind for most
> compiler vendors when developers ask about using them is "what are you
> doing?"  Some are useful, but there are many that are dangerous and/or
> problematic.
>
> >
> > For cyclic allocations, we need to keep a 'next' value somewhere.
> > The IDR had a moderately large root, so they didn't mind the cost to
> > all users of embedding the next value in the root.
>
> This sounds like some kind of "intrusive data structure?"  Doesn't the
> kernel have macro's for generic intrusive containers?  Would you be
> adding similar macros for xarray (or is that irrelevant to your
> question)?
>
> > For the XArray,
> > I care about increasing it from the current 16 bytes on 64-bit or
> > 12 bytes on 32-bit.  If most users used it, I wouldn't care, but
> > only about 10% of the IDR users use the cyclic functionality.
> >
> > What I currently have is this (random example):
> >
> > -       struct idr      s2s_cp_stateids;
> > -       spinlock_t      s2s_cp_lock;
> > +       struct xarray   s2s_cp_stateids;
> > +       u32             s2s_cp_stateid_next;
> >
> > ...
> >
> > +       if (xa_alloc_cyclic(&nn->s2s_cp_stateids,
> > +                               &copy->cp_stateid.si_opaque.so_id, copy,
> > +                               xa_u32_limit, &nn->s2s_cp_stateid_next,
> > +                               GFP_KERNEL))
> >                 return 0;
> >
> > What I'd really like to do is have a special data structure for the
> > cyclic users that embeds that "next" field, but I can pass to all the
> > regular xa_foo() functions.  Something like this:
> >
> > struct cyclic_xarray {
> >         struct xarray;
> >         u32 next;
> > };
> >
> > Then this user would do:
> >
> >         struct cyclic_xarray s2s_cp_stateids;
> >
> >         if (xa_alloc_cyclic(&nn->s2s_cp_stateids,
> >                                 &copy->cp_stateid.si_opaque.so_id, copy,
> >                                 xa_u32_limit, GFP_KERNEL))
>
> Sorry, this doesn't look quite syntactically valid, so I'm having a
> hard time following along.  Would you be able to show me a simple
> reproducer in godbolt (https://godbolt.org/) with GCC what you're
> trying to do (maybe with comments showing before/after)?  (Godbolt is
> a godsend for developers to clearly communicate codegen issues with
> compiler vendors, and its short links can easily be embedded in bug
> reports and commit messages).
>
> >
> > (ie one fewer argument, as well as one fewer thing to track in their struct).
>
> The difference in the second case from the first looks like the "one
> fewer argument" is the address of another member of `nn`.  In general
> cases of trying to minimize the number of parameters passed, rather
> than pass 2 pointers to 2 members, I would curious if you could just
> pass 1 pointer to parent struct (ie. `nn`) to `xa_foo()` and friends
> and pull out the members there?
>
> >
> > That's what -fplan9-extensions would allow us to do.  Without it,
> > we'd need to name that struct xarray and need extra syntactic sugar;
> > eg later when it calls xa_erase(), it'd look like this:
> >
> >         xa_erase(&nn->s2s_cp_stateids, copy->cp_stateid.si_opaque.so_id);
> >
> > instead of:
> >
> >         xa_erase(&nn->s2s_cp_stateids.xa, copy->cp_stateid.si_opaque.so_id);
>
> Also hard to spot the intended difference as there's no `xa` member in
> any of your examples (I assume you meant `xarray`, but a godbolt link
> would be appreciated to make sure I'm understanding the problem
> correctly).
>
> >
> > -fplan9-extensions is supported in gcc since 4.6 which is now our minimum
> > version.  This might annoy the people who want to get the kernel compiling
> > with LLVM though (and any other compilers?  is icc still a thing?).  Added
> > those people to the cc.
>
> Thanks for including us (LLVM folks)!  It would be good that new
> additions be made optional if possible if unsupported in Clang; but
> that's not doable in this case.  From the documentation, it seems that
> `-fplan9-extensions` enables a couple of things, and I think with a
> clearer example of what you're trying to do, I would be better
> equipped to comment on which part of the compiler flag you'd like to
> make use of.

-- 
Thanks,
~Nick Desaulniers

Re: [RFC] Use plan9 C extensions

[Matthew Wilcox]

On Wed, Jan 09, 2019 at 02:07:11PM -0800, Nick Desaulniers wrote:
> On Wed, Jan 9, 2019 at 12:31 PM Matthew Wilcox <willy@infradead.org> wrote:
> > For cyclic allocations, we need to keep a 'next' value somewhere.
> > The IDR had a moderately large root, so they didn't mind the cost to
> > all users of embedding the next value in the root.
> 
> This sounds like some kind of "intrusive data structure?"  Doesn't the
> kernel have macro's for generic intrusive containers?  Would you be
> adding similar macros for xarray (or is that irrelevant to your
> question)?

I think that's irrelevant ...

> Sorry, this doesn't look quite syntactically valid, so I'm having a
> hard time following along.  Would you be able to show me a simple
> reproducer in godbolt (https://godbolt.org/) with GCC what you're
> trying to do (maybe with comments showing before/after)?  (Godbolt is
> a godsend for developers to clearly communicate codegen issues with
> compiler vendors, and its short links can easily be embedded in bug
> reports and commit messages).

Sure!  https://godbolt.org/z/OfHOLI is an example of a client of the API
I want to create.

> > (ie one fewer argument, as well as one fewer thing to track in their struct).
> 
> The difference in the second case from the first looks like the "one
> fewer argument" is the address of another member of `nn`.  In general
> cases of trying to minimize the number of parameters passed, rather
> than pass 2 pointers to 2 members, I would curious if you could just
> pass 1 pointer to parent struct (ie. `nn`) to `xa_foo()` and friends
> and pull out the members there?

Not really; nn is going to be a per-client type, ie hundreds of different
structs.  The struct xarray gets embedded in it.

> > That's what -fplan9-extensions would allow us to do.  Without it,
> > we'd need to name that struct xarray and need extra syntactic sugar;
> > eg later when it calls xa_erase(), it'd look like this:
> >
> >         xa_erase(&nn->s2s_cp_stateids, copy->cp_stateid.si_opaque.so_id);
> >
> > instead of:
> >
> >         xa_erase(&nn->s2s_cp_stateids.xa, copy->cp_stateid.si_opaque.so_id);
> 
> Also hard to spot the intended difference as there's no `xa` member in
> any of your examples (I assume you meant `xarray`, but a godbolt link
> would be appreciated to make sure I'm understanding the problem
> correctly).

Right, what I'm trying to communicate there is that if we need to name
the xarray, we then have to name it everywhere that we use it.

> Thanks for including us (LLVM folks)!  It would be good that new
> additions be made optional if possible if unsupported in Clang; but
> that's not doable in this case.  From the documentation, it seems that
> `-fplan9-extensions` enables a couple of things, and I think with a
> clearer example of what you're trying to do, I would be better
> equipped to comment on which part of the compiler flag you'd like to
> make use of.

I hope the example makes things clearer!

Re: [RFC] Use plan9 C extensions

[Linus Torvalds]

On Wed, Jan 9, 2019 at 12:31 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> While replacing idr_alloc_cyclic(), I've come across a situation in
> which being able to depend on -fplan9-extensions is going to lead to
> nicer code in the users.

Oh, no. Please don't.

The subset of the pla9 extensions that are called just "ms" extenions
is fine. That's a reasonable thing, and is a very natural expansion of
the unnamed structures we already have - namely being able to
pre-declare that unnamed structure/union.

But the full plan9 extensions are nasty, and makes it much too easy to
write "convenient" code that is really hard to read as an outsider
because of how the types are silently converted.

And I think what you want is explicitly that silent conversion.

So no. Don't do it. Use a macro or a inline function that makes the
conversion explicit so that it's shown when grepping.

The "one extra argument" is not a strong argument for something that
simply isn't that common. The upsides of a nonstandard feature like
that needs to be pretty compelling.

We've used various gcc extensions since day #1 ("inline" being perhaps
the biggest one that took _forever_ to become standard C), but these
things need to have very strong arguments.

"One extra argument" that could be hidden by a macro or a helper
inline is simply not a strong argument.

                      Linus

Re: [RFC] Use plan9 C extensions

[Nick Desaulniers]

This is Matthew's example tweaked a little bit for my understanding, FWIW:
https://godbolt.org/z/mD7HdX

On Wed, Jan 9, 2019 at 3:34 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, Jan 9, 2019 at 12:31 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > While replacing idr_alloc_cyclic(), I've come across a situation in
> > which being able to depend on -fplan9-extensions is going to lead to
> > nicer code in the users.
>
> Oh, no. Please don't.
>
> The subset of the pla9 extensions that are called just "ms" extenions
> is fine. That's a reasonable thing, and is a very natural expansion of
> the unnamed structures we already have - namely being able to
> pre-declare that unnamed structure/union.
>
> But the full plan9 extensions are nasty, and makes it much too easy to
> write "convenient" code that is really hard to read as an outsider
> because of how the types are silently converted.
>
> And I think what you want is explicitly that silent conversion.

I agree that this reminds me of an implicit conversion, which is one
of the *worst* parts of C IMO.  Implicit conversions violate the
principle of least surprise
(https://en.wikipedia.org/wiki/Principle_of_least_astonishment), and
implicit conversions are the most pointed to "defect" in languages
like JavaScript.  I understand why they're convenient, but the
resulting surprising bugs don't outweigh their cost (again, my
opinion), and developers frequently can't keep these implicit
conversion rules straight (whether we're talking about JavaScript or
C).  When it comes to type conversions, my personal thoughts align
with the Zen of Python ("Explicit is better than implicit.").

Of course, each code base can weigh the benefits with the risks and
decide what works best for them.

>
> So no. Don't do it. Use a macro or a inline function that makes the
> conversion explicit so that it's shown when grepping.
>
> The "one extra argument" is not a strong argument for something that
> simply isn't that common. The upsides of a nonstandard feature like
> that needs to be pretty compelling.
>
> We've used various gcc extensions since day #1 ("inline" being perhaps
> the biggest one that took _forever_ to become standard C), but these
> things need to have very strong arguments.
>
> "One extra argument" that could be hidden by a macro or a helper
> inline is simply not a strong argument.

I agree that something like builtin_types_compatible_p() in a macro
could help make these functions more "generic" in the sense of being
able to accept either a `struct xarray*` or `struct xarray_cyclic*`.

We're always happy to see where Clang is in terms of GCC
compatibility; I'm always happy to chat about this subject more in the
future. (and TIL about -fplan9-extensions) :D
-- 
Thanks,
~Nick Desaulniers

Re: [RFC] Use plan9 C extensions

[Rasmus Villemoes]

On 10/01/2019 00.55, Nick Desaulniers wrote:

> I agree that something like builtin_types_compatible_p() in a macro
> could help make these functions more "generic" in the sense of being
> able to accept either a `struct xarray*` or `struct xarray_cyclic*`.

An alternative to implementing all the generic functions as macros is
transparent_union:

https://godbolt.org/z/TK_SJD

If there is an interface that takes a "const xarray *" one would define
another union type

union __transparent_union const_xarray_any {
  const struct xarray *xa;
  const struct xarray_cyclic *xac;
};

The obvious implementation using _Generic fails since all expressions
must be valid:

https://godbolt.org/z/X0bvwO

and _Generic is gcc >= 4.9 anyway. I think an implementation based on
choose_expr/types_compatible might have the same problem, but maybe
there's some clever way around that.

Rasmus