From patchwork Fri Mar 21 18:47:29 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laura Abbott X-Patchwork-Id: 3876331 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id EC2249F382 for ; Fri, 21 Mar 2014 21:01:11 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id E94BF20203 for ; Fri, 21 Mar 2014 21:01:10 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id C7A0E201FB for ; Fri, 21 Mar 2014 21:01:09 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 7CCA56E435; Fri, 21 Mar 2014 14:01:08 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from smtp.codeaurora.org (smtp.codeaurora.org [198.145.11.231]) by gabe.freedesktop.org (Postfix) with ESMTP id 6C50E6E29A for ; Fri, 21 Mar 2014 11:47:30 -0700 (PDT) Received: from smtp.codeaurora.org (localhost [127.0.0.1]) by smtp.codeaurora.org (Postfix) with ESMTP id 48A8413EF19; Fri, 21 Mar 2014 18:47:30 +0000 (UTC) Received: by smtp.codeaurora.org (Postfix, from userid 486) id 3A68B13EFDC; Fri, 21 Mar 2014 18:47:30 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 Received: from [10.42.111.28] (i-global254.qualcomm.com [199.106.103.254]) (using TLSv1 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) (Authenticated sender: lauraa@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id 76EDC13ED8D; Fri, 21 Mar 2014 18:47:29 +0000 (UTC) Message-ID: <532C8941.4090104@codeaurora.org> Date: Fri, 21 Mar 2014 11:47:29 -0700 From: Laura Abbott User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: Michal Nazarewicz , Marek Szyprowski , Russell King - ARM Linux Subject: Re: [BUG] Circular locking dependency - DRM/CMA/MM/hotplug/... References: <20140211183543.GK26684@n2100.arm.linux.org.uk> <52FB9602.1000805@samsung.com> <20140212163317.GQ26684@n2100.arm.linux.org.uk> <53036D51.2070502@samsung.com> In-Reply-To: X-Virus-Scanned: ClamAV using ClamSMTP X-Mailman-Approved-At: Fri, 21 Mar 2014 14:01:07 -0700 Cc: linux-arm-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP On 2/18/2014 9:44 AM, Michal Nazarewicz wrote: >> On 2014-02-12 17:33, Russell King - ARM Linux wrote: >>> What if we did these changes: >>> >>> struct page *dma_alloc_from_contiguous(struct device *dev, int count, >>> unsigned int align) >>> { >>> ... >>> mutex_lock(&cma_mutex); >>> ... >>> for (;;) { >>> pageno = bitmap_find_next_zero_area(cma->bitmap, cma->count, >>> start, count, mask); >>> if (pageno >= cma->count) >>> break; >>> >>> pfn = cma->base_pfn + pageno; >>> + bitmap_set(cma->bitmap, pageno, count); >>> + mutex_unlock(&cma_mutex); >>> ret = alloc_contig_range(pfn, pfn + count, MIGRATE_CMA); >>> + mutex_lock(&cma_mutex); >>> if (ret == 0) { >>> - bitmap_set(cma->bitmap, pageno, count); >>> page = pfn_to_page(pfn); >>> break; >>> - } else if (ret != -EBUSY) { >>> + } >>> + bitmap_clear(cma->bitmap, pageno, count); >>> + if (ret != -EBUSY) { >>> break; >>> } >>> ... >>> mutex_unlock(&cma_mutex); >>> pr_debug("%s(): returned %p\n", __func__, page); >>> return page; >>> } > > Like Marek said, this will fail if two concurrent calls to > alloc_contig_range are made such that they operate on the same pageblock > (which is possible as the allocated regions do not need to be pageblock > aligned). > > Another mutex could be added just for this one call, but as I understand > this would not solve the problem. > >>> bool dma_release_from_contiguous(struct device *dev, struct page *pages, >>> int count) >>> { >>> ... >>> + free_contig_range(pfn, count); >>> mutex_lock(&cma_mutex); >>> bitmap_clear(cma->bitmap, pfn - cma->base_pfn, count); >>> - free_contig_range(pfn, count); >>> mutex_unlock(&cma_mutex); >>> ... >>> } > > This *should* be fine. Didn't test it. > I managed to hit a different deadlock that had a similar root cause. I also managed to independently come up with a similar solution. This has been tested somewhat but not in wide distribution. Thanks, Laura ----- 8< ------ From 2aa000fbd4189d967c45c4f1ac5aee812ed83082 Mon Sep 17 00:00:00 2001 From: Laura Abbott Date: Tue, 25 Feb 2014 11:01:19 -0800 Subject: [PATCH] cma: Remove potential deadlock situation CMA locking is currently very coarse. The cma_mutex protects both the bitmap and avoids concurrency with alloc_contig_range. There are several situations which may result in a deadlock on the CMA mutex currently, mostly involving AB/BA situations with alloc and free. Fix this issue by protecting the bitmap with a mutex per CMA region and use the existing mutex for protecting against concurrency with alloc_contig_range. Signed-off-by: Laura Abbott Acked-by: Michal Nazarewicz --- drivers/base/dma-contiguous.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c index 165c2c2..dfb48ef 100644 --- a/drivers/base/dma-contiguous.c +++ b/drivers/base/dma-contiguous.c @@ -37,6 +37,7 @@ struct cma { unsigned long base_pfn; unsigned long count; unsigned long *bitmap; + struct mutex lock; }; struct cma *dma_contiguous_default_area; @@ -161,6 +162,7 @@ static int __init cma_activate_area(struct cma *cma) init_cma_reserved_pageblock(pfn_to_page(base_pfn)); } while (--i); + mutex_init(&cma->lock); return 0; } @@ -261,6 +263,13 @@ err: return ret; } +static void clear_cma_bitmap(struct cma *cma, unsigned long pfn, int count) +{ + mutex_lock(&cma->lock); + bitmap_clear(cma->bitmap, pfn - cma->base_pfn, count); + mutex_unlock(&cma->lock); +} + /** * dma_alloc_from_contiguous() - allocate pages from contiguous area * @dev: Pointer to device for which the allocation is performed. @@ -294,30 +303,41 @@ struct page *dma_alloc_from_contiguous(struct device *dev, int count, mask = (1 << align) - 1; - mutex_lock(&cma_mutex); for (;;) { + mutex_lock(&cma->lock); pageno = bitmap_find_next_zero_area(cma->bitmap, cma->count, start, count, mask); - if (pageno >= cma->count) + if (pageno >= cma->count) { + mutex_unlock(&cma_mutex); break; + } + bitmap_set(cma->bitmap, pageno, count); + /* + * It's safe to drop the lock here. We've marked this region for + * our exclusive use. If the migration fails we will take the + * lock again and unmark it. + */ + mutex_unlock(&cma->lock); pfn = cma->base_pfn + pageno; + mutex_lock(&cma_mutex); ret = alloc_contig_range(pfn, pfn + count, MIGRATE_CMA); + mutex_unlock(&cma_mutex); if (ret == 0) { - bitmap_set(cma->bitmap, pageno, count); page = pfn_to_page(pfn); break; } else if (ret != -EBUSY) { + clear_cma_bitmap(cma, pfn, count); break; } + clear_cma_bitmap(cma, pfn, count); pr_debug("%s(): memory range at %p is busy, retrying\n", __func__, pfn_to_page(pfn)); /* try again with a bit different memory target */ start = pageno + mask + 1; } - mutex_unlock(&cma_mutex); pr_debug("%s(): returned %p\n", __func__, page); return page; } @@ -350,10 +370,8 @@ bool dma_release_from_contiguous(struct device *dev, struct page *pages, VM_BUG_ON(pfn + count > cma->base_pfn + cma->count); - mutex_lock(&cma_mutex); - bitmap_clear(cma->bitmap, pfn - cma->base_pfn, count); free_contig_range(pfn, count); - mutex_unlock(&cma_mutex); + clear_cma_bitmap(cma, pfn, count); return true; }