Message ID | 20190929163028.9665-1-romain.perier@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | Modernize the tasklet API | expand |
On Sun, Sep 29, 2019 at 06:30:12PM +0200, Romain Perier wrote: > Hello, > > Nowadays, modern kernel subsystems that use callbacks pass the data > structure associated with a given callback as argument to the callback. > The data structure specific to the driver or subsystem that uses this > callback framework is usually "derivated" from the data structure that > is passed as argument to the callback. > > The tasklet subsystem remains the one to pass callback argument as an > arbitrary unsigned long argument (This has several issues that are > explained in the first commit). > > This series aims to improve the tasklet API and converts all the code > that is using it. It is based on the series for timer_list at [1]. > > 1. https://lore.kernel.org/patchwork/patch/835464 > > > Romain Perier (16): > tasklet: Prepare to change tasklet callback argument type > crypto: ccp - Prepare to use the new tasklet API > mmc: renesas_sdhi: Prepare to use the new tasklet API > net: liquidio: Prepare to use the new tasklet API > chelsio: Prepare to use the new tasklet API > net: mvpp2: Prepare to use the new tasklet API > qed: Prepare to use the new tasklet API > isdn: Prepare to use the new tasklet API > scsi: pm8001: Prepare to use the new tasklet API > scsi: pmcraid: Prepare to use the new tasklet API > treewide: Globally replace tasklet_init() by tasklet_setup() > tasklet: Pass tasklet_struct pointer as .data in DECLARE_TASKLET > tasklet: Pass tasklet_struct pointer to callbacks unconditionally > tasklet: Remove the data argument from DECLARE_TASKLET() macros > tasklet: convert callbacks prototype for using struct tasklet_struct * > arguments > tasklet: Add the new initialization function permanently This is looking really good; thank you! I think for easier review it would make sense to break out the "special" cases (where you're changing structures, etc) into their own patches (and not as a bulk change -- they need review by different subsystem maintainers, etc). Then the patch phases can be: 1) Introduce new APIs and casts 2) Convert special cases include passing the tasklet as their .data (while also changing the prototypes and replacing tasklet_init() with tasklet_setup()) 3) Convert DECLARE_TASKLET() users to the same 4) Manual one-off conversions of tasklet_init() -> tasklet_setup() 5) Mechanical mass conversion of tasklet_init() -> tasklet_setup() 6) Mass removal of .data argument from DECLARE_TASKLET() 7) tasklet API internal swap and removal of .data 8) tasklet_init() and helper cast removals. Step 1 needs to happen in an -rc1 (e.g. v5.5-rc1). Then steps 2, 3, and 4 can happen simultaneously across all the maintainers that need to be aware of it and land in the next release (the linux-next for v5.6). Finally steps 5, 6, 7, and 8 happen in the next release's -rc1 (v5.6-rc1). If we can get the "phase 1" patch ready quick, maybe we can get into -rc2 for v5.4 and move things up by a release... -Kees
On Mon, Sep 30, 2019 at 04:06:50PM -0700, Kees Cook wrote: > On Sun, Sep 29, 2019 at 06:30:12PM +0200, Romain Perier wrote: > This is looking really good; thank you! Glad to read this :) > I think for easier review it > would make sense to break out the "special" cases (where you're changing > structures, etc) into their own patches (and not as a bulk change -- > they need review by different subsystem maintainers, etc). > > Then the patch phases can be: > > 1) Introduce new APIs and casts > 2) Convert special cases include passing the tasklet as their .data > (while also changing the prototypes and replacing tasklet_init() with > tasklet_setup()) => So, one commit per driver for preparing the data structure to own a sub "struct tasklet_struct" + tasklet_init() -> tasklet_setup() with use of "from_tasklet" in the same commit. Right ? For example: the commit "[PRE-REVIEW PATCH 03/16] mmc: renesas_sdhi: Prepare to use the new tasklet API" would contain changes for preparing the driver to use a "struct tasklet_struct" correctly + convert the driver to the new API (tasklet_init() -> tasklet_setup()) Same for commit "[PRE-REVIEW PATCH 04/16] net: liquidio: Prepare to use the new tasklet API". This is what you had in mind ? > 3) Convert DECLARE_TASKLET() users to the same Yeah, this is what you explain in reply to "[PRE-REVIEW PATCH 12/16] tasklet: Pass tasklet_struct pointer as .data in DECLARE_TASKLET", right ? > 4) Manual one-off conversions of tasklet_init() -> tasklet_setup() > 5) Mechanical mass conversion of tasklet_init() -> tasklet_setup() See the reply to the commit "treewide:" about this > 6) Mass removal of .data argument from DECLARE_TASKLET() > 7) tasklet API internal swap and removal of .data > 8) tasklet_init() and helper cast removals. Ack > > Step 1 needs to happen in an -rc1 (e.g. v5.5-rc1). > > Then steps 2, 3, and 4 can happen simultaneously across all the > maintainers that need to be aware of it and land in the next release > (the linux-next for v5.6). > > Finally steps 5, 6, 7, and 8 happen in the next release's -rc1 > (v5.6-rc1). > > If we can get the "phase 1" patch ready quick, maybe we can get into > -rc2 for v5.4 and move things up by a release... > > -Kees > Thanks for your time, Regards, Romain > -- > Kees Cook
On Tue, Oct 01, 2019 at 07:47:52PM +0200, Romain Perier wrote: > => So, one commit per driver for preparing the data structure to > own a sub "struct tasklet_struct" + tasklet_init() -> > tasklet_setup() with use of "from_tasklet" in the same commit. > Right ? > > For example: > the commit "[PRE-REVIEW PATCH 03/16] mmc: > renesas_sdhi: Prepare to use the new tasklet API" > > would contain changes for preparing the driver to use a > "struct tasklet_struct" correctly + convert the driver to > the new API (tasklet_init() -> tasklet_setup()) > > Same for commit "[PRE-REVIEW PATCH 04/16] net: liquidio: Prepare > to use the new tasklet API". > > This is what you had in mind ? Right -- the commit would be complete from a "touch-once" perspective. The one commit completely swaps to the new API and nothing will ever have to poke this file again. (The exception being any explicit casts that might be rarely needed for weird direct initialization, etc: most patches should be entirely self-contained, though they depend on the new API from patch #1.) > > 3) Convert DECLARE_TASKLET() users to the same > > Yeah, this is what you explain in reply to "[PRE-REVIEW PATCH 12/16] > tasklet: Pass tasklet_struct pointer as .data in DECLARE_TASKLET", right > ? Right -- after all the other stuff, do the the DECLARE_TASKLET changes, with a final mechanical patch that drops the unused .data argument from all the users. This is all a normally thankless set of patches, so I'll go out of my way to say again: thank you for working on this! I know how tedious it can be from when I did timer_struct. :)
Romain, > > This is all a normally thankless set of patches, so I'll go out of my > way to say again: thank you for working on this! I know how tedious it > can be from when I did timer_struct. :) First of all Romain, nice work. I started working on this set a few months back, but could only carve out limited time. I sent out RFC for this sometime in May[1]. And my approach was a little different when compared to what you have sent on the list. Well, I have pushed my work to github[2], only thing I could think of as an improvement in your patch set it to break it down into smaller chunk so that it's easier to review. I have made each occurrence of tasklet_init() into a commit[3] which I thought would make it easier to review. I'll leave that decision to you and kees. Let me know if I could help in any way. [1] https://www.openwall.com/lists/kernel-hardening/2019/05/06/1 [2] https://github.com/allenpais/tasklet [3] Sample list of patches: 5d0b728649b6 atm/solos-pci: Convert tasklets to use new tasklet_init API e5144c3c16d8 atm: Convert tasklets to use new tasklet_init API 71028976d3ed arch/um: Convert tasklets to use new tasklet_init API c9a39c23b78c xfrm: Convert tasklets to use new tasklet_init API 91d93fe12bbc mac80211: Convert tasklets to use new tasklet_init API d68f1e9e4531 ipv4: Convert tasklets to use new tasklet_init API 4f9379dcd8ad sound/timer: Convert tasklets to use new tasklet_init API b4519111b75e drivers/usb: Convert tasklets to use new tasklet_init API 52f04bf54a5a drivers:vt/keyboard: Convert tasklets to use new tasklet_init API 295de7c9812c dma/virt-dma: Convert tasklets to use new tasklet_init API 6c713c83b58f dma/dw: Convert tasklets to use new tasklet_init API eaaaaba8a4a7 debug:Convert tasklets to use new tasklet_init API b23f4ff5021b tasklet: prepare to change tasklet API Thanks, - Allen
Le mer. 30 oct. 2019 à 09:21, Allen <allen.lkml@gmail.com> a écrit : > > Romain, > > > > First of all Romain, nice work. I started working on this > set a few months back, but could only carve out limited time. > > I sent out RFC for this sometime in May[1]. And my approach > was a little different when compared to what you have sent on the > list. > > Well, I have pushed my work to github[2], only thing I could > think of as an improvement in your patch set it to break it down > into smaller chunk so that it's easier to review. I have made each > occurrence of tasklet_init() into a commit[3] which I thought would > make it easier to review. I'll leave that decision to you and kees. > > Let me know if I could help in any way. > > [1] https://www.openwall.com/lists/kernel-hardening/2019/05/06/1 > [2] https://github.com/allenpais/tasklet > [3] Sample list of patches: > 5d0b728649b6 atm/solos-pci: Convert tasklets to use new tasklet_init API > e5144c3c16d8 atm: Convert tasklets to use new tasklet_init API > 71028976d3ed arch/um: Convert tasklets to use new tasklet_init API > c9a39c23b78c xfrm: Convert tasklets to use new tasklet_init API > 91d93fe12bbc mac80211: Convert tasklets to use new tasklet_init API > d68f1e9e4531 ipv4: Convert tasklets to use new tasklet_init API > 4f9379dcd8ad sound/timer: Convert tasklets to use new tasklet_init API > b4519111b75e drivers/usb: Convert tasklets to use new tasklet_init API > 52f04bf54a5a drivers:vt/keyboard: Convert tasklets to use new tasklet_init API > 295de7c9812c dma/virt-dma: Convert tasklets to use new tasklet_init API > 6c713c83b58f dma/dw: Convert tasklets to use new tasklet_init API > eaaaaba8a4a7 debug:Convert tasklets to use new tasklet_init API > b23f4ff5021b tasklet: prepare to change tasklet API From experience, this is better to group bunch of commits like we currently do with Kees on this series, instead to have one commit per change (I mean for huge patchset) Mainly because you have too much replacements with this API change, and it will be really complicated to merge. Last time I have proposed an API change for removing "pci_pool" , it was a patchset of 20 commits (something like this), it tooks 6 months to be merged :) (with a fine grain granularity on each commit) This is better to be the more atomic as possible. If we split the "one massive tasklet_init replacement" commit into many commit, I am sure that we find old tasklet API for months in the kernel... it is not something we want , imho. + treewide commits are common in the kernel tree, for important API changes :) @Kees: agreed ? I think that the timer_list approach is good. You can help by providing feedbacks and by testing if you want. Regards, Romain > > Thanks, > - Allen
On Thu, Nov 07, 2019 at 08:29:40AM +0100, Romain Perier wrote: > Le mer. 30 oct. 2019 à 09:21, Allen <allen.lkml@gmail.com> a écrit : > > > > Romain, > > > > > > > First of all Romain, nice work. I started working on this > > set a few months back, but could only carve out limited time. > > > > I sent out RFC for this sometime in May[1]. And my approach > > was a little different when compared to what you have sent on the > > list. > > > > Well, I have pushed my work to github[2], only thing I could > > think of as an improvement in your patch set it to break it down > > into smaller chunk so that it's easier to review. I have made each > > occurrence of tasklet_init() into a commit[3] which I thought would > > make it easier to review. I'll leave that decision to you and kees. > > > > Let me know if I could help in any way. > > > > [1] https://www.openwall.com/lists/kernel-hardening/2019/05/06/1 > > [2] https://github.com/allenpais/tasklet > > [3] Sample list of patches: > > 5d0b728649b6 atm/solos-pci: Convert tasklets to use new tasklet_init API > > e5144c3c16d8 atm: Convert tasklets to use new tasklet_init API > > 71028976d3ed arch/um: Convert tasklets to use new tasklet_init API > > c9a39c23b78c xfrm: Convert tasklets to use new tasklet_init API > > 91d93fe12bbc mac80211: Convert tasklets to use new tasklet_init API > > d68f1e9e4531 ipv4: Convert tasklets to use new tasklet_init API > > 4f9379dcd8ad sound/timer: Convert tasklets to use new tasklet_init API > > b4519111b75e drivers/usb: Convert tasklets to use new tasklet_init API > > 52f04bf54a5a drivers:vt/keyboard: Convert tasklets to use new tasklet_init API > > 295de7c9812c dma/virt-dma: Convert tasklets to use new tasklet_init API > > 6c713c83b58f dma/dw: Convert tasklets to use new tasklet_init API > > eaaaaba8a4a7 debug:Convert tasklets to use new tasklet_init API > > b23f4ff5021b tasklet: prepare to change tasklet API > > From experience, this is better to group bunch of commits like we > currently do with Kees on this series, instead to have one commit per > change (I mean for huge patchset) > Mainly because you have too much replacements with this API change, > and it will be really complicated to merge. > > Last time I have proposed an API change for removing "pci_pool" , it > was a patchset of 20 commits (something like this), it tooks 6 months > to be merged :) (with a fine grain granularity on each commit) > > This is better to be the more atomic as possible. If we split the "one > massive tasklet_init replacement" commit into many commit, I am sure > that we find old tasklet API for months in the kernel... it is not > something we want , imho. + treewide commits are common in the kernel > tree, for important API changes :) > > @Kees: agreed ? > > I think that the timer_list approach is good. You can help by > providing feedbacks and by testing if you want. It worked well the last time. :) I think splitting the non-mechanical changes and landing those first is the right approach. Then we can land a massive treewide for all the "easy" cases without trickling them in over months. -Kees