Message-ID: <CAKwvOdns4358_UgKgBPGzMfXoFLDijv13wFnv5UP=G_zjZbcwA@mail.gmail.com>
Date:   Wed, 9 Jan 2019 14:07:11 -0800
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Nathan Chancellor <natechancellor@...il.com>,
        Joel Stanley <joel@....id.au>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Peter Collingbourne <pcc@...gle.com>, rsmith@...gle.com
Subject: Re: [RFC] Use plan9 C extensions

+ Peter, Richard

On Wed, Jan 9, 2019 at 12:31 PM Matthew Wilcox <willy@...radead.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

Powered by blists - more mailing lists