On Wed, Aug 17, 2022 at 08:39:12PM +0800, Duoming Zhou wrote:
> The dev_coredumpv() and dev_coredumpm() could not be used in atomic
> context, because they call kvasprintf_const() and kstrdup() with
> GFP_KERNEL parameter. The process is shown below:
>
> dev_coredumpv(.., gfp_t gfp)
> dev_coredumpm(.., gfp_t gfp)
> dev_set_name
> kobject_set_name_vargs
> kvasprintf_const(GFP_KERNEL, ...); //may sleep
> kstrdup(s, GFP_KERNEL); //may sleep
>
> This patch removes gfp_t parameter of dev_coredumpv() and dev_coredumpm()
> and changes the gfp_t parameter of kzalloc() in dev_coredumpm() to
> GFP_KERNEL in order to show they could not be used in atomic context.
>
> Fixes: 833c95456a70 ("device coredump: add new device coredump class")
> Reviewed-by: Brian Norris <[email protected]>
> Reviewed-by: Johannes Berg <[email protected]>
> Signed-off-by: Duoming Zhou <[email protected]>
> ---
> Changes in v7:
> - Remove gfp_t flag in amdgpu device.
Again, this creates a "flag day" where we have to be sure we hit all
users of this api at the exact same time. This will prevent any new
driver that comes into a maintainer tree during the next 3 months from
ever being able to use this api without cauing build breakages in the
linux-next tree.
Please evolve this api to work properly for everyone at the same time,
like was previously asked for so that we can take this change. It will
take 2 releases, but that's fine.
thanks,
greg k-h
Hello,
On Wed, 17 Aug 2022 14:43:29 +0200 Greg KH wrote:
> On Wed, Aug 17, 2022 at 08:39:12PM +0800, Duoming Zhou wrote:
> > The dev_coredumpv() and dev_coredumpm() could not be used in atomic
> > context, because they call kvasprintf_const() and kstrdup() with
> > GFP_KERNEL parameter. The process is shown below:
> >
> > dev_coredumpv(.., gfp_t gfp)
> > dev_coredumpm(.., gfp_t gfp)
> > dev_set_name
> > kobject_set_name_vargs
> > kvasprintf_const(GFP_KERNEL, ...); //may sleep
> > kstrdup(s, GFP_KERNEL); //may sleep
> >
> > This patch removes gfp_t parameter of dev_coredumpv() and dev_coredumpm()
> > and changes the gfp_t parameter of kzalloc() in dev_coredumpm() to
> > GFP_KERNEL in order to show they could not be used in atomic context.
> >
> > Fixes: 833c95456a70 ("device coredump: add new device coredump class")
> > Reviewed-by: Brian Norris <[email protected]>
> > Reviewed-by: Johannes Berg <[email protected]>
> > Signed-off-by: Duoming Zhou <[email protected]>
> > ---
> > Changes in v7:
> > - Remove gfp_t flag in amdgpu device.
>
> Again, this creates a "flag day" where we have to be sure we hit all
> users of this api at the exact same time. This will prevent any new
> driver that comes into a maintainer tree during the next 3 months from
> ever being able to use this api without cauing build breakages in the
> linux-next tree.
>
> Please evolve this api to work properly for everyone at the same time,
> like was previously asked for so that we can take this change. It will
> take 2 releases, but that's fine.
Thank you for your reply, I will evolve this api to work properly for everyone.
If there are not any new drivers that use this api during the next 3 months,
I will send this patch again. Otherwise, I will wait until there are not new
users anymore.
Best regards,
Duoming Zhou
On Wed, Aug 17, 2022 at 10:05:37PM +0800, [email protected] wrote:
> Hello,
>
> On Wed, 17 Aug 2022 14:43:29 +0200 Greg KH wrote:
>
> > On Wed, Aug 17, 2022 at 08:39:12PM +0800, Duoming Zhou wrote:
> > > The dev_coredumpv() and dev_coredumpm() could not be used in atomic
> > > context, because they call kvasprintf_const() and kstrdup() with
> > > GFP_KERNEL parameter. The process is shown below:
> > >
> > > dev_coredumpv(.., gfp_t gfp)
> > > dev_coredumpm(.., gfp_t gfp)
> > > dev_set_name
> > > kobject_set_name_vargs
> > > kvasprintf_const(GFP_KERNEL, ...); //may sleep
> > > kstrdup(s, GFP_KERNEL); //may sleep
> > >
> > > This patch removes gfp_t parameter of dev_coredumpv() and dev_coredumpm()
> > > and changes the gfp_t parameter of kzalloc() in dev_coredumpm() to
> > > GFP_KERNEL in order to show they could not be used in atomic context.
> > >
> > > Fixes: 833c95456a70 ("device coredump: add new device coredump class")
> > > Reviewed-by: Brian Norris <[email protected]>
> > > Reviewed-by: Johannes Berg <[email protected]>
> > > Signed-off-by: Duoming Zhou <[email protected]>
> > > ---
> > > Changes in v7:
> > > - Remove gfp_t flag in amdgpu device.
> >
> > Again, this creates a "flag day" where we have to be sure we hit all
> > users of this api at the exact same time. This will prevent any new
> > driver that comes into a maintainer tree during the next 3 months from
> > ever being able to use this api without cauing build breakages in the
> > linux-next tree.
> >
> > Please evolve this api to work properly for everyone at the same time,
> > like was previously asked for so that we can take this change. It will
> > take 2 releases, but that's fine.
>
> Thank you for your reply, I will evolve this api to work properly for everyone.
> If there are not any new drivers that use this api during the next 3 months,
> I will send this patch again. Otherwise, I will wait until there are not new
> users anymore.
No, that is not necessary. Do the work now so that there is no flag day
and you don't have to worry about new users, it will all "just work".
thanks,
greg k-h
Hello,
On Thu, 18 Aug 2022 16:58:01 +0200 Greg KH wrote:
> On Wed, Aug 17, 2022 at 10:05:37PM +0800, [email protected] wrote:
> > Hello,
> >
> > On Wed, 17 Aug 2022 14:43:29 +0200 Greg KH wrote:
> >
> > > On Wed, Aug 17, 2022 at 08:39:12PM +0800, Duoming Zhou wrote:
> > > > The dev_coredumpv() and dev_coredumpm() could not be used in atomic
> > > > context, because they call kvasprintf_const() and kstrdup() with
> > > > GFP_KERNEL parameter. The process is shown below:
> > > >
> > > > dev_coredumpv(.., gfp_t gfp)
> > > > dev_coredumpm(.., gfp_t gfp)
> > > > dev_set_name
> > > > kobject_set_name_vargs
> > > > kvasprintf_const(GFP_KERNEL, ...); //may sleep
> > > > kstrdup(s, GFP_KERNEL); //may sleep
> > > >
> > > > This patch removes gfp_t parameter of dev_coredumpv() and dev_coredumpm()
> > > > and changes the gfp_t parameter of kzalloc() in dev_coredumpm() to
> > > > GFP_KERNEL in order to show they could not be used in atomic context.
> > > >
> > > > Fixes: 833c95456a70 ("device coredump: add new device coredump class")
> > > > Reviewed-by: Brian Norris <[email protected]>
> > > > Reviewed-by: Johannes Berg <[email protected]>
> > > > Signed-off-by: Duoming Zhou <[email protected]>
> > > > ---
> > > > Changes in v7:
> > > > - Remove gfp_t flag in amdgpu device.
> > >
> > > Again, this creates a "flag day" where we have to be sure we hit all
> > > users of this api at the exact same time. This will prevent any new
> > > driver that comes into a maintainer tree during the next 3 months from
> > > ever being able to use this api without cauing build breakages in the
> > > linux-next tree.
> > >
> > > Please evolve this api to work properly for everyone at the same time,
> > > like was previously asked for so that we can take this change. It will
> > > take 2 releases, but that's fine.
> >
> > Thank you for your reply, I will evolve this api to work properly for everyone.
> > If there are not any new drivers that use this api during the next 3 months,
> > I will send this patch again. Otherwise, I will wait until there are not new
> > users anymore.
>
> No, that is not necessary. Do the work now so that there is no flag day
> and you don't have to worry about new users, it will all "just work".
Do you mean we should replace dev_set_name() in dev_coredumpm() to some other
functions that could work both in interrupt context and process context?
Best regards,
Duoming Zhou
Hi,
On Thu, Aug 18, 2022 at 8:47 AM <[email protected]> wrote:
> On Thu, 18 Aug 2022 16:58:01 +0200 Greg KH wrote:
> > No, that is not necessary. Do the work now so that there is no flag day
> > and you don't have to worry about new users, it will all "just work".
>
> Do you mean we should replace dev_set_name() in dev_coredumpm() to some other
> functions that could work both in interrupt context and process context?
No.
I believe the suggestion is that rather than change the signature for
dev_coredumpv() (which means everyone has to agree on the new
signature on day 1), you should introduce a new API, like
dev_coredumpv_noatomic() (I'm not good at naming [1]) with the
signature you want, and then migrate users over. Once we have a
release with no users of the old API, we drop it.
There are plenty of examples of the kernel community doing similar
transitions. You can search around for examples, but a quick search of
my own shows something like this:
https://lwn.net/Articles/735887/
(In particular, timer_setup() was introduced, and all setup_timer()
users were migrated to it within a release or two.)
Brian
[1] Seriously, dev_coredumpv_noatomic() is not a name I want to see
last very long. Maybe some other trivial modification? Examples:
dev_core_dumpv()
dev_coredump_v()
device_coredumpv()
...
Hello,
On Mon, 22 Aug 2022 11:21:36 -0700 Brian Norris wrote:
> Hi,
>
> On Thu, Aug 18, 2022 at 8:47 AM <[email protected]> wrote:
> > On Thu, 18 Aug 2022 16:58:01 +0200 Greg KH wrote:
> > > No, that is not necessary. Do the work now so that there is no flag day
> > > and you don't have to worry about new users, it will all "just work".
> >
> > Do you mean we should replace dev_set_name() in dev_coredumpm() to some other
> > functions that could work both in interrupt context and process context?
>
> No.
>
> I believe the suggestion is that rather than change the signature for
> dev_coredumpv() (which means everyone has to agree on the new
> signature on day 1), you should introduce a new API, like
> dev_coredumpv_noatomic() (I'm not good at naming [1]) with the
> signature you want, and then migrate users over. Once we have a
> release with no users of the old API, we drop it.
>
> There are plenty of examples of the kernel community doing similar
> transitions. You can search around for examples, but a quick search of
> my own shows something like this:
> https://lwn.net/Articles/735887/
> (In particular, timer_setup() was introduced, and all setup_timer()
> users were migrated to it within a release or two.)
>
> Brian
>
> [1] Seriously, dev_coredumpv_noatomic() is not a name I want to see
> last very long. Maybe some other trivial modification? Examples:
>
> dev_core_dumpv()
> dev_coredump_v()
> device_coredumpv()
> ...
Thank you very much for your timer and suggestions!
Best regards,
Duoming Zhou