From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-la0-f42.google.com (mail-la0-f42.google.com [209.85.215.42]) by kanga.kvack.org (Postfix) with ESMTP id 111876B0036 for ; Tue, 20 May 2014 08:24:00 -0400 (EDT) Received: by mail-la0-f42.google.com with SMTP id el20so329138lab.15 for ; Tue, 20 May 2014 05:24:00 -0700 (PDT) Received: from mail-lb0-x236.google.com (mail-lb0-x236.google.com [2a00:1450:4010:c04::236]) by mx.google.com with ESMTPS id oc9si2830537lbb.57.2014.05.20.05.23.58 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 20 May 2014 05:23:59 -0700 (PDT) Received: by mail-lb0-f182.google.com with SMTP id z11so322691lbi.27 for ; Tue, 20 May 2014 05:23:58 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <537B3EA5.2040302@samsung.com> References: <1399509144-8898-1-git-send-email-iamjoonsoo.kim@lge.com> <1399509144-8898-3-git-send-email-iamjoonsoo.kim@lge.com> <20140513030057.GC32092@bbox> <20140515015301.GA10116@js1304-P5Q-DELUXE> <5375C619.8010501@lge.com> <537962A0.4090600@lge.com> <20140519055527.GA24099@js1304-P5Q-DELUXE> <537AA6C7.1040506@lge.com> <537B3EA5.2040302@samsung.com> Date: Tue, 20 May 2014 21:23:58 +0900 Message-ID: Subject: Re: [RFC][PATCH] CMA: drivers/base/Kconfig: restrict CMA size to non-zero value From: Gi-Oh Kim Content-Type: multipart/alternative; boundary=001a113367ac44bf8604f9d3f4a5 Sender: owner-linux-mm@kvack.org List-ID: To: Marek Szyprowski Cc: Gioh Kim , Michal Nazarewicz , Joonsoo Kim , Minchan Kim , Andrew Morton , Rik van Riel , Laura Abbott , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Heesub Shin , Mel Gorman , Johannes Weiner , =?UTF-8?B?7J206rG07Zi4?= --001a113367ac44bf8604f9d3f4a5 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 2014-05-20 20:38 GMT+09:00 Marek Szyprowski : > Hello, > > > On 2014-05-20 02:50, Gioh Kim wrote: > >> >> >> 2014-05-20 =EC=98=A4=EC=A0=84 4:59, Michal Nazarewicz =EC=93=B4 =EA=B8= =80: >> >>> On Sun, May 18 2014, Joonsoo Kim wrote: >>> >>>> I think that this problem is originated from atomic_pool_init(). >>>> If configured coherent_pool size is larger than default cma size, >>>> it can be failed even if this patch is applied. >>>> >>> >> The coherent_pool size (atomic_pool.size) should be restricted smaller >> than cma size. >> >> This is another issue, however I think the default atomic pool size is >> too small. >> Only one port of USB host needs at most 256Kbytes coherent memory >> (according to the USB host spec). >> > > This pool is used only for allocation done in atomic context (allocations > done with GFP_ATOMIC flag), otherwise the standard allocation path is use= d. > Are you sure that each usb host port really needs so much memory allocate= d > in atomic context? http://lxr.free-electrons.com/source/drivers/usb/host/ehci-mem.c#L210 dma_alloc_coherent is called with gfp as zero, no GFP_ATOMIC flag. If CMA is turned on and size is zero, ehci driver occurs panic. > > > If a platform has several ports, it needs more than 1MB. >> Therefore the default atomic pool size should be at least 1MB. >> >> >>>> How about below patch? >>>> It uses fallback allocation if CMA is failed. >>>> >>> >>> Yes, I thought about it, but __dma_alloc uses similar code: >>> >>> else if (!IS_ENABLED(CONFIG_DMA_CMA)) >>> addr =3D __alloc_remap_buffer(dev, size, gfp, prot, &page, call= er); >>> else >>> addr =3D __alloc_from_contiguous(dev, size, prot, &page, caller= ); >>> >>> so it probably needs to be changed as well. >>> >> >> If CMA option is not selected, __alloc_from_contiguous would not be >> called. >> We don't need to the fallback allocation. >> >> And if CMA option is selected and initialized correctly, >> the cma allocation can fail in case of no-CMA-memory situation. >> I thinks in that case we don't need to the fallback allocation also, >> because it is normal case. >> >> Therefore I think the restriction of CMA size option and make CMA work >> can cover every cases. >> >> I think below patch is also good choice. >> If both of you, Michal and Joonsoo, do not agree with me, please inform >> me. >> I will make a patch including option restriction and fallback allocation= . >> > > I'm not sure if we need a fallback for failed CMA allocation. The only > issue that > have been mentioned here and needs to be resolved is support for disablin= g > cma by > kernel command line. Right now it will fails completely. > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland > > --=20 ---- Love and Serve make me happy blog - http://gurugio.blogspot.com/ homepage - CalciumOS http://code.google.com/p/caoskernel/ --001a113367ac44bf8604f9d3f4a5 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable



2014-05-20 20:38 GMT+09:00 Marek Szyprowski <<= a href=3D"mailto:m.szyprowski@samsung.com" target=3D"_blank">m.szyprowski@s= amsung.com>:
Hello,


On 2014-05-20 02:50, Gioh Kim wrote:


2014-05-20 =EC=98=A4=EC=A0=84 4:59, Michal Nazarewicz =EC=93=B4 =EA=B8=80:<= br>
On Sun, May 18 2014, Joonsoo Kim wrote:
I think that this problem is originated from atomic_pool_init().
If configured coherent_pool size is larger than default cma size,
it can be failed even if this patch is applied.

The coherent_pool size (atomic_pool.size) should be restricted smaller than= cma size.

This is another issue, however I think the default atomic pool size is too = small.
Only one port of USB host needs at most 256Kbytes coherent memory (accordin= g to the USB host spec).

This pool is used only for allocation done in atomic context (allocations done with GFP_ATOMIC flag), otherwise the standard allocation path is used.=
Are you sure that each usb host port really needs so much memory allocated<= br> in atomic context?

dma_alloc_cohe= rent=C2=A0is called with gfp as zero, no GFP_ATOMIC flag.

If CMA is turned on and size is zero, ehci driver occur= s panic.
=C2=A0


If a platform has several ports, it needs more than 1MB.
Therefore the default atomic pool size should be at least 1MB.


How about below patch?
It uses fallback allocation if CMA is failed.

Yes, I thought about it, but __dma_alloc uses similar code:

=C2=A0 =C2=A0 else if (!IS_ENABLED(CONFIG_DMA_CMA))
=C2=A0 =C2=A0 =C2=A0 =C2=A0 addr =3D __alloc_remap_buffer(dev, size, gfp, p= rot, &page, caller);
=C2=A0 =C2=A0 else
=C2=A0 =C2=A0 =C2=A0 =C2=A0 addr =3D __alloc_from_contiguous(dev, size, pro= t, &page, caller);

so it probably needs to be changed as well.

If CMA option is not selected, __alloc_from_contiguous would not be called.=
We don't need to the fallback allocation.

And if CMA option is selected and initialized correctly,
the cma allocation can fail in case of no-CMA-memory situation.
I thinks in that case we don't need to the fallback allocation also, because it is normal case.

Therefore I think the restriction of CMA size option and make CMA work can = cover every cases.

I think below patch is also good choice.
If both of you, Michal and Joonsoo, do not agree with me, please inform me.=
I will make a patch including option restriction and fallback allocation.

I'm not sure if we need a fallback for failed CMA allocation. The only = issue that
have been mentioned here and needs to be resolved is support for disabling = cma by
kernel command line. Right now it will fails completely.
= =C2=A0

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland




-- ----
Love and Serve make me happy
blog - http://gurugio.blogspot.com/
homepage - CalciumOS http://code.google.com/p/caos= kernel/
--001a113367ac44bf8604f9d3f4a5-- -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org