Messages in this thread | | | From | Nick Desaulniers <> | Date | Wed, 9 Jan 2019 14:08:16 -0800 | Subject | Re: [RFC] Use plan9 C extensions |
| |
+ 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, > > + ©->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, > > ©->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
| |