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