| Message ID | 000d01cf328f$687d54b0$3977fe10$%han@samsung.com |
|---|---|
| State | Accepted |
| Headers | show |
On 26 February 2014 06:39, Jingoo Han <jg1.han@samsung.com> wrote: > The site-specific OOM messages are unnecessary, because they > duplicate the MM subsystem generic OOM message. > > Signed-off-by: Jingoo Han <jg1.han@samsung.com> > --- > drivers/pwm/pwm-spear.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c > index 8ad26b8..b4f6d0d 100644 > --- a/drivers/pwm/pwm-spear.c > +++ b/drivers/pwm/pwm-spear.c > @@ -179,10 +179,8 @@ static int spear_pwm_probe(struct platform_device *pdev) > u32 val; > > pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); > - if (!pc) { > - dev_err(&pdev->dev, "failed to allocate memory\n"); Can you please pin point which/file line will print similar message ? -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 26, 2014 1:49 PM, Viresh Kumar wrote: > On 26 February 2014 06:39, Jingoo Han <jg1.han@samsung.com> wrote: > > The site-specific OOM messages are unnecessary, because they > > duplicate the MM subsystem generic OOM message. > > > > Signed-off-by: Jingoo Han <jg1.han@samsung.com> > > --- > > drivers/pwm/pwm-spear.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c > > index 8ad26b8..b4f6d0d 100644 > > --- a/drivers/pwm/pwm-spear.c > > +++ b/drivers/pwm/pwm-spear.c > > @@ -179,10 +179,8 @@ static int spear_pwm_probe(struct platform_device *pdev) > > u32 val; > > > > pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); > > - if (!pc) { > > - dev_err(&pdev->dev, "failed to allocate memory\n"); > > Can you please pin point which/file line will print similar message ? (+cc Joe Perches, Andrew Morton) Sorry, I cannot pin point the exact file line. As far as I know, the similar message will be printed, because k.alloc and v.alloc failures use dump_stack(). Best regards, Jingoo Han -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26 February 2014 10:27, Jingoo Han <jg1.han@samsung.com> wrote: > On Wednesday, February 26, 2014 1:49 PM, Viresh Kumar wrote: >> On 26 February 2014 06:39, Jingoo Han <jg1.han@samsung.com> wrote: >> > The site-specific OOM messages are unnecessary, because they >> > duplicate the MM subsystem generic OOM message. >> > >> > Signed-off-by: Jingoo Han <jg1.han@samsung.com> >> > --- >> > drivers/pwm/pwm-spear.c | 4 +--- >> > 1 file changed, 1 insertion(+), 3 deletions(-) >> > >> > diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c >> > index 8ad26b8..b4f6d0d 100644 >> > --- a/drivers/pwm/pwm-spear.c >> > +++ b/drivers/pwm/pwm-spear.c >> > @@ -179,10 +179,8 @@ static int spear_pwm_probe(struct platform_device *pdev) >> > u32 val; >> > >> > pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); >> > - if (!pc) { >> > - dev_err(&pdev->dev, "failed to allocate memory\n"); >> >> Can you please pin point which/file line will print similar message ? > > (+cc Joe Perches, Andrew Morton) > > Sorry, I cannot pin point the exact file line. > As far as I know, the similar message will be printed, > because k.alloc and v.alloc failures use dump_stack(). I have tried tracing calls here and I am unable to find any such prints from these routines for majority of failure paths. So its better somebody tells me that I am wrong here :) -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-02-26 at 10:40 +0530, Viresh Kumar wrote: > On 26 February 2014 10:27, Jingoo Han <jg1.han@samsung.com> wrote: > > On Wednesday, February 26, 2014 1:49 PM, Viresh Kumar wrote: > >> On 26 February 2014 06:39, Jingoo Han <jg1.han@samsung.com> wrote: > >> > The site-specific OOM messages are unnecessary, because they > >> > duplicate the MM subsystem generic OOM message. > >> > > >> > Signed-off-by: Jingoo Han <jg1.han@samsung.com> > >> > --- > >> > drivers/pwm/pwm-spear.c | 4 +--- > >> > 1 file changed, 1 insertion(+), 3 deletions(-) > >> > > >> > diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c > >> > index 8ad26b8..b4f6d0d 100644 > >> > --- a/drivers/pwm/pwm-spear.c > >> > +++ b/drivers/pwm/pwm-spear.c > >> > @@ -179,10 +179,8 @@ static int spear_pwm_probe(struct platform_device *pdev) > >> > u32 val; > >> > > >> > pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); > >> > - if (!pc) { > >> > - dev_err(&pdev->dev, "failed to allocate memory\n"); > >> > >> Can you please pin point which/file line will print similar message ? > > > > (+cc Joe Perches, Andrew Morton) > > > > Sorry, I cannot pin point the exact file line. > > As far as I know, the similar message will be printed, > > because k.alloc and v.alloc failures use dump_stack(). > > I have tried tracing calls here and I am unable to find > any such prints from these routines for majority of > failure paths. So its better somebody tells me that I am > wrong here :) Look at warn_alloc_failed() in mm/page_alloc.c -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26 February 2014 10:49, Joe Perches <joe@perches.com> wrote:
> Look at warn_alloc_failed() in mm/page_alloc.c
Okay, there is a print there. But I am not able to reach to this routine
from devm_kzalloc().
devm_kzalloc() <linux/device.h>
devm_kmalloc() <drivers/base/devres.c>
alloc_dr() <drivers/base/devres.c>
kmalloc_track_caller() <linux/slab.h>
__kmalloc_track_caller() <mm/slab,slub/slob.c> Taking slab as example:
__do_kmalloc() <mm/slab.c>
...
I can see cases where NULL is returned after above paths and the function
you mentioned wasn't there. So, I am not sure that we will get a print for sure
for any error that might occur from devm_kzalloc().
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 26, 2014 3:26 PM, Viresh Kumar wrote: > On 26 February 2014 10:49, Joe Perches <joe@perches.com> wrote: > > Look at warn_alloc_failed() in mm/page_alloc.c > > Okay, there is a print there. But I am not able to reach to this routine > from devm_kzalloc(). > > devm_kzalloc() <linux/device.h> > devm_kmalloc() <drivers/base/devres.c> > alloc_dr() <drivers/base/devres.c> > kmalloc_track_caller() <linux/slab.h> > __kmalloc_track_caller() <mm/slab,slub/slob.c> Taking slab as example: > __do_kmalloc() <mm/slab.c> (+CC Laurent Pinchart, Dan Carpenter) Right, I also cannot find that warn_alloc_failed() is called, during devm_kzalloc(). However, in the case of vmalloc(), warn_alloc_failed() is called as below. ./mm/vmalloc.c vmalloc() __vmalloc_node_flags() __vmalloc_node() __vmalloc_node_range() ./mm/page_alloc.c warn_alloc_failed() > ... > > I can see cases where NULL is returned after above paths and the function > you mentioned wasn't there. So, I am not sure that we will get a print for sure > for any error that might occur from devm_kzalloc(). I guess that slab_out_of_memory() <./mm/slub.c> may print it for any errors. But, I am not sure. :-( Joe Perches, Would you confirm this? Best regards, Jingoo Han -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 03, 2014 at 10:14:22AM +0900, Jingoo Han wrote: > On Wednesday, February 26, 2014 3:26 PM, Viresh Kumar wrote: > > On 26 February 2014 10:49, Joe Perches <joe@perches.com> wrote: > > > Look at warn_alloc_failed() in mm/page_alloc.c > > > > Okay, there is a print there. But I am not able to reach to this routine > > from devm_kzalloc(). > > > > devm_kzalloc() <linux/device.h> > > devm_kmalloc() <drivers/base/devres.c> > > alloc_dr() <drivers/base/devres.c> > > kmalloc_track_caller() <linux/slab.h> > > __kmalloc_track_caller() <mm/slab,slub/slob.c> Taking slab as example: > > __do_kmalloc() <mm/slab.c> > > (+CC Laurent Pinchart, Dan Carpenter) > > Right, I also cannot find that warn_alloc_failed() is called, during > devm_kzalloc(). > > However, in the case of vmalloc(), warn_alloc_failed() is called > as below. > > ./mm/vmalloc.c > vmalloc() > __vmalloc_node_flags() > __vmalloc_node() > __vmalloc_node_range() > > ./mm/page_alloc.c > warn_alloc_failed() > > > ... > > > > I can see cases where NULL is returned after above paths and the function > > you mentioned wasn't there. So, I am not sure that we will get a print for sure > > for any error that might occur from devm_kzalloc(). > > I guess that slab_out_of_memory() <./mm/slub.c> may print it for any errors. > But, I am not sure. :-( > devm_kzalloc() is just kmalloc(). The OOM error messages are the same. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dan, On Monday 03 March 2014 12:53:11 Dan Carpenter wrote: > On Mon, Mar 03, 2014 at 10:14:22AM +0900, Jingoo Han wrote: > > On Wednesday, February 26, 2014 3:26 PM, Viresh Kumar wrote: > > > On 26 February 2014 10:49, Joe Perches <joe@perches.com> wrote: > > > > Look at warn_alloc_failed() in mm/page_alloc.c > > > > > > Okay, there is a print there. But I am not able to reach to this routine > > > from devm_kzalloc(). > > > > > > devm_kzalloc() <linux/device.h> > > > devm_kmalloc() <drivers/base/devres.c> > > > alloc_dr() <drivers/base/devres.c> > > > kmalloc_track_caller() <linux/slab.h> > > > __kmalloc_track_caller() <mm/slab,slub/slob.c> Taking slab as example: > > > __do_kmalloc() <mm/slab.c> > > > > (+CC Laurent Pinchart, Dan Carpenter) > > > > Right, I also cannot find that warn_alloc_failed() is called, during > > devm_kzalloc(). > > > > However, in the case of vmalloc(), warn_alloc_failed() is called > > as below. > > > > ./mm/vmalloc.c > > vmalloc() > > __vmalloc_node_flags() > > __vmalloc_node() > > __vmalloc_node_range() > > > > ./mm/page_alloc.c > > warn_alloc_failed() > > > > > ... > > > > > > I can see cases where NULL is returned after above paths and the > > > function you mentioned wasn't there. So, I am not sure that we will get > > > a print for sure for any error that might occur from devm_kzalloc(). > > > > I guess that slab_out_of_memory() <./mm/slub.c> may print it for any > > errors. But, I am not sure. :-( > > devm_kzalloc() is just kmalloc(). The OOM error messages are the same. Sure, but I wasn't sure whether all error code paths in kmalloc() resulted in an OOM message. For instance, the following code path results in an allocation failure but doesn't seem to print an OOM message: kmalloc __kmalloc __do_kmalloc slab_alloc slab_should_failslab should_failslab should_fail A bit far-fetched possibly as it requires fault injection. I haven't found any other such code path, but my understanding of that code is a bit limited.
On Wed, Mar 05, 2014 at 12:00:42PM +0100, Laurent Pinchart wrote: > Hi Dan, > > On Monday 03 March 2014 12:53:11 Dan Carpenter wrote: > > On Mon, Mar 03, 2014 at 10:14:22AM +0900, Jingoo Han wrote: > > > On Wednesday, February 26, 2014 3:26 PM, Viresh Kumar wrote: > > > > On 26 February 2014 10:49, Joe Perches <joe@perches.com> wrote: > > > > > Look at warn_alloc_failed() in mm/page_alloc.c > > > > > > > > Okay, there is a print there. But I am not able to reach to this routine > > > > from devm_kzalloc(). > > > > > > > > devm_kzalloc() <linux/device.h> > > > > devm_kmalloc() <drivers/base/devres.c> > > > > alloc_dr() <drivers/base/devres.c> > > > > kmalloc_track_caller() <linux/slab.h> > > > > __kmalloc_track_caller() <mm/slab,slub/slob.c> Taking slab as example: > > > > __do_kmalloc() <mm/slab.c> > > > > > > (+CC Laurent Pinchart, Dan Carpenter) > > > > > > Right, I also cannot find that warn_alloc_failed() is called, during > > > devm_kzalloc(). > > > > > > However, in the case of vmalloc(), warn_alloc_failed() is called > > > as below. > > > > > > ./mm/vmalloc.c > > > vmalloc() > > > __vmalloc_node_flags() > > > __vmalloc_node() > > > __vmalloc_node_range() > > > > > > ./mm/page_alloc.c > > > warn_alloc_failed() > > > > > > > ... > > > > > > > > I can see cases where NULL is returned after above paths and the > > > > function you mentioned wasn't there. So, I am not sure that we will get > > > > a print for sure for any error that might occur from devm_kzalloc(). > > > > > > I guess that slab_out_of_memory() <./mm/slub.c> may print it for any > > > errors. But, I am not sure. :-( > > > > devm_kzalloc() is just kmalloc(). The OOM error messages are the same. > > Sure, but I wasn't sure whether all error code paths in kmalloc() resulted in > an OOM message. For instance, the following code path results in an allocation > failure but doesn't seem to print an OOM message: > > kmalloc > __kmalloc > __do_kmalloc > slab_alloc > slab_should_failslab > should_failslab > should_fail > > A bit far-fetched possibly as it requires fault injection. I haven't found any > other such code path, but my understanding of that code is a bit limited. should_fail() only returns true after calling fail_dump(). You can pass (GFP_KERNEL | __GFP_NOWARN) if you want to turn off warnings. Otherwise it shuold print a warning. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5 March 2014 19:00, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Sure, but I wasn't sure whether all error code paths in kmalloc() resulted in > an OOM message. For instance, the following code path results in an allocation > failure but doesn't seem to print an OOM message: > > kmalloc > __kmalloc > __do_kmalloc > slab_alloc > slab_should_failslab > should_failslab > should_fail > > A bit far-fetched possibly as it requires fault injection. I haven't found any > other such code path, but my understanding of that code is a bit limited. In that case should we actually accept patches like this at all? As they might be ending up removing some useful print messages? -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Viresh, On Thursday 06 March 2014 00:17:38 Viresh Kumar wrote: > On 5 March 2014 19:00, Laurent Pinchart wrote: > > Sure, but I wasn't sure whether all error code paths in kmalloc() resulted > > in an OOM message. For instance, the following code path results in an > > allocation failure but doesn't seem to print an OOM message: > > > > kmalloc > > __kmalloc > > __do_kmalloc > > slab_alloc > > slab_should_failslab > > should_failslab > > should_fail > > > > A bit far-fetched possibly as it requires fault injection. I haven't found > > any other such code path, but my understanding of that code is a bit > > limited. > > In that case should we actually accept patches like this at all? As they > might be ending up removing some useful print messages? Dan has pointed out that I've missed the fail_dump() call in should_fail(). One could argue that fail_dump() wouldn't print any message if the fault injection framework has verbosity set to 0, but I suppose we can assume that people using the fault injection framework know what they're doing. All other error paths in kmalloc() seem to result in a message being printed. I might have missed something, but I can trust the developers who know that code much better than I do that kmalloc() is designed to print an error message in all error paths. Any failure to print a message would be a kmalloc() bug that should be fixed, and getting rid of the allocation error messages in drivers would seem like a nice cleanup to me.
On Thursday, March 06, 2014 1:30 AM, Laurent Pinchart wrote: > On Thursday 06 March 2014 00:17:38 Viresh Kumar wrote: > > On 5 March 2014 19:00, Laurent Pinchart wrote: > > > Sure, but I wasn't sure whether all error code paths in kmalloc() resulted > > > in an OOM message. For instance, the following code path results in an > > > allocation failure but doesn't seem to print an OOM message: > > > > > > kmalloc > > > __kmalloc > > > __do_kmalloc > > > slab_alloc > > > slab_should_failslab > > > should_failslab > > > should_fail > > > > > > A bit far-fetched possibly as it requires fault injection. I haven't found > > > any other such code path, but my understanding of that code is a bit > > > limited. > > > > In that case should we actually accept patches like this at all? As they > > might be ending up removing some useful print messages? > > Dan has pointed out that I've missed the fail_dump() call in should_fail(). > One could argue that fail_dump() wouldn't print any message if the fault > injection framework has verbosity set to 0, but I suppose we can assume that > people using the fault injection framework know what they're doing. > > All other error paths in kmalloc() seem to result in a message being printed. > I might have missed something, but I can trust the developers who know that > code much better than I do that kmalloc() is designed to print an error > message in all error paths. Any failure to print a message would be a > kmalloc() bug that should be fixed, and getting rid of the allocation error > messages in drivers would seem like a nice cleanup to me. Hi Thierry Reding, There seems to be no objection. :-) Would you accept these patches? Thank you. Best regards, Jingoo Han -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 07, 2014 at 05:04:36PM +0900, Jingoo Han wrote: > On Thursday, March 06, 2014 1:30 AM, Laurent Pinchart wrote: > > On Thursday 06 March 2014 00:17:38 Viresh Kumar wrote: > > > On 5 March 2014 19:00, Laurent Pinchart wrote: > > > > Sure, but I wasn't sure whether all error code paths in kmalloc() resulted > > > > in an OOM message. For instance, the following code path results in an > > > > allocation failure but doesn't seem to print an OOM message: > > > > > > > > kmalloc > > > > __kmalloc > > > > __do_kmalloc > > > > slab_alloc > > > > slab_should_failslab > > > > should_failslab > > > > should_fail > > > > > > > > A bit far-fetched possibly as it requires fault injection. I haven't found > > > > any other such code path, but my understanding of that code is a bit > > > > limited. > > > > > > In that case should we actually accept patches like this at all? As they > > > might be ending up removing some useful print messages? > > > > Dan has pointed out that I've missed the fail_dump() call in should_fail(). > > One could argue that fail_dump() wouldn't print any message if the fault > > injection framework has verbosity set to 0, but I suppose we can assume that > > people using the fault injection framework know what they're doing. > > > > All other error paths in kmalloc() seem to result in a message being printed. > > I might have missed something, but I can trust the developers who know that > > code much better than I do that kmalloc() is designed to print an error > > message in all error paths. Any failure to print a message would be a > > kmalloc() bug that should be fixed, and getting rid of the allocation error > > messages in drivers would seem like a nice cleanup to me. > > Hi Thierry Reding, > > There seems to be no objection. :-) > Would you accept these patches? > Thank you. All 9 patches applied, thanks. Thierry
On Monday, March 10, 2014 9:23 PM, Thierry Reding wrote: > On Fri, Mar 07, 2014 at 05:04:36PM +0900, Jingoo Han wrote: [....] > > > > Hi Thierry Reding, > > > > There seems to be no objection. :-) > > Would you accept these patches? > > Thank you. > > All 9 patches applied, thanks. Hi Thierry, I cannot find these patches from your pwm git tree. [1] Would you confirm it? :-) Thank you. [1] http://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-next Best regards, Jingoo Han > > Thierry -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 03, 2014 at 12:24:54PM +0900, Jingoo Han wrote: > On Monday, March 10, 2014 9:23 PM, Thierry Reding wrote: > > On Fri, Mar 07, 2014 at 05:04:36PM +0900, Jingoo Han wrote: > > [....] > > > > > > > Hi Thierry Reding, > > > > > > There seems to be no objection. :-) > > > Would you accept these patches? > > > Thank you. > > > > All 9 patches applied, thanks. > > Hi Thierry, > > I cannot find these patches from your pwm git tree. [1] > Would you confirm it? :-) Thank you. > > [1] http://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-next It seems like I never replied to this, but the patches have now been merged for a while. Thierry
diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c index 8ad26b8..b4f6d0d 100644 --- a/drivers/pwm/pwm-spear.c +++ b/drivers/pwm/pwm-spear.c @@ -179,10 +179,8 @@ static int spear_pwm_probe(struct platform_device *pdev) u32 val; pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); - if (!pc) { - dev_err(&pdev->dev, "failed to allocate memory\n"); + if (!pc) return -ENOMEM; - } r = platform_get_resource(pdev, IORESOURCE_MEM, 0); pc->mmio_base = devm_ioremap_resource(&pdev->dev, r);
The site-specific OOM messages are unnecessary, because they duplicate the MM subsystem generic OOM message. Signed-off-by: Jingoo Han <jg1.han@samsung.com> --- drivers/pwm/pwm-spear.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)