* Re: [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc()
2018-07-09 12:19 ` [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc() Marek Szyprowski
@ 2018-07-09 13:09 ` Michal Hocko
2018-07-09 17:27 ` Laura Abbott
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Michal Hocko @ 2018-07-09 13:09 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-mm, linux-kernel, linux-arm-kernel, linuxppc-dev, iommu,
Andrew Morton, Michal Nazarewicz, Joonsoo Kim, Vlastimil Babka,
Christoph Hellwig, Russell King, Catalin Marinas, Will Deacon,
Paul Mackerras, Benjamin Herrenschmidt, Chris Zankel,
Martin Schwidefsky, Joerg Roedel, Sumit Semwal, Robin Murphy,
Laura Abbott, linaro-mm-sig
On Mon 09-07-18 14:19:55, Marek Szyprowski wrote:
> cma_alloc() function doesn't really support gfp flags other than
> __GFP_NOWARN, so convert gfp_mask parameter to boolean no_warn parameter.
>
> This will help to avoid giving false feeling that this function supports
> standard gfp flags and callers can pass __GFP_ZERO to get zeroed buffer,
> what has already been an issue: see commit dd65a941f6ba ("arm64:
> dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag").
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Thanks! This makes perfect sense to me. If there is a real need for the
gfp_mask then we should start by defining the semantic first.
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> arch/powerpc/kvm/book3s_hv_builtin.c | 2 +-
> drivers/s390/char/vmcp.c | 2 +-
> drivers/staging/android/ion/ion_cma_heap.c | 2 +-
> include/linux/cma.h | 2 +-
> kernel/dma/contiguous.c | 3 ++-
> mm/cma.c | 8 ++++----
> mm/cma_debug.c | 2 +-
> 7 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_builtin.c b/arch/powerpc/kvm/book3s_hv_builtin.c
> index d4a3f4da409b..fc6bb9630a9c 100644
> --- a/arch/powerpc/kvm/book3s_hv_builtin.c
> +++ b/arch/powerpc/kvm/book3s_hv_builtin.c
> @@ -77,7 +77,7 @@ struct page *kvm_alloc_hpt_cma(unsigned long nr_pages)
> VM_BUG_ON(order_base_2(nr_pages) < KVM_CMA_CHUNK_ORDER - PAGE_SHIFT);
>
> return cma_alloc(kvm_cma, nr_pages, order_base_2(HPT_ALIGN_PAGES),
> - GFP_KERNEL);
> + false);
> }
> EXPORT_SYMBOL_GPL(kvm_alloc_hpt_cma);
>
> diff --git a/drivers/s390/char/vmcp.c b/drivers/s390/char/vmcp.c
> index 948ce82a7725..0fa1b6b1491a 100644
> --- a/drivers/s390/char/vmcp.c
> +++ b/drivers/s390/char/vmcp.c
> @@ -68,7 +68,7 @@ static void vmcp_response_alloc(struct vmcp_session *session)
> * anymore the system won't work anyway.
> */
> if (order > 2)
> - page = cma_alloc(vmcp_cma, nr_pages, 0, GFP_KERNEL);
> + page = cma_alloc(vmcp_cma, nr_pages, 0, false);
> if (page) {
> session->response = (char *)page_to_phys(page);
> session->cma_alloc = 1;
> diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
> index 49718c96bf9e..3fafd013d80a 100644
> --- a/drivers/staging/android/ion/ion_cma_heap.c
> +++ b/drivers/staging/android/ion/ion_cma_heap.c
> @@ -39,7 +39,7 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
> if (align > CONFIG_CMA_ALIGNMENT)
> align = CONFIG_CMA_ALIGNMENT;
>
> - pages = cma_alloc(cma_heap->cma, nr_pages, align, GFP_KERNEL);
> + pages = cma_alloc(cma_heap->cma, nr_pages, align, false);
> if (!pages)
> return -ENOMEM;
>
> diff --git a/include/linux/cma.h b/include/linux/cma.h
> index bf90f0bb42bd..190184b5ff32 100644
> --- a/include/linux/cma.h
> +++ b/include/linux/cma.h
> @@ -33,7 +33,7 @@ extern int cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
> const char *name,
> struct cma **res_cma);
> extern struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
> - gfp_t gfp_mask);
> + bool no_warn);
> extern bool cma_release(struct cma *cma, const struct page *pages, unsigned int count);
>
> extern int cma_for_each_area(int (*it)(struct cma *cma, void *data), void *data);
> diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
> index d987dcd1bd56..19ea5d70150c 100644
> --- a/kernel/dma/contiguous.c
> +++ b/kernel/dma/contiguous.c
> @@ -191,7 +191,8 @@ struct page *dma_alloc_from_contiguous(struct device *dev, size_t count,
> if (align > CONFIG_CMA_ALIGNMENT)
> align = CONFIG_CMA_ALIGNMENT;
>
> - return cma_alloc(dev_get_cma_area(dev), count, align, gfp_mask);
> + return cma_alloc(dev_get_cma_area(dev), count, align,
> + gfp_mask & __GFP_NOWARN);
> }
>
> /**
> diff --git a/mm/cma.c b/mm/cma.c
> index 5809bbe360d7..4cb76121a3ab 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -395,13 +395,13 @@ static inline void cma_debug_show_areas(struct cma *cma) { }
> * @cma: Contiguous memory region for which the allocation is performed.
> * @count: Requested number of pages.
> * @align: Requested alignment of pages (in PAGE_SIZE order).
> - * @gfp_mask: GFP mask to use during compaction
> + * @no_warn: Avoid printing message about failed allocation
> *
> * This function allocates part of contiguous memory on specific
> * contiguous memory area.
> */
> struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
> - gfp_t gfp_mask)
> + bool no_warn)
> {
> unsigned long mask, offset;
> unsigned long pfn = -1;
> @@ -447,7 +447,7 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
> pfn = cma->base_pfn + (bitmap_no << cma->order_per_bit);
> mutex_lock(&cma_mutex);
> ret = alloc_contig_range(pfn, pfn + count, MIGRATE_CMA,
> - gfp_mask);
> + GFP_KERNEL | (no_warn ? __GFP_NOWARN : 0));
> mutex_unlock(&cma_mutex);
> if (ret == 0) {
> page = pfn_to_page(pfn);
> @@ -466,7 +466,7 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
>
> trace_cma_alloc(pfn, page, count, align)>
> - if (ret && !(gfp_mask & __GFP_NOWARN)) {
> + if (ret && !no_warn) {
> pr_err("%s: alloc failed, req-size: %zu pages, ret: %d\n",
> __func__, count, ret);
> cma_debug_show_areas(cma);
> diff --git a/mm/cma_debug.c b/mm/cma_debug.c
> index f23467291cfb..ad6723e9d110 100644
> --- a/mm/cma_debug.c
> +++ b/mm/cma_debug.c
> @@ -139,7 +139,7 @@ static int cma_alloc_mem(struct cma *cma, int count)
> if (!mem)
> return -ENOMEM;
>
> - p = cma_alloc(cma, count, 0, GFP_KERNEL);
> + p = cma_alloc(cma, count, 0, false);
> if (!p) {
> kfree(mem);
> return -ENOMEM;
> --
> 2.17.1
>
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc()
2018-07-09 12:19 ` [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc() Marek Szyprowski
2018-07-09 13:09 ` Michal Hocko
@ 2018-07-09 17:27 ` Laura Abbott
2018-07-10 7:19 ` Joonsoo Kim
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Laura Abbott @ 2018-07-09 17:27 UTC (permalink / raw)
To: Marek Szyprowski, linux-mm, linux-kernel, linux-arm-kernel,
linuxppc-dev, iommu
Cc: Andrew Morton, Michal Nazarewicz, Joonsoo Kim, Vlastimil Babka,
Christoph Hellwig, Michal Hocko, Russell King, Catalin Marinas,
Will Deacon, Paul Mackerras, Benjamin Herrenschmidt,
Chris Zankel, Martin Schwidefsky, Joerg Roedel, Sumit Semwal,
Robin Murphy, linaro-mm-sig
On 07/09/2018 05:19 AM, Marek Szyprowski wrote:
> cma_alloc() function doesn't really support gfp flags other than
> __GFP_NOWARN, so convert gfp_mask parameter to boolean no_warn parameter.
>
> This will help to avoid giving false feeling that this function supports
> standard gfp flags and callers can pass __GFP_ZERO to get zeroed buffer,
> what has already been an issue: see commit dd65a941f6ba ("arm64:
> dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag").
>
For Ion,
Acked-by: Laura Abbott <labbott@redhat.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> arch/powerpc/kvm/book3s_hv_builtin.c | 2 +-
> drivers/s390/char/vmcp.c | 2 +-
> drivers/staging/android/ion/ion_cma_heap.c | 2 +-
> include/linux/cma.h | 2 +-
> kernel/dma/contiguous.c | 3 ++-
> mm/cma.c | 8 ++++----
> mm/cma_debug.c | 2 +-
> 7 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_builtin.c b/arch/powerpc/kvm/book3s_hv_builtin.c
> index d4a3f4da409b..fc6bb9630a9c 100644
> --- a/arch/powerpc/kvm/book3s_hv_builtin.c
> +++ b/arch/powerpc/kvm/book3s_hv_builtin.c
> @@ -77,7 +77,7 @@ struct page *kvm_alloc_hpt_cma(unsigned long nr_pages)
> VM_BUG_ON(order_base_2(nr_pages) < KVM_CMA_CHUNK_ORDER - PAGE_SHIFT);
>
> return cma_alloc(kvm_cma, nr_pages, order_base_2(HPT_ALIGN_PAGES),
> - GFP_KERNEL);
> + false);
> }
> EXPORT_SYMBOL_GPL(kvm_alloc_hpt_cma);
>
> diff --git a/drivers/s390/char/vmcp.c b/drivers/s390/char/vmcp.c
> index 948ce82a7725..0fa1b6b1491a 100644
> --- a/drivers/s390/char/vmcp.c
> +++ b/drivers/s390/char/vmcp.c
> @@ -68,7 +68,7 @@ static void vmcp_response_alloc(struct vmcp_session *session)
> * anymore the system won't work anyway.
> */
> if (order > 2)
> - page = cma_alloc(vmcp_cma, nr_pages, 0, GFP_KERNEL);
> + page = cma_alloc(vmcp_cma, nr_pages, 0, false);
> if (page) {
> session->response = (char *)page_to_phys(page);
> session->cma_alloc = 1;
> diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
> index 49718c96bf9e..3fafd013d80a 100644
> --- a/drivers/staging/android/ion/ion_cma_heap.c
> +++ b/drivers/staging/android/ion/ion_cma_heap.c
> @@ -39,7 +39,7 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
> if (align > CONFIG_CMA_ALIGNMENT)
> align = CONFIG_CMA_ALIGNMENT;
>
> - pages = cma_alloc(cma_heap->cma, nr_pages, align, GFP_KERNEL);
> + pages = cma_alloc(cma_heap->cma, nr_pages, align, false);
> if (!pages)
> return -ENOMEM;
>
> diff --git a/include/linux/cma.h b/include/linux/cma.h
> index bf90f0bb42bd..190184b5ff32 100644
> --- a/include/linux/cma.h
> +++ b/include/linux/cma.h
> @@ -33,7 +33,7 @@ extern int cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
> const char *name,
> struct cma **res_cma);
> extern struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
> - gfp_t gfp_mask);
> + bool no_warn);
> extern bool cma_release(struct cma *cma, const struct page *pages, unsigned int count);
>
> extern int cma_for_each_area(int (*it)(struct cma *cma, void *data), void *data);
> diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
> index d987dcd1bd56..19ea5d70150c 100644
> --- a/kernel/dma/contiguous.c
> +++ b/kernel/dma/contiguous.c
> @@ -191,7 +191,8 @@ struct page *dma_alloc_from_contiguous(struct device *dev, size_t count,
> if (align > CONFIG_CMA_ALIGNMENT)
> align = CONFIG_CMA_ALIGNMENT;
>
> - return cma_alloc(dev_get_cma_area(dev), count, align, gfp_mask);
> + return cma_alloc(dev_get_cma_area(dev), count, align,
> + gfp_mask & __GFP_NOWARN);
> }
>
> /**
> diff --git a/mm/cma.c b/mm/cma.c
> index 5809bbe360d7..4cb76121a3ab 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -395,13 +395,13 @@ static inline void cma_debug_show_areas(struct cma *cma) { }
> * @cma: Contiguous memory region for which the allocation is performed.
> * @count: Requested number of pages.
> * @align: Requested alignment of pages (in PAGE_SIZE order).
> - * @gfp_mask: GFP mask to use during compaction
> + * @no_warn: Avoid printing message about failed allocation
> *
> * This function allocates part of contiguous memory on specific
> * contiguous memory area.
> */
> struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
> - gfp_t gfp_mask)
> + bool no_warn)
> {
> unsigned long mask, offset;
> unsigned long pfn = -1;
> @@ -447,7 +447,7 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
> pfn = cma->base_pfn + (bitmap_no << cma->order_per_bit);
> mutex_lock(&cma_mutex);
> ret = alloc_contig_range(pfn, pfn + count, MIGRATE_CMA,
> - gfp_mask);
> + GFP_KERNEL | (no_warn ? __GFP_NOWARN : 0));
> mutex_unlock(&cma_mutex);
> if (ret == 0) {
> page = pfn_to_page(pfn);
> @@ -466,7 +466,7 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
>
> trace_cma_alloc(pfn, page, count, align);
>
> - if (ret && !(gfp_mask & __GFP_NOWARN)) {
> + if (ret && !no_warn) {
> pr_err("%s: alloc failed, req-size: %zu pages, ret: %d\n",
> __func__, count, ret);
> cma_debug_show_areas(cma);
> diff --git a/mm/cma_debug.c b/mm/cma_debug.c
> index f23467291cfb..ad6723e9d110 100644
> --- a/mm/cma_debug.c
> +++ b/mm/cma_debug.c
> @@ -139,7 +139,7 @@ static int cma_alloc_mem(struct cma *cma, int count)
> if (!mem)
> return -ENOMEM;
>
> - p = cma_alloc(cma, count, 0, GFP_KERNEL);
> + p = cma_alloc(cma, count, 0, false);
> if (!p) {
> kfree(mem);
> return -ENOMEM;
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc()
2018-07-09 12:19 ` [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc() Marek Szyprowski
2018-07-09 13:09 ` Michal Hocko
2018-07-09 17:27 ` Laura Abbott
@ 2018-07-10 7:19 ` Joonsoo Kim
2018-07-10 9:50 ` Michal Hocko
2018-07-16 7:45 ` Vlastimil Babka
2018-07-17 15:08 ` Christoph Hellwig
4 siblings, 1 reply; 15+ messages in thread
From: Joonsoo Kim @ 2018-07-10 7:19 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Linux Memory Management List, LKML, linux-arm-kernel,
linuxppc-dev, iommu, Andrew Morton, Michal Nazarewicz,
Joonsoo Kim, Vlastimil Babka, Christoph Hellwig, Michal Hocko,
Russell King, Catalin Marinas, Will Deacon, Paul Mackerras,
Benjamin Herrenschmidt, Chris Zankel, Martin Schwidefsky,
Joerg Roedel, Sumit Semwal, Robin Murphy, Laura Abbott,
linaro-mm-sig
Hello, Marek.
2018-07-09 21:19 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> cma_alloc() function doesn't really support gfp flags other than
> __GFP_NOWARN, so convert gfp_mask parameter to boolean no_warn parameter.
Although gfp_mask isn't used in cma_alloc() except no_warn, it can be used
in alloc_contig_range(). For example, if passed gfp mask has no __GFP_FS,
compaction(isolation) would work differently. Do you have considered
such a case?
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc()
2018-07-10 7:19 ` Joonsoo Kim
@ 2018-07-10 9:50 ` Michal Hocko
2018-07-11 7:35 ` Joonsoo Kim
0 siblings, 1 reply; 15+ messages in thread
From: Michal Hocko @ 2018-07-10 9:50 UTC (permalink / raw)
To: Joonsoo Kim
Cc: Marek Szyprowski, Linux Memory Management List, LKML,
linux-arm-kernel, linuxppc-dev, iommu, Andrew Morton,
Michal Nazarewicz, Joonsoo Kim, Vlastimil Babka,
Christoph Hellwig, Russell King, Catalin Marinas, Will Deacon,
Paul Mackerras, Benjamin Herrenschmidt, Chris Zankel,
Martin Schwidefsky, Joerg Roedel, Sumit Semwal, Robin Murphy,
Laura Abbott, linaro-mm-sig
On Tue 10-07-18 16:19:32, Joonsoo Kim wrote:
> Hello, Marek.
>
> 2018-07-09 21:19 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> > cma_alloc() function doesn't really support gfp flags other than
> > __GFP_NOWARN, so convert gfp_mask parameter to boolean no_warn parameter.
>
> Although gfp_mask isn't used in cma_alloc() except no_warn, it can be used
> in alloc_contig_range(). For example, if passed gfp mask has no __GFP_FS,
> compaction(isolation) would work differently. Do you have considered
> such a case?
Does any of cma_alloc users actually care about GFP_NO{FS,IO}?
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc()
2018-07-10 9:50 ` Michal Hocko
@ 2018-07-11 7:35 ` Joonsoo Kim
2018-07-11 8:54 ` Michal Hocko
0 siblings, 1 reply; 15+ messages in thread
From: Joonsoo Kim @ 2018-07-11 7:35 UTC (permalink / raw)
To: Michal Hocko
Cc: Marek Szyprowski, Linux Memory Management List, LKML,
linux-arm-kernel, linuxppc-dev, iommu, Andrew Morton,
Michal Nazarewicz, Joonsoo Kim, Vlastimil Babka,
Christoph Hellwig, Russell King, Catalin Marinas, Will Deacon,
Paul Mackerras, Benjamin Herrenschmidt, Chris Zankel,
Martin Schwidefsky, Joerg Roedel, Sumit Semwal, Robin Murphy,
Laura Abbott, linaro-mm-sig
2018-07-10 18:50 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> On Tue 10-07-18 16:19:32, Joonsoo Kim wrote:
>> Hello, Marek.
>>
>> 2018-07-09 21:19 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
>> > cma_alloc() function doesn't really support gfp flags other than
>> > __GFP_NOWARN, so convert gfp_mask parameter to boolean no_warn parameter.
>>
>> Although gfp_mask isn't used in cma_alloc() except no_warn, it can be used
>> in alloc_contig_range(). For example, if passed gfp mask has no __GFP_FS,
>> compaction(isolation) would work differently. Do you have considered
>> such a case?
>
> Does any of cma_alloc users actually care about GFP_NO{FS,IO}?
I don't know. My guess is that cma_alloc() is used for DMA allocation so
block device would use it, too. If fs/block subsystem initiates the
request for the device,
it would be possible that cma_alloc() is called with such a flag.
Again, I don't know
much about those subsystem so I would be wrong.
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc()
2018-07-11 7:35 ` Joonsoo Kim
@ 2018-07-11 8:54 ` Michal Hocko
2018-07-12 2:48 ` Joonsoo Kim
0 siblings, 1 reply; 15+ messages in thread
From: Michal Hocko @ 2018-07-11 8:54 UTC (permalink / raw)
To: Joonsoo Kim
Cc: Marek Szyprowski, Linux Memory Management List, LKML,
linux-arm-kernel, linuxppc-dev, iommu, Andrew Morton,
Michal Nazarewicz, Joonsoo Kim, Vlastimil Babka,
Christoph Hellwig, Russell King, Catalin Marinas, Will Deacon,
Paul Mackerras, Benjamin Herrenschmidt, Chris Zankel,
Martin Schwidefsky, Joerg Roedel, Sumit Semwal, Robin Murphy,
Laura Abbott, linaro-mm-sig
On Wed 11-07-18 16:35:28, Joonsoo Kim wrote:
> 2018-07-10 18:50 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > On Tue 10-07-18 16:19:32, Joonsoo Kim wrote:
> >> Hello, Marek.
> >>
> >> 2018-07-09 21:19 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> >> > cma_alloc() function doesn't really support gfp flags other than
> >> > __GFP_NOWARN, so convert gfp_mask parameter to boolean no_warn parameter.
> >>
> >> Although gfp_mask isn't used in cma_alloc() except no_warn, it can be used
> >> in alloc_contig_range(). For example, if passed gfp mask has no __GFP_FS,
> >> compaction(isolation) would work differently. Do you have considered
> >> such a case?
> >
> > Does any of cma_alloc users actually care about GFP_NO{FS,IO}?
>
> I don't know. My guess is that cma_alloc() is used for DMA allocation so
> block device would use it, too. If fs/block subsystem initiates the
> request for the device,
> it would be possible that cma_alloc() is called with such a flag.
> Again, I don't know
> much about those subsystem so I would be wrong.
The patch converts existing users and none of them really tries to use
anything other than GFP_KERNEL [|__GFP_NOWARN] so this doesn't seem to
be the case. Should there be a new user requiring more restricted
gfp_mask we should carefuly re-evaluate and think how to support it.
Until then I would simply stick with the proposed approach because my
experience tells me that a wrong gfp mask usage is way too easy so the
simpler the api is the less likely we will see an abuse.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc()
2018-07-11 8:54 ` Michal Hocko
@ 2018-07-12 2:48 ` Joonsoo Kim
2018-07-12 7:15 ` Christoph Hellwig
0 siblings, 1 reply; 15+ messages in thread
From: Joonsoo Kim @ 2018-07-12 2:48 UTC (permalink / raw)
To: Michal Hocko
Cc: Marek Szyprowski, Linux Memory Management List, LKML,
linux-arm-kernel, linuxppc-dev, iommu, Andrew Morton,
Michal Nazarewicz, Joonsoo Kim, Vlastimil Babka,
Christoph Hellwig, Russell King, Catalin Marinas, Will Deacon,
Paul Mackerras, Benjamin Herrenschmidt, Chris Zankel,
Martin Schwidefsky, Joerg Roedel, Sumit Semwal, Robin Murphy,
Laura Abbott, linaro-mm-sig
2018-07-11 17:54 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> On Wed 11-07-18 16:35:28, Joonsoo Kim wrote:
>> 2018-07-10 18:50 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
>> > On Tue 10-07-18 16:19:32, Joonsoo Kim wrote:
>> >> Hello, Marek.
>> >>
>> >> 2018-07-09 21:19 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
>> >> > cma_alloc() function doesn't really support gfp flags other than
>> >> > __GFP_NOWARN, so convert gfp_mask parameter to boolean no_warn parameter.
>> >>
>> >> Although gfp_mask isn't used in cma_alloc() except no_warn, it can be used
>> >> in alloc_contig_range(). For example, if passed gfp mask has no __GFP_FS,
>> >> compaction(isolation) would work differently. Do you have considered
>> >> such a case?
>> >
>> > Does any of cma_alloc users actually care about GFP_NO{FS,IO}?
>>
>> I don't know. My guess is that cma_alloc() is used for DMA allocation so
>> block device would use it, too. If fs/block subsystem initiates the
>> request for the device,
>> it would be possible that cma_alloc() is called with such a flag.
>> Again, I don't know
>> much about those subsystem so I would be wrong.
>
> The patch converts existing users and none of them really tries to use
> anything other than GFP_KERNEL [|__GFP_NOWARN] so this doesn't seem to
> be the case. Should there be a new user requiring more restricted
> gfp_mask we should carefuly re-evaluate and think how to support it.
One of existing user is general DMA layer and it takes gfp flags that is
provided by user. I don't check all the DMA allocation sites but how do
you convince that none of them try to use anything other
than GFP_KERNEL [|__GFP_NOWARN]?
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc()
2018-07-12 2:48 ` Joonsoo Kim
@ 2018-07-12 7:15 ` Christoph Hellwig
2018-07-13 6:29 ` Joonsoo Kim
0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2018-07-12 7:15 UTC (permalink / raw)
To: Joonsoo Kim
Cc: Michal Hocko, Marek Szyprowski, Linux Memory Management List,
LKML, linux-arm-kernel, linuxppc-dev, iommu, Andrew Morton,
Michal Nazarewicz, Joonsoo Kim, Vlastimil Babka,
Christoph Hellwig, Russell King, Catalin Marinas, Will Deacon,
Paul Mackerras, Benjamin Herrenschmidt, Chris Zankel,
Martin Schwidefsky, Joerg Roedel, Sumit Semwal, Robin Murphy,
Laura Abbott, linaro-mm-sig
On Thu, Jul 12, 2018 at 11:48:47AM +0900, Joonsoo Kim wrote:
> One of existing user is general DMA layer and it takes gfp flags that is
> provided by user. I don't check all the DMA allocation sites but how do
> you convince that none of them try to use anything other
> than GFP_KERNEL [|__GFP_NOWARN]?
They use a few others things still like __GFP_COMP, __GPF_DMA or
GFP_HUGEPAGE. But all these are bogus as we have various implementations
that can't respect them. I plan to get rid of the gfp_t argument
in the dma_map_ops alloc method in a few merge windows because of that,
but it needs further implementation consolidation first.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc()
2018-07-12 7:15 ` Christoph Hellwig
@ 2018-07-13 6:29 ` Joonsoo Kim
0 siblings, 0 replies; 15+ messages in thread
From: Joonsoo Kim @ 2018-07-13 6:29 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Michal Hocko, Marek Szyprowski, Linux Memory Management List,
LKML, linux-arm-kernel, linuxppc-dev, iommu, Andrew Morton,
Michal Nazarewicz, Joonsoo Kim, Vlastimil Babka, Russell King,
Catalin Marinas, Will Deacon, Paul Mackerras,
Benjamin Herrenschmidt, Chris Zankel, Martin Schwidefsky,
Joerg Roedel, Sumit Semwal, Robin Murphy, Laura Abbott,
linaro-mm-sig
2018-07-12 16:15 GMT+09:00 Christoph Hellwig <hch@lst.de>:
> On Thu, Jul 12, 2018 at 11:48:47AM +0900, Joonsoo Kim wrote:
>> One of existing user is general DMA layer and it takes gfp flags that is
>> provided by user. I don't check all the DMA allocation sites but how do
>> you convince that none of them try to use anything other
>> than GFP_KERNEL [|__GFP_NOWARN]?
>
> They use a few others things still like __GFP_COMP, __GPF_DMA or
> GFP_HUGEPAGE. But all these are bogus as we have various implementations
> that can't respect them. I plan to get rid of the gfp_t argument
> in the dma_map_ops alloc method in a few merge windows because of that,
> but it needs further implementation consolidation first.
Okay. If those flags are all, this change would be okay.
For the remind of this gfp flag introduction in cma_alloc(), see the
following link.
https://marc.info/?l=linux-mm&m=148431452118407
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc()
2018-07-09 12:19 ` [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc() Marek Szyprowski
` (2 preceding siblings ...)
2018-07-10 7:19 ` Joonsoo Kim
@ 2018-07-16 7:45 ` Vlastimil Babka
2018-07-17 15:08 ` Christoph Hellwig
4 siblings, 0 replies; 15+ messages in thread
From: Vlastimil Babka @ 2018-07-16 7:45 UTC (permalink / raw)
To: Marek Szyprowski, linux-mm, linux-kernel, linux-arm-kernel,
linuxppc-dev, iommu
Cc: Andrew Morton, Michal Nazarewicz, Joonsoo Kim, Christoph Hellwig,
Michal Hocko, Russell King, Catalin Marinas, Will Deacon,
Paul Mackerras, Benjamin Herrenschmidt, Chris Zankel,
Martin Schwidefsky, Joerg Roedel, Sumit Semwal, Robin Murphy,
Laura Abbott, linaro-mm-sig
On 07/09/2018 02:19 PM, Marek Szyprowski wrote:
> cma_alloc() function doesn't really support gfp flags other than
> __GFP_NOWARN, so convert gfp_mask parameter to boolean no_warn parameter.
>
> This will help to avoid giving false feeling that this function supports
> standard gfp flags and callers can pass __GFP_ZERO to get zeroed buffer,
> what has already been an issue: see commit dd65a941f6ba ("arm64:
> dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag").
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc()
2018-07-09 12:19 ` [PATCH 1/2] mm/cma: remove unsupported gfp_mask parameter from cma_alloc() Marek Szyprowski
` (3 preceding siblings ...)
2018-07-16 7:45 ` Vlastimil Babka
@ 2018-07-17 15:08 ` Christoph Hellwig
4 siblings, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2018-07-17 15:08 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-mm, linux-kernel, linux-arm-kernel, linuxppc-dev, iommu,
Andrew Morton, Michal Nazarewicz, Joonsoo Kim, Vlastimil Babka,
Christoph Hellwig, Michal Hocko, Russell King, Catalin Marinas,
Will Deacon, Paul Mackerras, Benjamin Herrenschmidt,
Chris Zankel, Martin Schwidefsky, Joerg Roedel, Sumit Semwal,
Robin Murphy, Laura Abbott, linaro-mm-sig
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 15+ messages in thread