Re: [PATCH] mm: cma: print allocation failure reason and bitmap status

[Jaewon Kim <jaewon31.kim@samsung.com>]

On 2017e?? 01i?? 02i? 1/4  06:59, Michal Nazarewicz wrote:
> On Fri, Dec 30 2016, Michal Hocko wrote:
>> On Fri 30-12-16 16:24:46, Jaewon Kim wrote:
>> [...]
>>> >From 7577cc94da3af27907aa6eec590d2ef51e4b9d80 Mon Sep 17 00:00:00 2001
>>> From: Jaewon Kim <jaewon31.kim@samsung.com>
>>> Date: Thu, 29 Dec 2016 11:00:16 +0900
>>> Subject: [PATCH] mm: cma: print allocation failure reason and bitmap status
>>>
>>> There are many reasons of CMA allocation failure such as EBUSY, ENOMEM, EINTR.
>>> But we did not know error reason so far. This patch prints the error value.
>>>
>>> Additionally if CONFIG_CMA_DEBUG is enabled, this patch shows bitmap status to
>>> know available pages. Actually CMA internally try all available regions because
>>> some regions can be failed because of EBUSY. Bitmap status is useful to know in
>>> detail on both ENONEM and EBUSY;
>>>  ENOMEM: not tried at all because of no available region
>>>          it could be too small total region or could be fragmentation issue
>>>  EBUSY:  tried some region but all failed
>>>
>>> This is an ENOMEM example with this patch.
>>> [   13.250961]  [1:   Binder:715_1:  846] cma: cma_alloc: alloc failed, req-size: 256 pages, ret: -12
>>> Avabile pages also will be shown if CONFIG_CMA_DEBUG is enabled
>>> [   13.251052]  [1:   Binder:715_1:  846] cma: number of available pages: 4@572+7@585+7@601+8@632+38@730+166@1114+127@1921=>357 pages, total: 2048 pages
>> please mention how to interpret this information.
Thank you Michal Hocko. I added like this
If CONFIG_CMA_DEBUG is enabled, avabile pages also will be shown as concatenated
size@position format. So 4@572 means that there are 4 available pages at 572
position starting from 0 position.
>>
>> some more style suggestions below
>>> Signed-off-by: Jaewon Kim <jaewon31.kim@samsung.com>
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
Thank you I added your Ack
Acked-by: Michal Nazarewicz <mina86@mina86.com>
>>> ---
>>>  mm/cma.c | 29 ++++++++++++++++++++++++++++-
>>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/cma.c b/mm/cma.c
>>> index c960459..1bcd9db 100644
>>> --- a/mm/cma.c
>>> +++ b/mm/cma.c
>>> @@ -369,7 +369,7 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align)
>>>      unsigned long start = 0;
>>>      unsigned long bitmap_maxno, bitmap_no, bitmap_count;
>>>      struct page *page = NULL;
>>> -    int ret;
>>> +    int ret = -ENOMEM;
>>>  
>>>      if (!cma || !cma->count)
>>>          return NULL;
>>> @@ -427,6 +427,33 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align)
>>>      trace_cma_alloc(pfn, page, count, align);
>>>  
>>>      pr_debug("%s(): returned %p\n", __func__, page);
> This line should be moved after the a??if (ret != 0)a?? block, i.e. just
> before return.
Thank you Michal Nazarewicz
I moved the pr_debug right before return
     pr_debug("%s(): returned %p\n", __func__, page);
     return page;
>>> +
>>> +    if (ret != 0)
>> you can simply do
>> 	if (!ret) {
Thank you
I changed like this, it should be if(ret) rather than if(!ret)
+    if (ret) {
+        pr_info("%s: alloc failed, req-size: %zu pages, ret: %d\n",
+            __func__, count, ret);
+        debug_show_cma_areas(cma);
+    }
>>
>> 		pr_info("%s: alloc failed, req-size: %zu pages, ret: %d\n",
>> 			__func__, count, ret);
>> 		debug_show_cma_areas();
>> 	}
>>
>> 	return page;
>>
>> static void debug_show_cma_areas(void)
>> {
>> #ifdef CONFIG_CMA_DEBUG
>> 	unsigned int nr, nr_total = 0;
>> 	unsigned long next_set_bit;
>>
>> 	mutex_lock(&cma->lock);
>> 	pr_info("number of available pages: ");
>> 	start = 0;
>> 	for (;;) {
>> 		bitmap_no = find_next_zero_bit(cma->bitmap, cma->count, start);
>> 		if (bitmap_no >= cma->count)
>> 		break;
>> 		next_set_bit = find_next_bit(cma->bitmap, cma->count, bitmap_no);
>> 		nr = next_set_bit - bitmap_no;
>> 		pr_cont("%s%u@%lu", nr_total ? "+" : "", nr, bitmap_no);
>> 		nr_total += nr;
>> 		start = bitmap_no + nr;
>> 	}
>> 	pr_cont("=>%u pages, total: %lu pages\n", nr_total, cma->count);
> Perhaps:
> 	pr_cont("=> %u free of %lu total pages\n", nr_total, cma->count);
Thank you I will take this way.
+    pr_cont("=> %u free of %lu total pages\n", nr_total, cma->count);
> or shorter (but more cryptic):
> 	pr_cont("=> %u/%lu pages\n", nr_total, cma->count);
>
>> 	mutex_unlock(&cma->lock);
>> #endif
>> }
> Actually, Linux style is more like:
>
> #ifdef CONFIG_CMA_DEBUG
> static void cma_debug_show_areas()
> {
> 	a?|
> }
> #else
> static inline void cma_debug_show_areas() { }
> #endif
Thank you I will take this way. FYI struct cma address should be passed as a argument.
+#ifdef CONFIG_CMA_DEBUG
+static void debug_show_cma_areas(struct cma *cma)
+{
...
+#else
+static inline void debug_show_cma_areas(struct cma *cma) { }
+#endif
>> -- 
>> Michal Hocko
>> SUSE Labs

Thank you all of you.
Let me reattach my full patch to be clear.
Let me know if I resend this as a new mail thread.