Messages in this thread | | | Subject | Re: [PATCH] mm/cma.c: remove redundant cma_mutex lock | From | Vlastimil Babka <> | Date | Fri, 23 Oct 2020 13:29:16 +0200 |
| |
On 10/20/20 1:27 PM, David Hildenbrand wrote: > On 20.10.20 12:22, Lecopzer Chen wrote: >> The cma_mutex which protects alloc_contig_range() was first appeared in >> commit 7ee793a62fa8c ("cma: Remove potential deadlock situation"), >> at that time, there is no guarantee the behavior of concurrency inside >> alloc_contig_range(). >> >> After the commit 2c7452a075d4db2dc >> ("mm/page_isolation.c: make start_isolate_page_range() fail if already isolated") >> > However, two subsystems (CMA and gigantic >> > huge pages for example) could attempt operations on the same range. If >> > this happens, one thread may 'undo' the work another thread is doing. >> > This can result in pageblocks being incorrectly left marked as >> > MIGRATE_ISOLATE and therefore not available for page allocation. >> The concurrency inside alloc_contig_range() was clarified. >> >> Now we can find that hugepage and virtio call alloc_contig_range() without >> any lock, thus cma_mutex is "redundant" in cma_alloc() now. >> >> Signed-off-by: Lecopzer Chen <lecopzer.chen@mediatek.com> >> --- >> mm/cma.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/mm/cma.c b/mm/cma.c >> index 7f415d7cda9f..3692a34e2353 100644 >> --- a/mm/cma.c >> +++ b/mm/cma.c >> @@ -38,7 +38,6 @@ >> >> struct cma cma_areas[MAX_CMA_AREAS]; >> unsigned cma_area_count; >> -static DEFINE_MUTEX(cma_mutex); >> >> phys_addr_t cma_get_base(const struct cma *cma) >> { >> @@ -454,10 +453,9 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align, >> mutex_unlock(&cma->lock); >> >> pfn = cma->base_pfn + (bitmap_no << cma->order_per_bit); >> - mutex_lock(&cma_mutex); >> ret = alloc_contig_range(pfn, pfn + count, MIGRATE_CMA, >> GFP_KERNEL | (no_warn ? __GFP_NOWARN : 0)); >> - mutex_unlock(&cma_mutex); >> + >> if (ret == 0) { >> page = pfn_to_page(pfn); >> break; >> > > I guess this is fine. In case there is a race we return with -EBUSY - > which is suboptimal (as it could just be a temporary issue if the other > user backs off), but should be good enough for now.
Agreed.
> Acked-by: David Hildenbrand <david@redhat.com>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
| |