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@xxxxxxxxxx> wrote:
>
> + Peter, Richard
>
> On Wed, Jan 9, 2019 at 12:31 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> 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