Re: [PATCH] linux/kernel.h: add container_from()
> > > You really have to pick some pretty excessive type names (or variable > > > names) to get close to 80 characters. Again, to pick an example: > > > > > > struct timer_group_priv *priv = container_of(handle, > > > struct timer_group_priv, timer[handle->num]); > > > > > > ends up being long even if you were to split it, but that funky > > > container_from() wouldn't have helped the real problem - the fact that > > > the above is complex and nasty. > > The point about doing the assignment with the declaration certainly makes > the "ugliness" worse, I agree. I'm still not generally convinced about > the redundancy level pros/cons, but I concede that having a common idiom > (rather than a succinct but subsystem-dependent idiom) is better for > people reading the code for the first time. > > > > And I had to _search_ for that example. All the normal cases of > > > split-line container-of's were due to doing it with the declaration, > > > or beause the first argument ended up being an expression in itself > > > and the nested expressions made it more complex. > > > > Speaking of searching, this kind of typeof use is, IMO, actively > > harmful - it makes finding the places where we might get from > > e.g. linked list to containing objects much harder. container_of > > (unless combined with obfuscating use of typeof()) at least gives > > you a chance to grep - struct foo *not* followed by '*' is a pattern > > that doesn't give too many false positives. This one, OTOH, is > > essentially impossible to grep for. > > And this observation about workflow does strike a chord with me. I do end > up with those kind of searches too. In trying to examine my preferences > here, I think my instincts are to avoid open-coded types (leading me to > want to use typeof()) but I think those instincts were actually developed > from dealing with _sizeof_ and all the way it goes terribly wrong. So, > okay, I'm convinced. container_of() it is. so container_of() it is :) Will start updating the rest of the patches. Thanks, - Allen > Doing these conversions becomes a little less mechanical if assignment > needs to be split from declaration, but hey, we've got a 100 character > line "limit" now, so maybe it'll be less needed. :) > > -- > Kees Cook
Re: [PATCH] linux/kernel.h: add container_from()
> > I don't see that kind of redundancy being a _problem_, though. "So > much redundancy" is just over-stating the issue completely. > > In fact, we often encourage people to split declaration from > initialization exactly because it results in simpler expressions and > more legible code, even if that name is now redundant. So it's a small > extra typing of the type. Big deal. > > The above is also a common pattern that once you know how > container_of() works, it's very legible. > > Sure, if you're new to the kernel, and haven't seen "container_of()" > in other projects, it might initially be a bit of an odd pattern, but > that's the advantage of having one single standardized model: it > becomes a pattern, and you don't have to think about it. > > And particularly with that argument-type pattern, you really have to > work at making over-long lines, since the indentation level will by > definition be just one. > > Looking around, I do see a lot of people doing line-breaks, but that > tends to be when they insist on putting the variable initialization in > the declaration. And even then, it often seems pointless (eg > > struct idp_led *led = container_of(cdev, > struct idp_led, cdev); > > was split for no good reason I can see, but it seems to be a pattern > in that file). > > You really have to pick some pretty excessive type names (or variable > names) to get close to 80 characters. Again, to pick an example: > > struct timer_group_priv *priv = container_of(handle, > struct timer_group_priv, timer[handle->num]); > > ends up being long even if you were to split it, but that funky > container_from() wouldn't have helped the real problem - the fact that > the above is complex and nasty. > An example with a really long member name is +struct nokia_modem_device *modem = from_tasklet(modem, t, +nokia_modem_rst_ind_tasklet); With container_of() one can imagine how long it would end up. And am sure we have many more examples in the kernel. I agree, It would have been simpler to use container_of() as it's been widely used, but as mentioned by Kees, for 1-to-many conversions it does not work well. I guess container_from() is a NAK, if you would want us to just use container_of() to keep the code clean and simple instead of using wrappers, or any other method, we are open to suggestions. Thanks, - Allen
Re: [PATCH] linux/kernel.h: add container_from()
On Thu, Aug 27, 2020 at 10:36:36PM +0100, Al Viro wrote: > On Thu, Aug 27, 2020 at 01:46:33PM -0700, Linus Torvalds wrote: > > You really have to pick some pretty excessive type names (or variable > > names) to get close to 80 characters. Again, to pick an example: > > > > struct timer_group_priv *priv = container_of(handle, > > struct timer_group_priv, timer[handle->num]); > > > > ends up being long even if you were to split it, but that funky > > container_from() wouldn't have helped the real problem - the fact that > > the above is complex and nasty. The point about doing the assignment with the declaration certainly makes the "ugliness" worse, I agree. I'm still not generally convinced about the redundancy level pros/cons, but I concede that having a common idiom (rather than a succinct but subsystem-dependent idiom) is better for people reading the code for the first time. > > And I had to _search_ for that example. All the normal cases of > > split-line container-of's were due to doing it with the declaration, > > or beause the first argument ended up being an expression in itself > > and the nested expressions made it more complex. > > Speaking of searching, this kind of typeof use is, IMO, actively > harmful - it makes finding the places where we might get from > e.g. linked list to containing objects much harder. container_of > (unless combined with obfuscating use of typeof()) at least gives > you a chance to grep - struct foo *not* followed by '*' is a pattern > that doesn't give too many false positives. This one, OTOH, is > essentially impossible to grep for. And this observation about workflow does strike a chord with me. I do end up with those kind of searches too. In trying to examine my preferences here, I think my instincts are to avoid open-coded types (leading me to want to use typeof()) but I think those instincts were actually developed from dealing with _sizeof_ and all the way it goes terribly wrong. So, okay, I'm convinced. container_of() it is. Doing these conversions becomes a little less mechanical if assignment needs to be split from declaration, but hey, we've got a 100 character line "limit" now, so maybe it'll be less needed. :) -- Kees Cook
Re: [PATCH] linux/kernel.h: add container_from()
On Thu, Aug 27, 2020 at 01:46:33PM -0700, Linus Torvalds wrote: > You really have to pick some pretty excessive type names (or variable > names) to get close to 80 characters. Again, to pick an example: > > struct timer_group_priv *priv = container_of(handle, > struct timer_group_priv, timer[handle->num]); > > ends up being long even if you were to split it, but that funky > container_from() wouldn't have helped the real problem - the fact that > the above is complex and nasty. > > And I had to _search_ for that example. All the normal cases of > split-line container-of's were due to doing it with the declaration, > or beause the first argument ended up being an expression in itself > and the nested expressions made it more complex. Speaking of searching, this kind of typeof use is, IMO, actively harmful - it makes finding the places where we might get from e.g. linked list to containing objects much harder. container_of (unless combined with obfuscating use of typeof()) at least gives you a chance to grep - struct foo *not* followed by '*' is a pattern that doesn't give too many false positives. This one, OTOH, is essentially impossible to grep for.
Re: [PATCH] linux/kernel.h: add container_from()
On Thu, Aug 27, 2020 at 12:28 PM Kees Cook wrote:
>
> The common raw pattern for callbacks is:
>
> void callback(struct callback_handle *inner)
> {
> struct outer *instance;
> ...
> instance = container_of(inner, struct outer, member_name_of_inner);
>
> There's so much redundancy here.
What?
It's not all that complicated or even particularly redundant. The main
redundancy comes from you splitting up the declaration from the
initialization - which is certainly fine, and often a good idea, but
it does mean that you mention "struct outer" and "instance" twice.
I don't see that kind of redundancy being a _problem_, though. "So
much redundancy" is just over-stating the issue completely.
In fact, we often encourage people to split declaration from
initialization exactly because it results in simpler expressions and
more legible code, even if that name is now redundant. So it's a small
extra typing of the type. Big deal.
The above is also a common pattern that once you know how
container_of() works, it's very legible.
Sure, if you're new to the kernel, and haven't seen "container_of()"
in other projects, it might initially be a bit of an odd pattern, but
that's the advantage of having one single standardized model: it
becomes a pattern, and you don't have to think about it.
And particularly with that argument-type pattern, you really have to
work at making over-long lines, since the indentation level will by
definition be just one.
Looking around, I do see a lot of people doing line-breaks, but that
tends to be when they insist on putting the variable initialization in
the declaration. And even then, it often seems pointless (eg
struct idp_led *led = container_of(cdev,
struct idp_led, cdev);
was split for no good reason I can see, but it seems to be a pattern
in that file).
You really have to pick some pretty excessive type names (or variable
names) to get close to 80 characters. Again, to pick an example:
struct timer_group_priv *priv = container_of(handle,
struct timer_group_priv, timer[handle->num]);
ends up being long even if you were to split it, but that funky
container_from() wouldn't have helped the real problem - the fact that
the above is complex and nasty.
And I had to _search_ for that example. All the normal cases of
split-line container-of's were due to doing it with the declaration,
or beause the first argument ended up being an expression in itself
and the nested expressions made it more complex.
And in the above example, the real complexity - and the reason the
line ends up long - is because the "member" isn't a member. The above
case works - and it's in fact *intended* to work, I'm not claiming
it's some mis-use of the macro. But it's really a rather complex
case, where it would probably have been a good idea to add a comment
about how this really depends on handle->num being set correctly.
And in fact, it would probably have been a *perfect* example of where
a helper function really would have improved the code, not so much
from a line length perspective, but exactly because the above is a
much more complicated case than most container_of() cases are.
So a helper function like the kvm one I quoted would have been a good
idea. In ways that "container_from()" would not have been, since it
doesn't actually even address the source of complexity.
Linus
Re: [PATCH] linux/kernel.h: add container_from()
On Thu, Aug 27, 2020 at 11:48:19AM -0700, Linus Torvalds wrote:
> On Thu, Aug 27, 2020 at 11:40 AM Linus Torvalds
> wrote:
> >
> > On Thu, Aug 27, 2020 at 11:32 AM James Bottomley
> > wrote:
> > >
> > >
> > > The tasklet rework people don't want to use container_of, which was our
> > > first suggestion, because it produces lines which are "too long".
> >
> > WTF?
>
> Side note: I'm entirely serious. If somebody has problems with "too
> long lines", they are doing things wrong. Just ignore them.
>
> Changing standard kernel interfaces is the wrong solution. What's
> next? Using 2-character indentation like some broken projects do just
> to make lines shorter and encourage people to do deep nesting?
>
> No. The solution is to not write crap code with overly complex expressions.
>
> "container_of()" has been a _very_ successful model, and the only
> reason to ever wrap it is if you already *know* the type, and you wrap
> it with an inline function that actually checks it.
This works for the case where it's a known 1-to-1 conversion, as you
showed. It doesn't work well for the 1-to-many (like timer_struct before,
and tasklet now), where each user of the infrastructure contains the
callback handle struct in their specific containing struct, which is
common for these kinds of callbacks.
> which now results in a type-checked *true* simplification of container-of.
More important than the aesthetics is the type checking, for sure.
The common raw pattern for callbacks is:
void callback(struct callback_handle *inner)
{
struct outer *instance;
...
instance = container_of(inner, struct outer, member_name_of_inner);
There's so much redundancy here. And while mismatches between "instance"
and the "struct outer" type argument will be caught by the compiler,
it's weird to repeat it. Some places will make this less weird by doing:
instance = container_of(inner, typeof(*instance), member_name_of_inner);
and when doing the timer_struct replacement, people didn't like the line
wraps making their drivers "ugly", and the compromise was to implement
from_timer()[1]:
Since the regular pattern of using container_of() during local variable
declaration repeats the need for the variable type declaration
to be included, this adds a helper modeled after other from_*()
helpers that wrap container_of(), named from_timer(). This helper uses
typeof(*variable), removing the type redundancy and minimizing the need
for line wraps in forthcoming conversions from "unsigned data long" to
"struct timer_list *" in the timer callbacks:
-void callback(unsigned long data)
+void callback(struct timer_list *t)
{
- struct some_data_structure *local = (struct some_data_structure *)data;
+ struct some_data_structure *local = from_timer(local, t, timer);
I still didn't like this because "local" got repeated. I still think
some kind of DECLARE*() would be best. Like:
#define DECLARE_CONTAINER(outer_type, outer, member_name_of_inner, inner) \
outer_type outer_name = container_of(inner, typeof(*outer), \
member_name_of_inner)
Then the above old timer_struct example becomes:
-void callback(unsigned long data)
+void callback(struct timer_list *t)
{
- struct some_data_structure *local = (struct some_data_structure *)data;
+ DECLARE_CONTAINER(struct some_data_structure *, local, timer, t);
> Seriously. It sounds to me like the tasklet rework people are people
> we want to avoid. They're doing completely the wrong thing.
So, when it's only directed at me, I just delete the personal attacks
from the quoted sections in my replies and ignore it. When it splashes
on other people, though, I think I have a duty to object to the
behavior:
This is a particularly unfriendly way to mentor new contributors and to
treat existing contributors (and reinforces a false "we"/"them" split,
when everyone just wants to see the kernel be better). And to paint
what is a 300 patch effort that cleans up a poor API that no one else
has been willing to do as "doing completely the wrong thing" when the
complaint is mainly bikeshedding over a mostly mechanical step seems
like quite an overreaction.
But, yes, let's get the right API here. I think container_of() is a mess
for these 1-to-many cases. What do you suggest here? I'll change all of
from_timer() to use it too.
-Kees
[1] https://lore.kernel.org/lkml/20170928133817.GA113410@beast/
--
Kees Cook
Re: [PATCH] linux/kernel.h: add container_from()
On Thu, Aug 27, 2020 at 11:40 AM Linus Torvalds
wrote:
>
> On Thu, Aug 27, 2020 at 11:32 AM James Bottomley
> wrote:
> >
> >
> > The tasklet rework people don't want to use container_of, which was our
> > first suggestion, because it produces lines which are "too long".
>
> WTF?
Side note: I'm entirely serious. If somebody has problems with "too
long lines", they are doing things wrong. Just ignore them.
Changing standard kernel interfaces is the wrong solution. What's
next? Using 2-character indentation like some broken projects do just
to make lines shorter and encourage people to do deep nesting?
No. The solution is to not write crap code with overly complex expressions.
"container_of()" has been a _very_ successful model, and the only
reason to ever wrap it is if you already *know* the type, and you wrap
it with an inline function that actually checks it.
For a lot of (usually) good examples of this, just do a
git grep return.container_of
and find things like
static inline struct kvm_pit *pit_state_to_pit(struct kvm_kpit_state *ps)
{
return container_of(ps, struct kvm_pit, pit_state);
}
which now results in a type-checked *true* simplification of container-of.
It basically creates a specialized version which documents what it
does in the name, and does proper type checking, and doesn't try to be
another name for the existing generic container_of().
Seriously. It sounds to me like the tasklet rework people are people
we want to avoid. They're doing completely the wrong thing.
So just throw that garbage out.
Linus
Re: [PATCH] linux/kernel.h: add container_from()
On Thu, Aug 27, 2020 at 11:32 AM James Bottomley wrote: > > > The tasklet rework people don't want to use container_of, which was our > first suggestion, because it produces lines which are "too long". WTF? Next somebody will decide that our list handling macros don't match their mood, and make up their own. Guys, there's a real advantage to just following convention and not confusing people with new made-up stuff that does the same thing just using slightly different names and slightly different semantics. So let the tasklet rework people work on their own little thing if they can't play with the rest of the kernel. We'll just ignore them. Linus
Re: [PATCH] linux/kernel.h: add container_from()
On Thu, 2020-08-27 at 11:04 -0700, Linus Torvalds wrote: > On Wed, Aug 26, 2020 at 6:36 PM Allen Pais > wrote: > > > > Introduce container_from() as a generic helper instead of > > sub-systems defining a private from_ > > NAK. > > This seems completely broken. > > The documentation comment doesn't even match the macro, and claims > that "container" is a type. > > Which it isn't. That's what container_of()" already takes. > > And if the argument is that a broken commit introduced a broken > macro, > then that's not a great argument. Yes, we have that broken > "from_tasklet()" macro, but it's not even *USED* anywhere. > > So instead of adding a broken new concept that adds absolutely no > value, let's just remove the broken macro that isn't even used. The argument is more over the tasklet rework adding from_tasklet that was effectively completely generic. https://lore.kernel.org/dri-devel/20200817091617.28119-1-allen.cryp...@gmail.com/ The tasklet rework people don't want to use container_of, which was our first suggestion, because it produces lines which are "too long". So container_from is a compromise that takes the actual structure pointer as a second argument instead of the structure type, thus being completely generic and suitable for line length reduction in the tasklet rework code. James
Re: [PATCH] linux/kernel.h: add container_from()
On 27/08/2020 03.36, Allen Pais wrote: > Introduce container_from() as a generic helper instead of > sub-systems defining a private from_* API > (Eg: from_tasklets recently introduced in > 12cc923f1ccc: Tasklet: Introduce new initialization API) > > The helper is similar to container_of() in argument order > with the difference of naming the containing structure instead ^^^ > of having to specify its type. > > +/** > + * container_from - cast a member of a structure out to the containing > structure > + * @ptr: the pointer to the member. > + * @container: the type of the container struct. ^ This seems to have been copy-pasted from container_of? Shouldn't @container be the (local) value we're storing into? As in foo = container_from(..., foo, ...)? Or am I misunderstanding the purpose of this? [And I think it would read nicer if the bikeshed was called to_container(), but don't care deeply.] Rasmus
Re: [PATCH] linux/kernel.h: add container_from()
On Wed, Aug 26, 2020 at 6:36 PM Allen Pais wrote: > > Introduce container_from() as a generic helper instead of > sub-systems defining a private from_ NAK. This seems completely broken. The documentation comment doesn't even match the macro, and claims that "container" is a type. Which it isn't. That's what container_of()" already takes. And if the argument is that a broken commit introduced a broken macro, then that's not a great argument. Yes, we have that broken "from_tasklet()" macro, but it's not even *USED* anywhere. So instead of adding a broken new concept that adds absolutely no value, let's just remove the broken macro that isn't even used. Linus
Re: [PATCH] linux/kernel.h: add container_from()
On Thu, Aug 27, 2020 at 02:19:41PM +0200, Greg Kroah-Hartman wrote: > On Wed, Aug 26, 2020 at 07:31:40PM -0700, Kees Cook wrote: > > On Thu, Aug 27, 2020 at 07:06:36AM +0530, Allen Pais wrote: > > > Introduce container_from() as a generic helper instead of > > > sub-systems defining a private from_* API > > > (Eg: from_tasklets recently introduced in > > > 12cc923f1ccc: Tasklet: Introduce new initialization API) > > > > > > The helper is similar to container_of() in argument order > > > with the difference of naming the containing structure instead > > > of having to specify its type. > > > > > > Suggested-by: James E.J. Bottomley > > > Suggested-by: Greg Kroah-Hartman > > > Suggested-by: Jens Axboe > > > Signed-off-by: Allen Pais > > > > Acked-by: Kees Cook > > > > Who can carry this so it can get used by multiple trees? Should I keep a > > git branch folks should merge when taking Allen's conversion patches? > > I can put it in my driver core tree, and give everyone a stable, signed, > tag to pull it from so that it can get propagated everywhere, if that > makes it easy for others to use it now. Thank you; that would be lovely. :) -- Kees Cook
Re: [PATCH] linux/kernel.h: add container_from()
On Wed, Aug 26, 2020 at 07:31:40PM -0700, Kees Cook wrote: > On Thu, Aug 27, 2020 at 07:06:36AM +0530, Allen Pais wrote: > > Introduce container_from() as a generic helper instead of > > sub-systems defining a private from_* API > > (Eg: from_tasklets recently introduced in > > 12cc923f1ccc: Tasklet: Introduce new initialization API) > > > > The helper is similar to container_of() in argument order > > with the difference of naming the containing structure instead > > of having to specify its type. > > > > Suggested-by: James E.J. Bottomley > > Suggested-by: Greg Kroah-Hartman > > Suggested-by: Jens Axboe > > Signed-off-by: Allen Pais > > Acked-by: Kees Cook > > Who can carry this so it can get used by multiple trees? Should I keep a > git branch folks should merge when taking Allen's conversion patches? I can put it in my driver core tree, and give everyone a stable, signed, tag to pull it from so that it can get propagated everywhere, if that makes it easy for others to use it now. thanks, greg k-h
Re: [PATCH] linux/kernel.h: add container_from()
On Thu, Aug 27, 2020 at 07:06:36AM +0530, Allen Pais wrote: > Introduce container_from() as a generic helper instead of > sub-systems defining a private from_* API > (Eg: from_tasklets recently introduced in > 12cc923f1ccc: Tasklet: Introduce new initialization API) > > The helper is similar to container_of() in argument order > with the difference of naming the containing structure instead > of having to specify its type. > > Suggested-by: James E.J. Bottomley > Suggested-by: Greg Kroah-Hartman > Suggested-by: Jens Axboe > Signed-off-by: Allen Pais Acked-by: Kees Cook Who can carry this so it can get used by multiple trees? Should I keep a git branch folks should merge when taking Allen's conversion patches? (In the future we can do a treewide replacement of the subsystem-specific from*() macros into container_from()) Thanks! -- Kees Cook
