| Message ID | 20200818063005.13828-1-zbestahu@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Daniel Lezcano |
| Headers | show |
| Series | thermal: sysfs: fall back to vzalloc for cooling device's statistics | expand |
On Tue, Aug 18, 2020 at 12:00 PM Yue Hu <zbestahu@gmail.com> wrote: > > From: Yue Hu <huyue2@yulong.com> > > We observed warning about kzalloc() when register thermal cooling device > in backlight_device_register(). backlight display can be a cooling device > since reducing screen brightness will can help reduce temperature. > > However, ->get_max_state of backlight will assign max brightness of 1024 > to states. The memory size can be getting 1MB+ due to states * states. > That is so large to trigger kmalloc() warning. > > So, let's remove it and try vzalloc() if kzalloc() fails. If we can do with vzalloc()'ed memory i.e. we don't need contiguous physical memory, why even attempt kzalloc? > > Signed-off-by: Yue Hu <huyue2@yulong.com> > --- > drivers/thermal/thermal_sysfs.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c > index aa99edb..9bae0b6 100644 > --- a/drivers/thermal/thermal_sysfs.c > +++ b/drivers/thermal/thermal_sysfs.c > @@ -16,6 +16,8 @@ > #include <linux/device.h> > #include <linux/err.h> > #include <linux/slab.h> > +#include <linux/vmalloc.h> > +#include <linux/mm.h> > #include <linux/string.h> > #include <linux/jiffies.h> > > @@ -919,7 +921,9 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) > var += sizeof(*stats->time_in_state) * states; > var += sizeof(*stats->trans_table) * states * states; > > - stats = kzalloc(var, GFP_KERNEL); > + stats = kzalloc(var, GFP_KERNEL | __GFP_NOWARN); > + if (!stats) > + stats = vzalloc(var); > if (!stats) > return; > > @@ -938,7 +942,7 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) > > static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev) > { > - kfree(cdev->stats); > + kvfree(cdev->stats); > cdev->stats = NULL; > } > > -- > 1.9.1 >
On Tue, Aug 18, 2020 at 12:00 PM Yue Hu <zbestahu@gmail.com> wrote: > > From: Yue Hu <huyue2@yulong.com> > > We observed warning about kzalloc() when register thermal cooling device > in backlight_device_register(). backlight display can be a cooling device > since reducing screen brightness will can help reduce temperature. > > However, ->get_max_state of backlight will assign max brightness of 1024 > to states. The memory size can be getting 1MB+ due to states * states. > That is so large to trigger kmalloc() warning. > > So, let's remove it and try vzalloc() if kzalloc() fails. > > Signed-off-by: Yue Hu <huyue2@yulong.com> > --- > drivers/thermal/thermal_sysfs.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c > index aa99edb..9bae0b6 100644 > --- a/drivers/thermal/thermal_sysfs.c > +++ b/drivers/thermal/thermal_sysfs.c > @@ -16,6 +16,8 @@ > #include <linux/device.h> > #include <linux/err.h> > #include <linux/slab.h> > +#include <linux/vmalloc.h> > +#include <linux/mm.h> > #include <linux/string.h> > #include <linux/jiffies.h> > > @@ -919,7 +921,9 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) > var += sizeof(*stats->time_in_state) * states; > var += sizeof(*stats->trans_table) * states * states; > > - stats = kzalloc(var, GFP_KERNEL); > + stats = kzalloc(var, GFP_KERNEL | __GFP_NOWARN); > + if (!stats) > + stats = vzalloc(var); Couldn't this be replaced by kvzalloc()? > if (!stats) > return; > > @@ -938,7 +942,7 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) > > static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev) > { > - kfree(cdev->stats); > + kvfree(cdev->stats); > cdev->stats = NULL; > } > > -- > 1.9.1 >
On Wed, 19 Aug 2020 16:47:01 +0530 Amit Kucheria <amitk@kernel.org> wrote: > On Tue, Aug 18, 2020 at 12:00 PM Yue Hu <zbestahu@gmail.com> wrote: > > > > From: Yue Hu <huyue2@yulong.com> > > > > We observed warning about kzalloc() when register thermal cooling device > > in backlight_device_register(). backlight display can be a cooling device > > since reducing screen brightness will can help reduce temperature. > > > > However, ->get_max_state of backlight will assign max brightness of 1024 > > to states. The memory size can be getting 1MB+ due to states * states. > > That is so large to trigger kmalloc() warning. > > > > So, let's remove it and try vzalloc() if kzalloc() fails. > > > > Signed-off-by: Yue Hu <huyue2@yulong.com> > > --- > > drivers/thermal/thermal_sysfs.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c > > index aa99edb..9bae0b6 100644 > > --- a/drivers/thermal/thermal_sysfs.c > > +++ b/drivers/thermal/thermal_sysfs.c > > @@ -16,6 +16,8 @@ > > #include <linux/device.h> > > #include <linux/err.h> > > #include <linux/slab.h> > > +#include <linux/vmalloc.h> > > +#include <linux/mm.h> > > #include <linux/string.h> > > #include <linux/jiffies.h> > > > > @@ -919,7 +921,9 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) > > var += sizeof(*stats->time_in_state) * states; > > var += sizeof(*stats->trans_table) * states * states; > > > > - stats = kzalloc(var, GFP_KERNEL); > > + stats = kzalloc(var, GFP_KERNEL | __GFP_NOWARN); > > + if (!stats) > > + stats = vzalloc(var); > > Couldn't this be replaced by kvzalloc()? Yes, it should be more better as kvzalloc has a vmalloc fallback. Thx. > > > if (!stats) > > return; > > > > @@ -938,7 +942,7 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) > > > > static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev) > > { > > - kfree(cdev->stats); > > + kvfree(cdev->stats); > > cdev->stats = NULL; > > } > > > > -- > > 1.9.1 > >
On 18/08/2020 08:30, Yue Hu wrote: > From: Yue Hu <huyue2@yulong.com> > > We observed warning about kzalloc() when register thermal cooling device > in backlight_device_register(). backlight display can be a cooling device > since reducing screen brightness will can help reduce temperature. > > However, ->get_max_state of backlight will assign max brightness of 1024 > to states. The memory size can be getting 1MB+ due to states * states. What are the benefits of a 1024 states cooling device ? Is the difference noticeable with a such small step ? > That is so large to trigger kmalloc() warning. > > So, let's remove it and try vzalloc() if kzalloc() fails. > > Signed-off-by: Yue Hu <huyue2@yulong.com> > --- > drivers/thermal/thermal_sysfs.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c > index aa99edb..9bae0b6 100644 > --- a/drivers/thermal/thermal_sysfs.c > +++ b/drivers/thermal/thermal_sysfs.c > @@ -16,6 +16,8 @@ > #include <linux/device.h> > #include <linux/err.h> > #include <linux/slab.h> > +#include <linux/vmalloc.h> > +#include <linux/mm.h> > #include <linux/string.h> > #include <linux/jiffies.h> > > @@ -919,7 +921,9 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) > var += sizeof(*stats->time_in_state) * states; > var += sizeof(*stats->trans_table) * states * states; > > - stats = kzalloc(var, GFP_KERNEL); > + stats = kzalloc(var, GFP_KERNEL | __GFP_NOWARN); > + if (!stats) > + stats = vzalloc(var); > if (!stats) > return; > > @@ -938,7 +942,7 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) > > static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev) > { > - kfree(cdev->stats); > + kvfree(cdev->stats); > cdev->stats = NULL; > } > >
On Mon, 24 Aug 2020 12:40:35 +0200 Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 18/08/2020 08:30, Yue Hu wrote: > > From: Yue Hu <huyue2@yulong.com> > > > > We observed warning about kzalloc() when register thermal cooling device > > in backlight_device_register(). backlight display can be a cooling device > > since reducing screen brightness will can help reduce temperature. > > > > However, ->get_max_state of backlight will assign max brightness of 1024 > > to states. The memory size can be getting 1MB+ due to states * states. > > What are the benefits of a 1024 states cooling device ? Is the > difference noticeable with a such small step ? Okay, this issue is happened under MSM/Android platform. QCOM spmi wled driver will define the max brightness. We needs to fix the issue to get thermal statistics. Thx. > > > > That is so large to trigger kmalloc() warning. > > > > So, let's remove it and try vzalloc() if kzalloc() fails. > > > > Signed-off-by: Yue Hu <huyue2@yulong.com> > > --- > > drivers/thermal/thermal_sysfs.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c > > index aa99edb..9bae0b6 100644 > > --- a/drivers/thermal/thermal_sysfs.c > > +++ b/drivers/thermal/thermal_sysfs.c > > @@ -16,6 +16,8 @@ > > #include <linux/device.h> > > #include <linux/err.h> > > #include <linux/slab.h> > > +#include <linux/vmalloc.h> > > +#include <linux/mm.h> > > #include <linux/string.h> > > #include <linux/jiffies.h> > > > > @@ -919,7 +921,9 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) > > var += sizeof(*stats->time_in_state) * states; > > var += sizeof(*stats->trans_table) * states * states; > > > > - stats = kzalloc(var, GFP_KERNEL); > > + stats = kzalloc(var, GFP_KERNEL | __GFP_NOWARN); > > + if (!stats) > > + stats = vzalloc(var); > > if (!stats) > > return; > > > > @@ -938,7 +942,7 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) > > > > static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev) > > { > > - kfree(cdev->stats); > > + kvfree(cdev->stats); > > cdev->stats = NULL; > > } > > > > > >
Hi Yue, On 26/08/2020 04:13, Yue Hu wrote: > On Mon, 24 Aug 2020 12:40:35 +0200 > Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > >> On 18/08/2020 08:30, Yue Hu wrote: >>> From: Yue Hu <huyue2@yulong.com> >>> >>> We observed warning about kzalloc() when register thermal cooling device >>> in backlight_device_register(). backlight display can be a cooling device >>> since reducing screen brightness will can help reduce temperature. >>> >>> However, ->get_max_state of backlight will assign max brightness of 1024 >>> to states. The memory size can be getting 1MB+ due to states * states. >> >> What are the benefits of a 1024 states cooling device ? Is the >> difference noticeable with a such small step ? > > Okay, this issue is happened under MSM/Android platform. QCOM spmi wled driver > will define the max brightness. We needs to fix the issue to get thermal statistics. Let me rephrase my questions: Don't you think there is something wrong in creating a 1024 x 1024 matrix to show transitions ? What is the benefit of such stats ? What is the benefit of having a 1024 states cooling device ?
On Wed, 26 Aug 2020 11:19:02 +0200 Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > Hi Yue, > > On 26/08/2020 04:13, Yue Hu wrote: > > On Mon, 24 Aug 2020 12:40:35 +0200 > > Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > > >> On 18/08/2020 08:30, Yue Hu wrote: > >>> From: Yue Hu <huyue2@yulong.com> > >>> > >>> We observed warning about kzalloc() when register thermal cooling device > >>> in backlight_device_register(). backlight display can be a cooling device > >>> since reducing screen brightness will can help reduce temperature. > >>> > >>> However, ->get_max_state of backlight will assign max brightness of 1024 > >>> to states. The memory size can be getting 1MB+ due to states * states. > >> > >> What are the benefits of a 1024 states cooling device ? Is the > >> difference noticeable with a such small step ? > > > > Okay, this issue is happened under MSM/Android platform. QCOM spmi wled driver > > will define the max brightness. We needs to fix the issue to get thermal statistics. > > Let me rephrase my questions: > > Don't you think there is something wrong in creating a 1024 x 1024 > matrix to show transitions ? > > What is the benefit of such stats ? > > What is the benefit of having a 1024 states cooling device ? Hi Daniel, Now, i'm just focus on removing the kernel warning based on current code logic. Commit 8ea229511e06 (thermal: Add cooling device's statistics in sysfs) added the thermal statistics by viresh and viresh gived the patch an acknowledgement in anther mail thread. Hi viresh, Could you review the patch again about the question above? Thank you. > > > >
On 27-08-20, 12:03, Yue Hu wrote: > Hi Daniel, > > Now, i'm just focus on removing the kernel warning based on current code logic. > Commit 8ea229511e06 (thermal: Add cooling device's statistics in sysfs) added > the thermal statistics by viresh and viresh gived the patch an acknowledgement > in anther mail thread. > > Hi viresh, > > Could you review the patch again about the question above? Yeah, I Acked it but the questions raised by Daniel are very valid and must be answered. I understand that you only cared about fixing the warning, but maybe we need to fix the driver and the warning will go away by itself. If you don't want to do it, then someone who is responsible for the driver should do it. Was it the acpi_video.c driver that you got the warning from ? I have added Rafael to the email in case that driver needs getting fixed.
On Thu, 27 Aug 2020 10:44:01 +0530 Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 27-08-20, 12:03, Yue Hu wrote: > > Hi Daniel, > > > > Now, i'm just focus on removing the kernel warning based on current code logic. > > Commit 8ea229511e06 (thermal: Add cooling device's statistics in sysfs) added > > the thermal statistics by viresh and viresh gived the patch an acknowledgement > > in anther mail thread. > > > > Hi viresh, > > > > Could you review the patch again about the question above? > > Yeah, I Acked it but the questions raised by Daniel are very valid and must be > answered. Yes, sure. > > I understand that you only cared about fixing the warning, but maybe we need to > fix the driver and the warning will go away by itself. If you don't want to do > it, then someone who is responsible for the driver should do it. Yes, maybe the patch is not totally correct. maybe the driver has issue. Let's check the driver firstly. > > Was it the acpi_video.c driver that you got the warning from ? I have added > Rafael to the email in case that driver needs getting fixed. > Currenly, drivers/video/backlight does not call thermal_of_cooling_device_register() to register thermal cooling device. The issue happened in msm-4.19 kernel for QCOM/Android platform. Backlight in msm-4.19 kernel will register thermal cooling device as below: +static int bd_cdev_get_max_brightness(struct thermal_cooling_device *cdev, + unsigned long *state) +{ + struct backlight_device *bd = (struct backlight_device *)cdev->devdata; + + *state = bd->props.max_brightness; + + return 0; +} +static struct thermal_cooling_device_ops bd_cdev_ops = { + .get_max_state = bd_cdev_get_max_brightness, +static void backlight_cdev_register(struct device *parent, + struct backlight_device *bd) +{ + if (of_find_property(parent->of_node, "#cooling-cells", NULL)) { + bd->cdev = thermal_of_cooling_device_register(parent->of_node, + (char *)dev_name(&bd->dev), bd, &bd_cdev_ops); And the bd->props.max_brightness is getting from video/backlight/qcom-wled.c. Maybe the driver should not assign 1024 to states/max_brightness. I'm not sure about it. So i consider to change memory allocation methord. That's the origin of the patch. Thank you.
On 27-08-20, 14:20, Yue Hu wrote: > Currenly, drivers/video/backlight does not call thermal_of_cooling_device_register() > to register thermal cooling device. The issue happened in msm-4.19 kernel for > QCOM/Android platform. Backlight in msm-4.19 kernel will register thermal cooling > device as below: > > +static int bd_cdev_get_max_brightness(struct thermal_cooling_device *cdev, > + unsigned long *state) > +{ > + struct backlight_device *bd = (struct backlight_device *)cdev->devdata; > + > + *state = bd->props.max_brightness; > + > + return 0; > +} > > > +static struct thermal_cooling_device_ops bd_cdev_ops = { > + .get_max_state = bd_cdev_get_max_brightness, > > +static void backlight_cdev_register(struct device *parent, > + struct backlight_device *bd) > +{ > + if (of_find_property(parent->of_node, "#cooling-cells", NULL)) { > + bd->cdev = thermal_of_cooling_device_register(parent->of_node, > + (char *)dev_name(&bd->dev), bd, &bd_cdev_ops); > > And the bd->props.max_brightness is getting from video/backlight/qcom-wled.c. Maybe > the driver should not assign 1024 to states/max_brightness. I'm not sure about it. > So i consider to change memory allocation methord. That's the origin of the patch. Thanks for the details. So this is not about upstream tree, as a rule we aren't going to make changes here for any downstream tree. Now coming back to the downstream driver, I also don't see a point in returning bd->props.max_brightness as the max number of states there. Maybe have 10 states, each occupying bd->props.max_brightness/10 brightness and so you will end up with 10 states only. But yeah, whatever downstream decides on that.
On Thu, 27 Aug 2020 11:56:46 +0530 Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 27-08-20, 14:20, Yue Hu wrote: > > Currenly, drivers/video/backlight does not call thermal_of_cooling_device_register() > > to register thermal cooling device. The issue happened in msm-4.19 kernel for > > QCOM/Android platform. Backlight in msm-4.19 kernel will register thermal cooling > > device as below: > > > > +static int bd_cdev_get_max_brightness(struct thermal_cooling_device *cdev, > > + unsigned long *state) > > +{ > > + struct backlight_device *bd = (struct backlight_device *)cdev->devdata; > > + > > + *state = bd->props.max_brightness; > > + > > + return 0; > > +} > > > > > > +static struct thermal_cooling_device_ops bd_cdev_ops = { > > + .get_max_state = bd_cdev_get_max_brightness, > > > > +static void backlight_cdev_register(struct device *parent, > > + struct backlight_device *bd) > > +{ > > + if (of_find_property(parent->of_node, "#cooling-cells", NULL)) { > > + bd->cdev = thermal_of_cooling_device_register(parent->of_node, > > + (char *)dev_name(&bd->dev), bd, &bd_cdev_ops); > > > > And the bd->props.max_brightness is getting from video/backlight/qcom-wled.c. Maybe > > the driver should not assign 1024 to states/max_brightness. I'm not sure about it. > > So i consider to change memory allocation methord. That's the origin of the patch. > > Thanks for the details. So this is not about upstream tree, as a rule > we aren't going to make changes here for any downstream tree. I at first thought it's a possible issue in upstream tree. > > Now coming back to the downstream driver, I also don't see a point in > returning bd->props.max_brightness as the max number of states there. > Maybe have 10 states, each occupying bd->props.max_brightness/10 > brightness and so you will end up with 10 states only. But yeah, > whatever downstream decides on that. > Got it. Thx.
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c index aa99edb..9bae0b6 100644 --- a/drivers/thermal/thermal_sysfs.c +++ b/drivers/thermal/thermal_sysfs.c @@ -16,6 +16,8 @@ #include <linux/device.h> #include <linux/err.h> #include <linux/slab.h> +#include <linux/vmalloc.h> +#include <linux/mm.h> #include <linux/string.h> #include <linux/jiffies.h> @@ -919,7 +921,9 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) var += sizeof(*stats->time_in_state) * states; var += sizeof(*stats->trans_table) * states * states; - stats = kzalloc(var, GFP_KERNEL); + stats = kzalloc(var, GFP_KERNEL | __GFP_NOWARN); + if (!stats) + stats = vzalloc(var); if (!stats) return; @@ -938,7 +942,7 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev) static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev) { - kfree(cdev->stats); + kvfree(cdev->stats); cdev->stats = NULL; }