| Message ID | 20200817091617.28119-2-allen.cryptic@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | block: convert tasklets to use new tasklet_setup() API | expand |
On 8/17/20 2:15 AM, Allen Pais wrote: > From: Allen Pais <allen.lkml@gmail.com> > > In preparation for unconditionally passing the > struct tasklet_struct pointer to all tasklet > callbacks, switch to using the new tasklet_setup() > and from_tasklet() to pass the tasklet pointer explicitly. Who came up with the idea to add a macro 'from_tasklet' that is just container_of? container_of in the code would be _much_ more readable, and not leave anyone guessing wtf from_tasklet is doing. I'd fix that up now before everything else goes in...
On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: > On 8/17/20 2:15 AM, Allen Pais wrote: > > From: Allen Pais <allen.lkml@gmail.com> > > > > In preparation for unconditionally passing the > > struct tasklet_struct pointer to all tasklet > > callbacks, switch to using the new tasklet_setup() > > and from_tasklet() to pass the tasklet pointer explicitly. > > Who came up with the idea to add a macro 'from_tasklet' that is just > container_of? container_of in the code would be _much_ more readable, > and not leave anyone guessing wtf from_tasklet is doing. > > I'd fix that up now before everything else goes in... As I mentioned in the other thread, I think this makes things much more readable. It's the same thing that the timer_struct conversion did (added a container_of wrapper) to avoid the ever-repeating use of typeof(), long lines, etc.
On 8/17/20 12:29 PM, Kees Cook wrote: > On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: >> On 8/17/20 2:15 AM, Allen Pais wrote: >>> From: Allen Pais <allen.lkml@gmail.com> >>> >>> In preparation for unconditionally passing the >>> struct tasklet_struct pointer to all tasklet >>> callbacks, switch to using the new tasklet_setup() >>> and from_tasklet() to pass the tasklet pointer explicitly. >> >> Who came up with the idea to add a macro 'from_tasklet' that is just >> container_of? container_of in the code would be _much_ more readable, >> and not leave anyone guessing wtf from_tasklet is doing. >> >> I'd fix that up now before everything else goes in... > > As I mentioned in the other thread, I think this makes things much more > readable. It's the same thing that the timer_struct conversion did > (added a container_of wrapper) to avoid the ever-repeating use of > typeof(), long lines, etc. But then it should use a generic name, instead of each sub-system using some random name that makes people look up exactly what it does. I'm not huge fan of the container_of() redundancy, but adding private variants of this doesn't seem like the best way forward. Let's have a generic helper that does this, and use it everywhere.
On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: > On 8/17/20 12:29 PM, Kees Cook wrote: > > On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: > >> On 8/17/20 2:15 AM, Allen Pais wrote: > >>> From: Allen Pais <allen.lkml@gmail.com> > >>> > >>> In preparation for unconditionally passing the > >>> struct tasklet_struct pointer to all tasklet > >>> callbacks, switch to using the new tasklet_setup() > >>> and from_tasklet() to pass the tasklet pointer explicitly. > >> > >> Who came up with the idea to add a macro 'from_tasklet' that is just > >> container_of? container_of in the code would be _much_ more readable, > >> and not leave anyone guessing wtf from_tasklet is doing. > >> > >> I'd fix that up now before everything else goes in... > > > > As I mentioned in the other thread, I think this makes things much more > > readable. It's the same thing that the timer_struct conversion did > > (added a container_of wrapper) to avoid the ever-repeating use of > > typeof(), long lines, etc. > > But then it should use a generic name, instead of each sub-system using > some random name that makes people look up exactly what it does. I'm not > huge fan of the container_of() redundancy, but adding private variants > of this doesn't seem like the best way forward. Let's have a generic > helper that does this, and use it everywhere. I'm open to suggestions, but as things stand, these kinds of treewide changes end up getting whole-release delays because of the need to have the API in place for everyone before patches to do the changes can be sent to multiple maintainers, etc.
On 8/17/20 12:48 PM, Kees Cook wrote: > On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: >> On 8/17/20 12:29 PM, Kees Cook wrote: >>> On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: >>>> On 8/17/20 2:15 AM, Allen Pais wrote: >>>>> From: Allen Pais <allen.lkml@gmail.com> >>>>> >>>>> In preparation for unconditionally passing the >>>>> struct tasklet_struct pointer to all tasklet >>>>> callbacks, switch to using the new tasklet_setup() >>>>> and from_tasklet() to pass the tasklet pointer explicitly. >>>> >>>> Who came up with the idea to add a macro 'from_tasklet' that is just >>>> container_of? container_of in the code would be _much_ more readable, >>>> and not leave anyone guessing wtf from_tasklet is doing. >>>> >>>> I'd fix that up now before everything else goes in... >>> >>> As I mentioned in the other thread, I think this makes things much more >>> readable. It's the same thing that the timer_struct conversion did >>> (added a container_of wrapper) to avoid the ever-repeating use of >>> typeof(), long lines, etc. >> >> But then it should use a generic name, instead of each sub-system using >> some random name that makes people look up exactly what it does. I'm not >> huge fan of the container_of() redundancy, but adding private variants >> of this doesn't seem like the best way forward. Let's have a generic >> helper that does this, and use it everywhere. > > I'm open to suggestions, but as things stand, these kinds of treewide On naming? Implementation is just as it stands, from_tasklet() is totally generic which is why I objected to it. from_member()? Not great with naming... But I can see this going further and then we'll suddenly have tons of these. It's not good for readability. > changes end up getting whole-release delays because of the need to have > the API in place for everyone before patches to do the changes can be > sent to multiple maintainers, etc. Sure, that's always true of treewide changes like that.
On Mon, 2020-08-17 at 13:02 -0700, Jens Axboe wrote: > On 8/17/20 12:48 PM, Kees Cook wrote: > > On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: > > > On 8/17/20 12:29 PM, Kees Cook wrote: > > > > On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: > > > > > On 8/17/20 2:15 AM, Allen Pais wrote: > > > > > > From: Allen Pais <allen.lkml@gmail.com> > > > > > > > > > > > > In preparation for unconditionally passing the > > > > > > struct tasklet_struct pointer to all tasklet > > > > > > callbacks, switch to using the new tasklet_setup() > > > > > > and from_tasklet() to pass the tasklet pointer explicitly. > > > > > > > > > > Who came up with the idea to add a macro 'from_tasklet' that > > > > > is just container_of? container_of in the code would be > > > > > _much_ more readable, and not leave anyone guessing wtf > > > > > from_tasklet is doing. > > > > > > > > > > I'd fix that up now before everything else goes in... > > > > > > > > As I mentioned in the other thread, I think this makes things > > > > much more readable. It's the same thing that the timer_struct > > > > conversion did (added a container_of wrapper) to avoid the > > > > ever-repeating use of typeof(), long lines, etc. > > > > > > But then it should use a generic name, instead of each sub-system > > > using some random name that makes people look up exactly what it > > > does. I'm not huge fan of the container_of() redundancy, but > > > adding private variants of this doesn't seem like the best way > > > forward. Let's have a generic helper that does this, and use it > > > everywhere. > > > > I'm open to suggestions, but as things stand, these kinds of > > treewide > > On naming? Implementation is just as it stands, from_tasklet() is > totally generic which is why I objected to it. from_member()? Not > great with naming... But I can see this going further and then we'll > suddenly have tons of these. It's not good for readability. Since both threads seem to have petered out, let me suggest in kernel.h: #define cast_out(ptr, container, member) \ container_of(ptr, typeof(*container), member) It does what you want, the argument order is the same as container_of with the only difference being you name the containing structure instead of having to specify its type. James
On Tue, Aug 18, 2020 at 01:00:33PM -0700, James Bottomley wrote: > On Mon, 2020-08-17 at 13:02 -0700, Jens Axboe wrote: > > On 8/17/20 12:48 PM, Kees Cook wrote: > > > On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: > > > > On 8/17/20 12:29 PM, Kees Cook wrote: > > > > > On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: > > > > > > On 8/17/20 2:15 AM, Allen Pais wrote: > > > > > > > From: Allen Pais <allen.lkml@gmail.com> > > > > > > > > > > > > > > In preparation for unconditionally passing the > > > > > > > struct tasklet_struct pointer to all tasklet > > > > > > > callbacks, switch to using the new tasklet_setup() > > > > > > > and from_tasklet() to pass the tasklet pointer explicitly. > > > > > > > > > > > > Who came up with the idea to add a macro 'from_tasklet' that > > > > > > is just container_of? container_of in the code would be > > > > > > _much_ more readable, and not leave anyone guessing wtf > > > > > > from_tasklet is doing. > > > > > > > > > > > > I'd fix that up now before everything else goes in... > > > > > > > > > > As I mentioned in the other thread, I think this makes things > > > > > much more readable. It's the same thing that the timer_struct > > > > > conversion did (added a container_of wrapper) to avoid the > > > > > ever-repeating use of typeof(), long lines, etc. > > > > > > > > But then it should use a generic name, instead of each sub-system > > > > using some random name that makes people look up exactly what it > > > > does. I'm not huge fan of the container_of() redundancy, but > > > > adding private variants of this doesn't seem like the best way > > > > forward. Let's have a generic helper that does this, and use it > > > > everywhere. > > > > > > I'm open to suggestions, but as things stand, these kinds of > > > treewide > > > > On naming? Implementation is just as it stands, from_tasklet() is > > totally generic which is why I objected to it. from_member()? Not > > great with naming... But I can see this going further and then we'll > > suddenly have tons of these. It's not good for readability. > > Since both threads seem to have petered out, let me suggest in > kernel.h: > > #define cast_out(ptr, container, member) \ > container_of(ptr, typeof(*container), member) > > It does what you want, the argument order is the same as container_of > with the only difference being you name the containing structure > instead of having to specify its type. I like this! Shall I send this to Linus to see if this can land in -rc2 for use going forward?
On Tue, 2020-08-18 at 13:10 -0700, Kees Cook wrote: > On Tue, Aug 18, 2020 at 01:00:33PM -0700, James Bottomley wrote: > > On Mon, 2020-08-17 at 13:02 -0700, Jens Axboe wrote: > > > On 8/17/20 12:48 PM, Kees Cook wrote: > > > > On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: > > > > > On 8/17/20 12:29 PM, Kees Cook wrote: > > > > > > On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: > > > > > > > On 8/17/20 2:15 AM, Allen Pais wrote: > > > > > > > > From: Allen Pais <allen.lkml@gmail.com> > > > > > > > > > > > > > > > > In preparation for unconditionally passing the > > > > > > > > struct tasklet_struct pointer to all tasklet > > > > > > > > callbacks, switch to using the new tasklet_setup() > > > > > > > > and from_tasklet() to pass the tasklet pointer > > > > > > > > explicitly. > > > > > > > > > > > > > > Who came up with the idea to add a macro 'from_tasklet' > > > > > > > that > > > > > > > is just container_of? container_of in the code would be > > > > > > > _much_ more readable, and not leave anyone guessing wtf > > > > > > > from_tasklet is doing. > > > > > > > > > > > > > > I'd fix that up now before everything else goes in... > > > > > > > > > > > > As I mentioned in the other thread, I think this makes > > > > > > things > > > > > > much more readable. It's the same thing that the > > > > > > timer_struct > > > > > > conversion did (added a container_of wrapper) to avoid the > > > > > > ever-repeating use of typeof(), long lines, etc. > > > > > > > > > > But then it should use a generic name, instead of each sub- > > > > > system > > > > > using some random name that makes people look up exactly what > > > > > it > > > > > does. I'm not huge fan of the container_of() redundancy, but > > > > > adding private variants of this doesn't seem like the best > > > > > way > > > > > forward. Let's have a generic helper that does this, and use > > > > > it > > > > > everywhere. > > > > > > > > I'm open to suggestions, but as things stand, these kinds of > > > > treewide > > > > > > On naming? Implementation is just as it stands, from_tasklet() is > > > totally generic which is why I objected to it. from_member()? Not > > > great with naming... But I can see this going further and then > > > we'll > > > suddenly have tons of these. It's not good for readability. > > > > Since both threads seem to have petered out, let me suggest in > > kernel.h: > > > > #define cast_out(ptr, container, member) \ > > container_of(ptr, typeof(*container), member) > > > > It does what you want, the argument order is the same as > > container_of with the only difference being you name the containing > > structure instead of having to specify its type. > > I like this! Shall I send this to Linus to see if this can land in > -rc2 for use going forward? Sure ... he's probably been lurking on this thread anyway ... it's about time he got off his arse^Wthe fence and made an executive decision ... James
> > > > > > > > > > > > > > > > In preparation for unconditionally passing the > > > > > > > > struct tasklet_struct pointer to all tasklet > > > > > > > > callbacks, switch to using the new tasklet_setup() > > > > > > > > and from_tasklet() to pass the tasklet pointer explicitly. > > > > > > > > > > > > > > Who came up with the idea to add a macro 'from_tasklet' that > > > > > > > is just container_of? container_of in the code would be > > > > > > > _much_ more readable, and not leave anyone guessing wtf > > > > > > > from_tasklet is doing. > > > > > > > > > > > > > > I'd fix that up now before everything else goes in... > > > > > > > > > > > > As I mentioned in the other thread, I think this makes things > > > > > > much more readable. It's the same thing that the timer_struct > > > > > > conversion did (added a container_of wrapper) to avoid the > > > > > > ever-repeating use of typeof(), long lines, etc. > > > > > > > > > > But then it should use a generic name, instead of each sub-system > > > > > using some random name that makes people look up exactly what it > > > > > does. I'm not huge fan of the container_of() redundancy, but > > > > > adding private variants of this doesn't seem like the best way > > > > > forward. Let's have a generic helper that does this, and use it > > > > > everywhere. > > > > > > > > I'm open to suggestions, but as things stand, these kinds of > > > > treewide > > > > > > On naming? Implementation is just as it stands, from_tasklet() is > > > totally generic which is why I objected to it. from_member()? Not > > > great with naming... But I can see this going further and then we'll > > > suddenly have tons of these. It's not good for readability. > > > > Since both threads seem to have petered out, let me suggest in > > kernel.h: > > > > #define cast_out(ptr, container, member) \ > > container_of(ptr, typeof(*container), member) > > > > It does what you want, the argument order is the same as container_of > > with the only difference being you name the containing structure > > instead of having to specify its type. > > I like this! Shall I send this to Linus to see if this can land in -rc2 > for use going forward? > Cool, I shall wait for it to be accepted and then spin out V2 with cast_out()
On 8/18/20 1:00 PM, James Bottomley wrote: > On Mon, 2020-08-17 at 13:02 -0700, Jens Axboe wrote: >> On 8/17/20 12:48 PM, Kees Cook wrote: >>> On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: >>>> On 8/17/20 12:29 PM, Kees Cook wrote: >>>>> On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: >>>>>> On 8/17/20 2:15 AM, Allen Pais wrote: >>>>>>> From: Allen Pais <allen.lkml@gmail.com> >>>>>>> >>>>>>> In preparation for unconditionally passing the >>>>>>> struct tasklet_struct pointer to all tasklet >>>>>>> callbacks, switch to using the new tasklet_setup() >>>>>>> and from_tasklet() to pass the tasklet pointer explicitly. >>>>>> >>>>>> Who came up with the idea to add a macro 'from_tasklet' that >>>>>> is just container_of? container_of in the code would be >>>>>> _much_ more readable, and not leave anyone guessing wtf >>>>>> from_tasklet is doing. >>>>>> >>>>>> I'd fix that up now before everything else goes in... >>>>> >>>>> As I mentioned in the other thread, I think this makes things >>>>> much more readable. It's the same thing that the timer_struct >>>>> conversion did (added a container_of wrapper) to avoid the >>>>> ever-repeating use of typeof(), long lines, etc. >>>> >>>> But then it should use a generic name, instead of each sub-system >>>> using some random name that makes people look up exactly what it >>>> does. I'm not huge fan of the container_of() redundancy, but >>>> adding private variants of this doesn't seem like the best way >>>> forward. Let's have a generic helper that does this, and use it >>>> everywhere. >>> >>> I'm open to suggestions, but as things stand, these kinds of >>> treewide >> >> On naming? Implementation is just as it stands, from_tasklet() is >> totally generic which is why I objected to it. from_member()? Not >> great with naming... But I can see this going further and then we'll >> suddenly have tons of these. It's not good for readability. > > Since both threads seem to have petered out, let me suggest in > kernel.h: > > #define cast_out(ptr, container, member) \ > container_of(ptr, typeof(*container), member) > > It does what you want, the argument order is the same as container_of > with the only difference being you name the containing structure > instead of having to specify its type. Not to incessantly bike shed on the naming, but I don't like cast_out, it's not very descriptive. And it has connotations of getting rid of something, which isn't really true. FWIW, I like the from_ part of the original naming, as it has some clues as to what is being done here. Why not just from_container()? That should immediately tell people what it does without having to look up the implementation, even before this becomes a part of the accepted coding norm.
On Wed, Aug 19, 2020 at 07:00:53AM -0600, Jens Axboe wrote: > On 8/18/20 1:00 PM, James Bottomley wrote: > > On Mon, 2020-08-17 at 13:02 -0700, Jens Axboe wrote: > >> On 8/17/20 12:48 PM, Kees Cook wrote: > >>> On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: > >>>> On 8/17/20 12:29 PM, Kees Cook wrote: > >>>>> On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: > >>>>>> On 8/17/20 2:15 AM, Allen Pais wrote: > >>>>>>> From: Allen Pais <allen.lkml@gmail.com> > >>>>>>> > >>>>>>> In preparation for unconditionally passing the > >>>>>>> struct tasklet_struct pointer to all tasklet > >>>>>>> callbacks, switch to using the new tasklet_setup() > >>>>>>> and from_tasklet() to pass the tasklet pointer explicitly. > >>>>>> > >>>>>> Who came up with the idea to add a macro 'from_tasklet' that > >>>>>> is just container_of? container_of in the code would be > >>>>>> _much_ more readable, and not leave anyone guessing wtf > >>>>>> from_tasklet is doing. > >>>>>> > >>>>>> I'd fix that up now before everything else goes in... > >>>>> > >>>>> As I mentioned in the other thread, I think this makes things > >>>>> much more readable. It's the same thing that the timer_struct > >>>>> conversion did (added a container_of wrapper) to avoid the > >>>>> ever-repeating use of typeof(), long lines, etc. > >>>> > >>>> But then it should use a generic name, instead of each sub-system > >>>> using some random name that makes people look up exactly what it > >>>> does. I'm not huge fan of the container_of() redundancy, but > >>>> adding private variants of this doesn't seem like the best way > >>>> forward. Let's have a generic helper that does this, and use it > >>>> everywhere. > >>> > >>> I'm open to suggestions, but as things stand, these kinds of > >>> treewide > >> > >> On naming? Implementation is just as it stands, from_tasklet() is > >> totally generic which is why I objected to it. from_member()? Not > >> great with naming... But I can see this going further and then we'll > >> suddenly have tons of these. It's not good for readability. > > > > Since both threads seem to have petered out, let me suggest in > > kernel.h: > > > > #define cast_out(ptr, container, member) \ > > container_of(ptr, typeof(*container), member) > > > > It does what you want, the argument order is the same as container_of > > with the only difference being you name the containing structure > > instead of having to specify its type. > > Not to incessantly bike shed on the naming, but I don't like cast_out, > it's not very descriptive. And it has connotations of getting rid of > something, which isn't really true. I agree, if we want to bike shed, I don't like this color either. > FWIW, I like the from_ part of the original naming, as it has some clues > as to what is being done here. Why not just from_container()? That > should immediately tell people what it does without having to look up > the implementation, even before this becomes a part of the accepted > coding norm. Why are people hating on the well-known and used container_of()? If you really hate to type the type and want a new macro, what about 'container_from()'? (noun/verb is nicer to sort symbols by...) But really, why is this even needed? thanks, greg k-h
On 8/19/20 6:11 AM, Greg KH wrote: > On Wed, Aug 19, 2020 at 07:00:53AM -0600, Jens Axboe wrote: >> On 8/18/20 1:00 PM, James Bottomley wrote: >>> On Mon, 2020-08-17 at 13:02 -0700, Jens Axboe wrote: >>>> On 8/17/20 12:48 PM, Kees Cook wrote: >>>>> On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: >>>>>> On 8/17/20 12:29 PM, Kees Cook wrote: >>>>>>> On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: >>>>>>>> On 8/17/20 2:15 AM, Allen Pais wrote: >>>>>>>>> From: Allen Pais <allen.lkml@gmail.com> >>>>>>>>> >>>>>>>>> In preparation for unconditionally passing the >>>>>>>>> struct tasklet_struct pointer to all tasklet >>>>>>>>> callbacks, switch to using the new tasklet_setup() >>>>>>>>> and from_tasklet() to pass the tasklet pointer explicitly. >>>>>>>> >>>>>>>> Who came up with the idea to add a macro 'from_tasklet' that >>>>>>>> is just container_of? container_of in the code would be >>>>>>>> _much_ more readable, and not leave anyone guessing wtf >>>>>>>> from_tasklet is doing. >>>>>>>> >>>>>>>> I'd fix that up now before everything else goes in... >>>>>>> >>>>>>> As I mentioned in the other thread, I think this makes things >>>>>>> much more readable. It's the same thing that the timer_struct >>>>>>> conversion did (added a container_of wrapper) to avoid the >>>>>>> ever-repeating use of typeof(), long lines, etc. >>>>>> >>>>>> But then it should use a generic name, instead of each sub-system >>>>>> using some random name that makes people look up exactly what it >>>>>> does. I'm not huge fan of the container_of() redundancy, but >>>>>> adding private variants of this doesn't seem like the best way >>>>>> forward. Let's have a generic helper that does this, and use it >>>>>> everywhere. >>>>> >>>>> I'm open to suggestions, but as things stand, these kinds of >>>>> treewide >>>> >>>> On naming? Implementation is just as it stands, from_tasklet() is >>>> totally generic which is why I objected to it. from_member()? Not >>>> great with naming... But I can see this going further and then we'll >>>> suddenly have tons of these. It's not good for readability. >>> >>> Since both threads seem to have petered out, let me suggest in >>> kernel.h: >>> >>> #define cast_out(ptr, container, member) \ >>> container_of(ptr, typeof(*container), member) >>> >>> It does what you want, the argument order is the same as container_of >>> with the only difference being you name the containing structure >>> instead of having to specify its type. >> >> Not to incessantly bike shed on the naming, but I don't like cast_out, >> it's not very descriptive. And it has connotations of getting rid of >> something, which isn't really true. > > I agree, if we want to bike shed, I don't like this color either. > >> FWIW, I like the from_ part of the original naming, as it has some clues >> as to what is being done here. Why not just from_container()? That >> should immediately tell people what it does without having to look up >> the implementation, even before this becomes a part of the accepted >> coding norm. > > Why are people hating on the well-known and used container_of()? > > If you really hate to type the type and want a new macro, what about > 'container_from()'? (noun/verb is nicer to sort symbols by...) > > But really, why is this even needed? container_from() or from_container(), either works just fine for me in terms of naming. I think people are hating on it because it makes for _really_ long lines, and it's arguably cleaner/simpler to just pass in the pointer type instead. Then you end up with lines like this: struct request_queue *q = container_of(work, struct request_queue, requeue_work.work); But I'm not the one that started this addition of from_tasklet(), my objection was adding a private macro for something that should be generic functionality. Hence I think we either need to provide that, or tell the from_tasklet() folks that they should just use container_of().
On Wed, Aug 19, 2020 at 07:17:19AM -0600, Jens Axboe wrote: > On 8/19/20 6:11 AM, Greg KH wrote: > > On Wed, Aug 19, 2020 at 07:00:53AM -0600, Jens Axboe wrote: > >> On 8/18/20 1:00 PM, James Bottomley wrote: > >>> On Mon, 2020-08-17 at 13:02 -0700, Jens Axboe wrote: > >>>> On 8/17/20 12:48 PM, Kees Cook wrote: > >>>>> On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: > >>>>>> On 8/17/20 12:29 PM, Kees Cook wrote: > >>>>>>> On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: > >>>>>>>> On 8/17/20 2:15 AM, Allen Pais wrote: > >>>>>>>>> From: Allen Pais <allen.lkml@gmail.com> > >>>>>>>>> > >>>>>>>>> In preparation for unconditionally passing the > >>>>>>>>> struct tasklet_struct pointer to all tasklet > >>>>>>>>> callbacks, switch to using the new tasklet_setup() > >>>>>>>>> and from_tasklet() to pass the tasklet pointer explicitly. > >>>>>>>> > >>>>>>>> Who came up with the idea to add a macro 'from_tasklet' that > >>>>>>>> is just container_of? container_of in the code would be > >>>>>>>> _much_ more readable, and not leave anyone guessing wtf > >>>>>>>> from_tasklet is doing. > >>>>>>>> > >>>>>>>> I'd fix that up now before everything else goes in... > >>>>>>> > >>>>>>> As I mentioned in the other thread, I think this makes things > >>>>>>> much more readable. It's the same thing that the timer_struct > >>>>>>> conversion did (added a container_of wrapper) to avoid the > >>>>>>> ever-repeating use of typeof(), long lines, etc. > >>>>>> > >>>>>> But then it should use a generic name, instead of each sub-system > >>>>>> using some random name that makes people look up exactly what it > >>>>>> does. I'm not huge fan of the container_of() redundancy, but > >>>>>> adding private variants of this doesn't seem like the best way > >>>>>> forward. Let's have a generic helper that does this, and use it > >>>>>> everywhere. > >>>>> > >>>>> I'm open to suggestions, but as things stand, these kinds of > >>>>> treewide > >>>> > >>>> On naming? Implementation is just as it stands, from_tasklet() is > >>>> totally generic which is why I objected to it. from_member()? Not > >>>> great with naming... But I can see this going further and then we'll > >>>> suddenly have tons of these. It's not good for readability. > >>> > >>> Since both threads seem to have petered out, let me suggest in > >>> kernel.h: > >>> > >>> #define cast_out(ptr, container, member) \ > >>> container_of(ptr, typeof(*container), member) > >>> > >>> It does what you want, the argument order is the same as container_of > >>> with the only difference being you name the containing structure > >>> instead of having to specify its type. > >> > >> Not to incessantly bike shed on the naming, but I don't like cast_out, > >> it's not very descriptive. And it has connotations of getting rid of > >> something, which isn't really true. > > > > I agree, if we want to bike shed, I don't like this color either. > > > >> FWIW, I like the from_ part of the original naming, as it has some clues > >> as to what is being done here. Why not just from_container()? That > >> should immediately tell people what it does without having to look up > >> the implementation, even before this becomes a part of the accepted > >> coding norm. > > > > Why are people hating on the well-known and used container_of()? > > > > If you really hate to type the type and want a new macro, what about > > 'container_from()'? (noun/verb is nicer to sort symbols by...) > > > > But really, why is this even needed? > > container_from() or from_container(), either works just fine for me > in terms of naming. > > I think people are hating on it because it makes for _really_ long > lines, and it's arguably cleaner/simpler to just pass in the pointer > type instead. Then you end up with lines like this: > > struct request_queue *q = > container_of(work, struct request_queue, requeue_work.work); > > But I'm not the one that started this addition of from_tasklet(), my > objection was adding a private macro for something that should be > generic functionality. Agreed. > Hence I think we either need to provide that, or > tell the from_tasklet() folks that they should just use container_of(). Also agreed, thanks. greg k-h
On Wed, 2020-08-19 at 07:00 -0600, Jens Axboe wrote: > On 8/18/20 1:00 PM, James Bottomley wrote: [...] > > Since both threads seem to have petered out, let me suggest in > > kernel.h: > > > > #define cast_out(ptr, container, member) \ > > container_of(ptr, typeof(*container), member) > > > > It does what you want, the argument order is the same as > > container_of with the only difference being you name the containing > > structure instead of having to specify its type. > > Not to incessantly bike shed on the naming, but I don't like > cast_out, it's not very descriptive. And it has connotations of > getting rid of something, which isn't really true. Um, I thought it was exactly descriptive: you're casting to the outer container. I thought about following the C++ dynamic casting style, so out_cast(), but that seemed a bit pejorative. What about outer_cast()? > FWIW, I like the from_ part of the original naming, as it has some > clues as to what is being done here. Why not just from_container()? > That should immediately tell people what it does without having to > look up the implementation, even before this becomes a part of the > accepted coding norm. I'm not opposed to container_from() but it seems a little less descriptive than outer_cast() but I don't really care. I always have to look up container_of() when I'm using it so this would just be another macro of that type ... James
> [...] > > > Since both threads seem to have petered out, let me suggest in > > > kernel.h: > > > > > > #define cast_out(ptr, container, member) \ > > > container_of(ptr, typeof(*container), member) > > > > > > It does what you want, the argument order is the same as > > > container_of with the only difference being you name the containing > > > structure instead of having to specify its type. > > > > Not to incessantly bike shed on the naming, but I don't like > > cast_out, it's not very descriptive. And it has connotations of > > getting rid of something, which isn't really true. > > Um, I thought it was exactly descriptive: you're casting to the outer > container. I thought about following the C++ dynamic casting style, so > out_cast(), but that seemed a bit pejorative. What about outer_cast()? > > > FWIW, I like the from_ part of the original naming, as it has some > > clues as to what is being done here. Why not just from_container()? > > That should immediately tell people what it does without having to > > look up the implementation, even before this becomes a part of the > > accepted coding norm. > > I'm not opposed to container_from() but it seems a little less > descriptive than outer_cast() but I don't really care. I always have > to look up container_of() when I'm using it so this would just be > another macro of that type ... > So far we have a few which have been suggested as replacement for from_tasklet() - out_cast() or outer_cast() - from_member(). - container_from() or from_container() from_container() sounds fine, would trimming it a bit work? like from_cont().
On 8/19/20 9:24 AM, Allen wrote: >> [...] >>>> Since both threads seem to have petered out, let me suggest in >>>> kernel.h: >>>> >>>> #define cast_out(ptr, container, member) \ >>>> container_of(ptr, typeof(*container), member) >>>> >>>> It does what you want, the argument order is the same as >>>> container_of with the only difference being you name the containing >>>> structure instead of having to specify its type. >>> >>> Not to incessantly bike shed on the naming, but I don't like >>> cast_out, it's not very descriptive. And it has connotations of >>> getting rid of something, which isn't really true. >> >> Um, I thought it was exactly descriptive: you're casting to the outer >> container. I thought about following the C++ dynamic casting style, so >> out_cast(), but that seemed a bit pejorative. What about outer_cast()? >> >>> FWIW, I like the from_ part of the original naming, as it has some >>> clues as to what is being done here. Why not just from_container()? >>> That should immediately tell people what it does without having to >>> look up the implementation, even before this becomes a part of the >>> accepted coding norm. >> >> I'm not opposed to container_from() but it seems a little less >> descriptive than outer_cast() but I don't really care. I always have >> to look up container_of() when I'm using it so this would just be >> another macro of that type ... >> > > So far we have a few which have been suggested as replacement > for from_tasklet() > > - out_cast() or outer_cast() > - from_member(). > - container_from() or from_container() > > from_container() sounds fine, would trimming it a bit work? like from_cont(). I like container_from() the most, since it's the closest to contain_of() which is a well known idiom for years. The lines will already be shorter without the need to specify the struct, so don't like the idea of squeezing container into cont for any of them. For most people, cont is usually short for continue, not container.
On Wed, 2020-08-19 at 21:54 +0530, Allen wrote: > > [...] > > > > Since both threads seem to have petered out, let me suggest in > > > > kernel.h: > > > > > > > > #define cast_out(ptr, container, member) \ > > > > container_of(ptr, typeof(*container), member) > > > > > > > > It does what you want, the argument order is the same as > > > > container_of with the only difference being you name the > > > > containing structure instead of having to specify its type. > > > > > > Not to incessantly bike shed on the naming, but I don't like > > > cast_out, it's not very descriptive. And it has connotations of > > > getting rid of something, which isn't really true. > > > > Um, I thought it was exactly descriptive: you're casting to the > > outer container. I thought about following the C++ dynamic casting > > style, so out_cast(), but that seemed a bit pejorative. What about > > outer_cast()? > > > > > FWIW, I like the from_ part of the original naming, as it has > > > some clues as to what is being done here. Why not just > > > from_container()? That should immediately tell people what it > > > does without having to look up the implementation, even before > > > this becomes a part of the accepted coding norm. > > > > I'm not opposed to container_from() but it seems a little less > > descriptive than outer_cast() but I don't really care. I always > > have to look up container_of() when I'm using it so this would just > > be another macro of that type ... > > > > So far we have a few which have been suggested as replacement > for from_tasklet() > > - out_cast() or outer_cast() > - from_member(). > - container_from() or from_container() > > from_container() sounds fine, would trimming it a bit work? like > from_cont(). I'm fine with container_from(). It's the same form as container_of() and I think we need urgent agreement to not stall everything else so the most innocuous name is likely to get the widest acceptance. James
On Thu, Aug 20, 2020 at 3:09 AM James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > On Wed, 2020-08-19 at 21:54 +0530, Allen wrote: > > > [...] > > > > > Since both threads seem to have petered out, let me suggest in > > > > > kernel.h: > > > > > > > > > > #define cast_out(ptr, container, member) \ > > > > > container_of(ptr, typeof(*container), member) > > > > > > > > > > It does what you want, the argument order is the same as > > > > > container_of with the only difference being you name the > > > > > containing structure instead of having to specify its type. > > > > > > > > Not to incessantly bike shed on the naming, but I don't like > > > > cast_out, it's not very descriptive. And it has connotations of > > > > getting rid of something, which isn't really true. > > > > > > Um, I thought it was exactly descriptive: you're casting to the > > > outer container. I thought about following the C++ dynamic casting > > > style, so out_cast(), but that seemed a bit pejorative. What about > > > outer_cast()? > > > > > > > FWIW, I like the from_ part of the original naming, as it has > > > > some clues as to what is being done here. Why not just > > > > from_container()? That should immediately tell people what it > > > > does without having to look up the implementation, even before > > > > this becomes a part of the accepted coding norm. > > > > > > I'm not opposed to container_from() but it seems a little less > > > descriptive than outer_cast() but I don't really care. I always > > > have to look up container_of() when I'm using it so this would just > > > be another macro of that type ... > > > > > > > So far we have a few which have been suggested as replacement > > for from_tasklet() > > > > - out_cast() or outer_cast() > > - from_member(). > > - container_from() or from_container() > > > > from_container() sounds fine, would trimming it a bit work? like > > from_cont(). > > I'm fine with container_from(). It's the same form as container_of() > and I think we need urgent agreement to not stall everything else so > the most innocuous name is likely to get the widest acceptance. Kees, Will you be sending the newly proposed API to Linus? I have V2 which uses container_from() ready to be sent out. Thanks.
On Wed, Aug 26, 2020 at 07:21:35AM +0530, Allen Pais wrote: > On Thu, Aug 20, 2020 at 3:09 AM James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > > > On Wed, 2020-08-19 at 21:54 +0530, Allen wrote: > > > > [...] > > > > > > Since both threads seem to have petered out, let me suggest in > > > > > > kernel.h: > > > > > > > > > > > > #define cast_out(ptr, container, member) \ > > > > > > container_of(ptr, typeof(*container), member) > > > > > > > > > > > > It does what you want, the argument order is the same as > > > > > > container_of with the only difference being you name the > > > > > > containing structure instead of having to specify its type. > > > > > > > > > > Not to incessantly bike shed on the naming, but I don't like > > > > > cast_out, it's not very descriptive. And it has connotations of > > > > > getting rid of something, which isn't really true. > > > > > > > > Um, I thought it was exactly descriptive: you're casting to the > > > > outer container. I thought about following the C++ dynamic casting > > > > style, so out_cast(), but that seemed a bit pejorative. What about > > > > outer_cast()? > > > > > > > > > FWIW, I like the from_ part of the original naming, as it has > > > > > some clues as to what is being done here. Why not just > > > > > from_container()? That should immediately tell people what it > > > > > does without having to look up the implementation, even before > > > > > this becomes a part of the accepted coding norm. > > > > > > > > I'm not opposed to container_from() but it seems a little less > > > > descriptive than outer_cast() but I don't really care. I always > > > > have to look up container_of() when I'm using it so this would just > > > > be another macro of that type ... > > > > > > > > > > So far we have a few which have been suggested as replacement > > > for from_tasklet() > > > > > > - out_cast() or outer_cast() > > > - from_member(). > > > - container_from() or from_container() > > > > > > from_container() sounds fine, would trimming it a bit work? like > > > from_cont(). > > > > I'm fine with container_from(). It's the same form as container_of() > > and I think we need urgent agreement to not stall everything else so > > the most innocuous name is likely to get the widest acceptance. > > Kees, > > Will you be sending the newly proposed API to Linus? I have V2 > which uses container_from() > ready to be sent out. I liked that James swapped the first two arguments so that it matches container_of(). Plus it's nice that when you have: struct whatever *foo = container_from(ptr, foo, member); Then it means that "ptr == &foo->member". regards, dan carpenter
On Wed, Aug 26, 2020 at 12:55:28PM +0300, Dan Carpenter wrote: > On Wed, Aug 26, 2020 at 07:21:35AM +0530, Allen Pais wrote: > > On Thu, Aug 20, 2020 at 3:09 AM James Bottomley > > <James.Bottomley@hansenpartnership.com> wrote: > > > > > > On Wed, 2020-08-19 at 21:54 +0530, Allen wrote: > > > > > [...] > > > > > > > Since both threads seem to have petered out, let me suggest in > > > > > > > kernel.h: > > > > > > > > > > > > > > #define cast_out(ptr, container, member) \ > > > > > > > container_of(ptr, typeof(*container), member) > > > > > > > > > > > > > > It does what you want, the argument order is the same as > > > > > > > container_of with the only difference being you name the > > > > > > > containing structure instead of having to specify its type. > > > > > > > > > > > > Not to incessantly bike shed on the naming, but I don't like > > > > > > cast_out, it's not very descriptive. And it has connotations of > > > > > > getting rid of something, which isn't really true. > > > > > > > > > > Um, I thought it was exactly descriptive: you're casting to the > > > > > outer container. I thought about following the C++ dynamic casting > > > > > style, so out_cast(), but that seemed a bit pejorative. What about > > > > > outer_cast()? > > > > > > > > > > > FWIW, I like the from_ part of the original naming, as it has > > > > > > some clues as to what is being done here. Why not just > > > > > > from_container()? That should immediately tell people what it > > > > > > does without having to look up the implementation, even before > > > > > > this becomes a part of the accepted coding norm. > > > > > > > > > > I'm not opposed to container_from() but it seems a little less > > > > > descriptive than outer_cast() but I don't really care. I always > > > > > have to look up container_of() when I'm using it so this would just > > > > > be another macro of that type ... > > > > > > > > > > > > > So far we have a few which have been suggested as replacement > > > > for from_tasklet() > > > > > > > > - out_cast() or outer_cast() > > > > - from_member(). > > > > - container_from() or from_container() > > > > > > > > from_container() sounds fine, would trimming it a bit work? like > > > > from_cont(). > > > > > > I'm fine with container_from(). It's the same form as container_of() > > > and I think we need urgent agreement to not stall everything else so > > > the most innocuous name is likely to get the widest acceptance. > > > > Kees, > > > > Will you be sending the newly proposed API to Linus? I have V2 > > which uses container_from() > > ready to be sent out. > > I liked that James swapped the first two arguments so that it matches > container_of(). Plus it's nice that when you have: > > struct whatever *foo = container_from(ptr, foo, member); > > Then it means that "ptr == &foo->member". I'm a bit stalled right now -- the merge window was keeping me busy, and this week is the Linux Plumbers Conference. This is on my list, but I haven't gotten back around to it. If you want, feel free to send the container_from() patch; you might be able to unblock this faster than me right now. :) -Kees
On Wed, Aug 26, 2020 at 8:43 PM Kees Cook <keescook@chromium.org> wrote: > > On Wed, Aug 26, 2020 at 12:55:28PM +0300, Dan Carpenter wrote: > > On Wed, Aug 26, 2020 at 07:21:35AM +0530, Allen Pais wrote: > > > On Thu, Aug 20, 2020 at 3:09 AM James Bottomley > > > <James.Bottomley@hansenpartnership.com> wrote: > > > > > > > > On Wed, 2020-08-19 at 21:54 +0530, Allen wrote: > > > > > > [...] > > > > > > > > Since both threads seem to have petered out, let me suggest in > > > > > > > > kernel.h: > > > > > > > > > > > > > > > > #define cast_out(ptr, container, member) \ > > > > > > > > container_of(ptr, typeof(*container), member) > > > > > > > > > > > > > > > > It does what you want, the argument order is the same as > > > > > > > > container_of with the only difference being you name the > > > > > > > > containing structure instead of having to specify its type. > > > > > > > > > > > > > > Not to incessantly bike shed on the naming, but I don't like > > > > > > > cast_out, it's not very descriptive. And it has connotations of > > > > > > > getting rid of something, which isn't really true. > > > > > > > > > > > > Um, I thought it was exactly descriptive: you're casting to the > > > > > > outer container. I thought about following the C++ dynamic casting > > > > > > style, so out_cast(), but that seemed a bit pejorative. What about > > > > > > outer_cast()? > > > > > > > > > > > > > FWIW, I like the from_ part of the original naming, as it has > > > > > > > some clues as to what is being done here. Why not just > > > > > > > from_container()? That should immediately tell people what it > > > > > > > does without having to look up the implementation, even before > > > > > > > this becomes a part of the accepted coding norm. > > > > > > > > > > > > I'm not opposed to container_from() but it seems a little less > > > > > > descriptive than outer_cast() but I don't really care. I always > > > > > > have to look up container_of() when I'm using it so this would just > > > > > > be another macro of that type ... > > > > > > > > > > > > > > > > So far we have a few which have been suggested as replacement > > > > > for from_tasklet() > > > > > > > > > > - out_cast() or outer_cast() > > > > > - from_member(). > > > > > - container_from() or from_container() > > > > > > > > > > from_container() sounds fine, would trimming it a bit work? like > > > > > from_cont(). > > > > > > > > I'm fine with container_from(). It's the same form as container_of() > > > > and I think we need urgent agreement to not stall everything else so > > > > the most innocuous name is likely to get the widest acceptance. > > > > > > Kees, > > > > > > Will you be sending the newly proposed API to Linus? I have V2 > > > which uses container_from() > > > ready to be sent out. > > > > I liked that James swapped the first two arguments so that it matches > > container_of(). Plus it's nice that when you have: > > > > struct whatever *foo = container_from(ptr, foo, member); > > > > Then it means that "ptr == &foo->member". > > I'm a bit stalled right now -- the merge window was keeping me busy, and > this week is the Linux Plumbers Conference. This is on my list, but I > haven't gotten back around to it. If you want, feel free to send the > container_from() patch; you might be able to unblock this faster than me > right now. :) > Sure, Thanks.
diff --git a/drivers/block/umem.c b/drivers/block/umem.c index 2b95d7b33b91..320781d5d156 100644 --- a/drivers/block/umem.c +++ b/drivers/block/umem.c @@ -405,7 +405,7 @@ static int add_bio(struct cardinfo *card) return 1; } -static void process_page(unsigned long data) +static void process_page(struct tasklet_struct *t) { /* check if any of the requests in the page are DMA_COMPLETE, * and deal with them appropriately. @@ -415,7 +415,7 @@ static void process_page(unsigned long data) */ struct mm_page *page; struct bio *return_bio = NULL; - struct cardinfo *card = (struct cardinfo *)data; + struct cardinfo *card = from_tasklet(card, t, tasklet); unsigned int dma_status = card->dma_status; spin_lock(&card->lock); @@ -891,7 +891,7 @@ static int mm_pci_probe(struct pci_dev *dev, const struct pci_device_id *id) if (!card->queue) goto failed_alloc; - tasklet_init(&card->tasklet, process_page, (unsigned long)card); + tasklet_setup(&card->tasklet, process_page); card->check_batteries = 0; diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c index 5d8e0ab3f054..bdd50a87d10f 100644 --- a/drivers/block/xsysace.c +++ b/drivers/block/xsysace.c @@ -762,9 +762,9 @@ static void ace_fsm_dostate(struct ace_device *ace) } } -static void ace_fsm_tasklet(unsigned long data) +static void ace_fsm_tasklet(struct tasklet_struct *t) { - struct ace_device *ace = (void *)data; + struct ace_device *ace = from_tasklet(ace, t, fsm_tasklet); unsigned long flags; spin_lock_irqsave(&ace->lock, flags); @@ -1001,7 +1001,7 @@ static int ace_setup(struct ace_device *ace) /* * Initialize the state machine tasklet and stall timer */ - tasklet_init(&ace->fsm_tasklet, ace_fsm_tasklet, (unsigned long)ace); + tasklet_setup(&ace->fsm_tasklet, ace_fsm_tasklet); timer_setup(&ace->stall_timer, ace_stall_timer, 0); /*