[PATCH rfc 0/2] mm: cma: make cma_release() non-blocking

[PATCH rfc 0/2] mm: cma: make cma_release() non-blocking

[Roman Gushchin]

This small patchset makes cma_release() non-blocking and simplifies
the code in hugetlbfs, where previously we had to temporarily drop
hugetlb_lock around the cma_release() call.

It should help Zi Yan on his work on 1 GB THPs: splitting a gigantic
THP under a memory pressure requires a cma_release() call. If it's
a blocking function, it complicates the already complicated code.
Because there are at least two use cases like this (hugetlbfs is
another example), I believe it's just better to make cma_release()
non-blocking.

It also makes it more consistent with other memory releasing functions
in the kernel: most of them are non-blocking.


Roman Gushchin (2):
  mm: cma: make cma_release() non-blocking
  mm: hugetlb: don't drop hugetlb_lock around cma_release() call

 mm/cma.c     | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 mm/hugetlb.c |  6 ------
 2 files changed, 49 insertions(+), 8 deletions(-)

-- 
2.26.2

[PATCH rfc 1/2] mm: cma: make cma_release() non-blocking

[Roman Gushchin]

cma_release() has to lock the cma_lock mutex to clear the cma bitmap.
It makes it a blocking function, which complicates its usage from
non-blocking contexts. For instance, hugetlbfs code is temporarily
dropping the hugetlb_lock spinlock to call cma_release().

This patch makes cma_release non-blocking by postponing the cma
bitmap clearance. It's done later from a work context. The first page
in the cma allocation is used to store the work struct.

To make sure that subsequent cma_alloc() call will pass, cma_alloc()
flushes the corresponding workqueue.

Because CMA allocations and de-allocations are usually not that
frequent, a single global workqueue is used.

Signed-off-by: Roman Gushchin <guro@fb.com>
---
 mm/cma.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/mm/cma.c b/mm/cma.c
index 7f415d7cda9f..523cd9356bc7 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -36,10 +36,19 @@
 
 #include "cma.h"
 
+struct cma_clear_bitmap_work {
+	struct work_struct work;
+	struct cma *cma;
+	unsigned long pfn;
+	unsigned int count;
+};
+
 struct cma cma_areas[MAX_CMA_AREAS];
 unsigned cma_area_count;
 static DEFINE_MUTEX(cma_mutex);
 
+struct workqueue_struct *cma_release_wq;
+
 phys_addr_t cma_get_base(const struct cma *cma)
 {
 	return PFN_PHYS(cma->base_pfn);
@@ -148,6 +157,10 @@ static int __init cma_init_reserved_areas(void)
 	for (i = 0; i < cma_area_count; i++)
 		cma_activate_area(&cma_areas[i]);
 
+	cma_release_wq = create_workqueue("cma_release");
+	if (!cma_release_wq)
+		return -ENOMEM;
+
 	return 0;
 }
 core_initcall(cma_init_reserved_areas);
@@ -437,6 +450,13 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
 		return NULL;
 
 	for (;;) {
+		/*
+		 * CMA bitmaps are cleared asynchronously from works,
+		 * scheduled by cma_release(). To make sure the allocation
+		 * will success, cma release workqueue is flushed here.
+		 */
+		flush_workqueue(cma_release_wq);
+
 		mutex_lock(&cma->lock);
 		bitmap_no = bitmap_find_next_zero_area_off(cma->bitmap,
 				bitmap_maxno, start, bitmap_count, mask,
@@ -495,6 +515,17 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
 	return page;
 }
 
+static void cma_clear_bitmap_fn(struct work_struct *work)
+{
+	struct cma_clear_bitmap_work *w;
+
+	w = container_of(work, struct cma_clear_bitmap_work, work);
+
+	cma_clear_bitmap(w->cma, w->pfn, w->count);
+
+	__free_page(pfn_to_page(w->pfn));
+}
+
 /**
  * cma_release() - release allocated pages
  * @cma:   Contiguous memory region for which the allocation is performed.
@@ -507,6 +538,7 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
  */
 bool cma_release(struct cma *cma, const struct page *pages, unsigned int count)
 {
+	struct cma_clear_bitmap_work *work;
 	unsigned long pfn;
 
 	if (!cma || !pages)
@@ -521,8 +553,23 @@ bool cma_release(struct cma *cma, const struct page *pages, unsigned int count)
 
 	VM_BUG_ON(pfn + count > cma->base_pfn + cma->count);
 
-	free_contig_range(pfn, count);
-	cma_clear_bitmap(cma, pfn, count);
+	/*
+	 * To make cma_release() non-blocking, cma bitmap is cleared from
+	 * a work context (see cma_clear_bitmap_fn()). The first page
+	 * in the cma allocation is used to store the work structure,
+	 * so it's released after the cma bitmap clearance. Other pages
+	 * are released immediately as previously.
+	 */
+	if (count > 1)
+		free_contig_range(pfn + 1, count - 1);
+
+	work = (struct cma_clear_bitmap_work *)page_to_virt(pages);
+	INIT_WORK(&work->work, cma_clear_bitmap_fn);
+	work->cma = cma;
+	work->pfn = pfn;
+	work->count = count;
+	queue_work(cma_release_wq, &work->work);
+
 	trace_cma_release(pfn, pages, count);
 
 	return true;
-- 
2.26.2

[PATCH rfc 2/2] mm: hugetlb: don't drop hugetlb_lock around cma_release() call

[Roman Gushchin]

cma_release() now is a non-blocking function, so there is no more need
to drop hugetlb_lock to call it.

Signed-off-by: Roman Gushchin <guro@fb.com>
---
 mm/hugetlb.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index fe76f8fd5a73..c8c892b1cabc 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1312,14 +1312,8 @@ static void update_and_free_page(struct hstate *h, struct page *page)
 	set_compound_page_dtor(page, NULL_COMPOUND_DTOR);
 	set_page_refcounted(page);
 	if (hstate_is_gigantic(h)) {
-		/*
-		 * Temporarily drop the hugetlb_lock, because
-		 * we might block in free_gigantic_page().
-		 */
-		spin_unlock(&hugetlb_lock);
 		destroy_compound_gigantic_page(page, huge_page_order(h));
 		free_gigantic_page(page, huge_page_order(h));
-		spin_lock(&hugetlb_lock);
 	} else {
 		__free_pages(page, huge_page_order(h));
 	}
-- 
2.26.2

Re: [PATCH rfc 0/2] mm: cma: make cma_release() non-blocking

[Mike Kravetz]

On 10/16/20 3:52 PM, Roman Gushchin wrote:
> This small patchset makes cma_release() non-blocking and simplifies
> the code in hugetlbfs, where previously we had to temporarily drop
> hugetlb_lock around the cma_release() call.
> 
> It should help Zi Yan on his work on 1 GB THPs: splitting a gigantic
> THP under a memory pressure requires a cma_release() call. If it's
> a blocking function, it complicates the already complicated code.
> Because there are at least two use cases like this (hugetlbfs is
> another example), I believe it's just better to make cma_release()
> non-blocking.
> 
> It also makes it more consistent with other memory releasing functions
> in the kernel: most of them are non-blocking.

Thanks for looking into this Roman.

I may be missing something, but why does cma_release have to be blocking
today?  Certainly, it takes the bitmap in cma_clear_bitmap and could
block.  However, I do not see why cma->lock has to be a mutex.  I may be
missing something, but I do not see any code protected by the mutex doing
anything that could sleep?

Could we simply change that mutex to a spinlock?
-- 
Mike Kravetz

Re: [PATCH rfc 0/2] mm: cma: make cma_release() non-blocking

[Xiaqing (A)]

On 2020/10/17 6:52, Roman Gushchin wrote:

> This small patchset makes cma_release() non-blocking and simplifies
> the code in hugetlbfs, where previously we had to temporarily drop
> hugetlb_lock around the cma_release() call.
>
> It should help Zi Yan on his work on 1 GB THPs: splitting a gigantic
> THP under a memory pressure requires a cma_release() call. If it's
> a blocking function, it complicates the already complicated code.
> Because there are at least two use cases like this (hugetlbfs is
> another example), I believe it's just better to make cma_release()
> non-blocking.
>
> It also makes it more consistent with other memory releasing functions
> in the kernel: most of them are non-blocking.
>
>
> Roman Gushchin (2):
>    mm: cma: make cma_release() non-blocking
>    mm: hugetlb: don't drop hugetlb_lock around cma_release() call
>
>   mm/cma.c     | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
>   mm/hugetlb.c |  6 ------
>   2 files changed, 49 insertions(+), 8 deletions(-)
>
I don't think this patch is a good idea.It transfers part or even all of the time of
cma_release to cma_alloc, which is more concerned by performance indicators.
On Android phones, CPU resource competition is intense in many scenarios,
As a result, kernel threads and workers can be scheduled only after some ticks or more.
In this case, the performance of cma_alloc will deteriorate significantly,
which is not good news for many services on Android.

Re: [PATCH rfc 0/2] mm: cma: make cma_release() non-blocking

[Roman Gushchin]

On Wed, Oct 21, 2020 at 05:15:53PM -0700, Mike Kravetz wrote:
> On 10/16/20 3:52 PM, Roman Gushchin wrote:
> > This small patchset makes cma_release() non-blocking and simplifies
> > the code in hugetlbfs, where previously we had to temporarily drop
> > hugetlb_lock around the cma_release() call.
> > 
> > It should help Zi Yan on his work on 1 GB THPs: splitting a gigantic
> > THP under a memory pressure requires a cma_release() call. If it's
> > a blocking function, it complicates the already complicated code.
> > Because there are at least two use cases like this (hugetlbfs is
> > another example), I believe it's just better to make cma_release()
> > non-blocking.
> > 
> > It also makes it more consistent with other memory releasing functions
> > in the kernel: most of them are non-blocking.
> 
> Thanks for looking into this Roman.

Hi Mike,

> 
> I may be missing something, but why does cma_release have to be blocking
> today?  Certainly, it takes the bitmap in cma_clear_bitmap and could
> block.  However, I do not see why cma->lock has to be a mutex.  I may be
> missing something, but I do not see any code protected by the mutex doing
> anything that could sleep?
> 
> Could we simply change that mutex to a spinlock?

I actually have suggested it few months ago, but the idea was
opposed by Joonsoo: https://lkml.org/lkml/2020/4/3/12 .

The time of a bitmap operation is definitely not an issue in my context,
but I can't speak for something like an embedded/rt case.

Thanks!

Re: [PATCH rfc 0/2] mm: cma: make cma_release() non-blocking

[Roman Gushchin]

On Thu, Oct 22, 2020 at 09:54:53AM +0800, Xiaqing (A) wrote:
> 
> 
> On 2020/10/17 6:52, Roman Gushchin wrote:
> 
> > This small patchset makes cma_release() non-blocking and simplifies
> > the code in hugetlbfs, where previously we had to temporarily drop
> > hugetlb_lock around the cma_release() call.
> > 
> > It should help Zi Yan on his work on 1 GB THPs: splitting a gigantic
> > THP under a memory pressure requires a cma_release() call. If it's
> > a blocking function, it complicates the already complicated code.
> > Because there are at least two use cases like this (hugetlbfs is
> > another example), I believe it's just better to make cma_release()
> > non-blocking.
> > 
> > It also makes it more consistent with other memory releasing functions
> > in the kernel: most of them are non-blocking.
> > 
> > 
> > Roman Gushchin (2):
> >    mm: cma: make cma_release() non-blocking
> >    mm: hugetlb: don't drop hugetlb_lock around cma_release() call
> > 
> >   mm/cma.c     | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
> >   mm/hugetlb.c |  6 ------
> >   2 files changed, 49 insertions(+), 8 deletions(-)
> > 
> I don't think this patch is a good idea.It transfers part or even all of the time of
> cma_release to cma_alloc, which is more concerned by performance indicators.

I'm not quite sure: if cma_alloc() is racing with cma_release(), cma_alloc() will
wait for the cma_lock mutex anyway. So we don't really transfer anything to cma_alloc().

> On Android phones, CPU resource competition is intense in many scenarios,
> As a result, kernel threads and workers can be scheduled only after some ticks or more.
> In this case, the performance of cma_alloc will deteriorate significantly,
> which is not good news for many services on Android.

Ok, I agree, if the cpu is heavily loaded, it might affect the total execution time.

If we aren't going into the mutex->spinlock conversion direction (as Mike suggested),
we can address the performance concerns by introducing a cma_release_nowait() function,
so that the default cma_release() would work in the old way.
cma_release_nowait() can set an atomic flag on a cma area, which will cause following
cma_alloc()'s to flush the release queue. In this case there will be no performance
penalty unless somebody is using cma_release_nowait().
Will it work for you?

Thank you!

Re: [PATCH rfc 0/2] mm: cma: make cma_release() non-blocking

[Xiaqing (A)]

On 2020/10/22 10:45, Roman Gushchin wrote:

> On Thu, Oct 22, 2020 at 09:54:53AM +0800, Xiaqing (A) wrote:
>>
>> On 2020/10/17 6:52, Roman Gushchin wrote:
>>
>>> This small patchset makes cma_release() non-blocking and simplifies
>>> the code in hugetlbfs, where previously we had to temporarily drop
>>> hugetlb_lock around the cma_release() call.
>>>
>>> It should help Zi Yan on his work on 1 GB THPs: splitting a gigantic
>>> THP under a memory pressure requires a cma_release() call. If it's
>>> a blocking function, it complicates the already complicated code.
>>> Because there are at least two use cases like this (hugetlbfs is
>>> another example), I believe it's just better to make cma_release()
>>> non-blocking.
>>>
>>> It also makes it more consistent with other memory releasing functions
>>> in the kernel: most of them are non-blocking.
>>>
>>>
>>> Roman Gushchin (2):
>>>     mm: cma: make cma_release() non-blocking
>>>     mm: hugetlb: don't drop hugetlb_lock around cma_release() call
>>>
>>>    mm/cma.c     | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
>>>    mm/hugetlb.c |  6 ------
>>>    2 files changed, 49 insertions(+), 8 deletions(-)
>>>
>> I don't think this patch is a good idea.It transfers part or even all of the time of
>> cma_release to cma_alloc, which is more concerned by performance indicators.
> I'm not quite sure: if cma_alloc() is racing with cma_release(), cma_alloc() will
> wait for the cma_lock mutex anyway. So we don't really transfer anything to cma_alloc().
>
>> On Android phones, CPU resource competition is intense in many scenarios,
>> As a result, kernel threads and workers can be scheduled only after some ticks or more.
>> In this case, the performance of cma_alloc will deteriorate significantly,
>> which is not good news for many services on Android.
> Ok, I agree, if the cpu is heavily loaded, it might affect the total execution time.
>
> If we aren't going into the mutex->spinlock conversion direction (as Mike suggested),
> we can address the performance concerns by introducing a cma_release_nowait() function,
> so that the default cma_release() would work in the old way.
> cma_release_nowait() can set an atomic flag on a cma area, which will cause following
> cma_alloc()'s to flush the release queue. In this case there will be no performance
> penalty unless somebody is using cma_release_nowait().
> Will it work for you?

That looks good to me.

Thanks!

>
> Thank you!
>
>

Re: [PATCH rfc 0/2] mm: cma: make cma_release() non-blocking

[Mike Kravetz]

On 10/21/20 7:33 PM, Roman Gushchin wrote:
> On Wed, Oct 21, 2020 at 05:15:53PM -0700, Mike Kravetz wrote:
>> On 10/16/20 3:52 PM, Roman Gushchin wrote:
>>> This small patchset makes cma_release() non-blocking and simplifies
>>> the code in hugetlbfs, where previously we had to temporarily drop
>>> hugetlb_lock around the cma_release() call.
>>>
>>> It should help Zi Yan on his work on 1 GB THPs: splitting a gigantic
>>> THP under a memory pressure requires a cma_release() call. If it's
>>> a blocking function, it complicates the already complicated code.
>>> Because there are at least two use cases like this (hugetlbfs is
>>> another example), I believe it's just better to make cma_release()
>>> non-blocking.
>>>
>>> It also makes it more consistent with other memory releasing functions
>>> in the kernel: most of them are non-blocking.
>>
>> Thanks for looking into this Roman.
> 
> Hi Mike,
> 
>>
>> I may be missing something, but why does cma_release have to be blocking
>> today?  Certainly, it takes the bitmap in cma_clear_bitmap and could
>> block.  However, I do not see why cma->lock has to be a mutex.  I may be
>> missing something, but I do not see any code protected by the mutex doing
>> anything that could sleep?
>>
>> Could we simply change that mutex to a spinlock?
> 
> I actually have suggested it few months ago, but the idea was
> opposed by Joonsoo: https://lkml.org/lkml/2020/4/3/12 .
> 
> The time of a bitmap operation is definitely not an issue in my context,
> but I can't speak for something like an embedded/rt case.
> 

I wonder if it may be time to look into replacing the cma area bitmap
with some other data structure?  Joonsoo was concerned about the time
required to traverse the bitmap for an 8GB area.  With new support for
allocating 1GB hugetlb pages from cma, I can imagine someone setting
up a cma area that is hundreds of GB if not TB in size.  It is going
take some time to traverse a bitmap describing such a huge area.

-- 
Mike Kravetz

Re: [PATCH rfc 0/2] mm: cma: make cma_release() non-blocking

[Roman Gushchin]

On Thu, Oct 22, 2020 at 09:42:11AM -0700, Mike Kravetz wrote:
> On 10/21/20 7:33 PM, Roman Gushchin wrote:
> > On Wed, Oct 21, 2020 at 05:15:53PM -0700, Mike Kravetz wrote:
> >> On 10/16/20 3:52 PM, Roman Gushchin wrote:
> >>> This small patchset makes cma_release() non-blocking and simplifies
> >>> the code in hugetlbfs, where previously we had to temporarily drop
> >>> hugetlb_lock around the cma_release() call.
> >>>
> >>> It should help Zi Yan on his work on 1 GB THPs: splitting a gigantic
> >>> THP under a memory pressure requires a cma_release() call. If it's
> >>> a blocking function, it complicates the already complicated code.
> >>> Because there are at least two use cases like this (hugetlbfs is
> >>> another example), I believe it's just better to make cma_release()
> >>> non-blocking.
> >>>
> >>> It also makes it more consistent with other memory releasing functions
> >>> in the kernel: most of them are non-blocking.
> >>
> >> Thanks for looking into this Roman.
> > 
> > Hi Mike,
> > 
> >>
> >> I may be missing something, but why does cma_release have to be blocking
> >> today?  Certainly, it takes the bitmap in cma_clear_bitmap and could
> >> block.  However, I do not see why cma->lock has to be a mutex.  I may be
> >> missing something, but I do not see any code protected by the mutex doing
> >> anything that could sleep?
> >>
> >> Could we simply change that mutex to a spinlock?
> > 
> > I actually have suggested it few months ago, but the idea was
> > opposed by Joonsoo: https://lkml.org/lkml/2020/4/3/12 .
> > 
> > The time of a bitmap operation is definitely not an issue in my context,
> > but I can't speak for something like an embedded/rt case.
> > 
> 
> I wonder if it may be time to look into replacing the cma area bitmap
> with some other data structure?  Joonsoo was concerned about the time
> required to traverse the bitmap for an 8GB area.  With new support for
> allocating 1GB hugetlb pages from cma, I can imagine someone setting
> up a cma area that is hundreds of GB if not TB in size.  It is going
> take some time to traverse a bitmap describing such a huge area.

If the cma area is used exclusively for 1 GB allocations, the bitmap can
have only 1 bit per GB, so it shouldn't be a big problem.

Long-term I have some hopes to be able to allocate 1 GB pages without
a need to reserve a cma area: we can try to group pages based on their mobility
on a 1 GB scale, so that all non-movable pages will reside in few 1 GB blocks.
I'm looking into that direction, but don't have any results yet.

If this idea fails and we'll end up allocating a large cma area unconditionally
and shrink it on demand (I think Rik suggested something like this),
replacing the bitmap with something else sounds like a good idea to me.

As now, I want to unblock Zi Yan on his work on 1 GB THPs, so maybe
we can go with introducing cma_release_nowait(), as I suggested in
the other e-mail in this thread? Do you have any objections?

Thanks!

Re: [PATCH rfc 0/2] mm: cma: make cma_release() non-blocking

[Mike Kravetz]

On 10/22/20 10:16 AM, Roman Gushchin wrote:
> As now, I want to unblock Zi Yan on his work on 1 GB THPs, so maybe
> we can go with introducing cma_release_nowait(), as I suggested in
> the other e-mail in this thread? Do you have any objections?

No objections from me.  And, I like that it could be used by the hugetlb
code to make it simpler.

-- 
Mike Kravetz