From patchwork Wed Apr 27 03:26:50 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jassi Brar X-Patchwork-Id: 8952131 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 0A33CBF29F for ; Wed, 27 Apr 2016 03:28:54 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 2083120225 for ; Wed, 27 Apr 2016 03:28:53 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 45286200FE for ; Wed, 27 Apr 2016 03:28:52 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1avG8A-0002tm-FM; Wed, 27 Apr 2016 03:27:14 +0000 Received: from mail-io0-x22e.google.com ([2607:f8b0:4001:c06::22e]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1avG88-0002nG-3Q for linux-arm-kernel@lists.infradead.org; Wed, 27 Apr 2016 03:27:12 +0000 Received: by mail-io0-x22e.google.com with SMTP id u185so40594135iod.3 for ; Tue, 26 Apr 2016 20:26:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc; bh=mxPGPMaDZPseDz6R4mmEoVy4Oej7QRshM32iycRFsgI=; b=TwcS6wvYitmQaJnYIR913W/Ww2f8ciS/VsFR+lzfZONolQrKyjoqCRzQY18yDFK+1L DVEaaALCQpYODkzUwsiKGat6EKs+LRsj4rG9BDlPuaCQQLs1W9/Vpw/+W9zGFf4Di4Wi 3tHeojKT1L+hUUIp/S/B916Rs84xMjAimfk63cla84NzZJXdN/F13v3lGkBTo1FPEEXg ELHx022PJJyaBm4efPh+dI1VAQzw6pDBdbhBmhhD2B0FGuUPRWm6f5JY4EDNA2i7KeVL aiMFEYps16DJkWj1nLzFEBObPc/0N6dW3wRL2sxbFy13biKR/xAT/IO8sC/YHvVaseKU EGoA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=mxPGPMaDZPseDz6R4mmEoVy4Oej7QRshM32iycRFsgI=; b=BJaumvSh1Y5bg8UDiyu/IHTOeInseOOb8ZS+t/La7CJfc/YezL3OcWEzFKfgzJd7dq K7+kNighbkHYyt9GI3M3pvv6bwz2wgK57AqFsfCjyzTJmec8ZmJ51eViReb7b9/WwSrQ rutd/pJfM9I5Ve4Q6u5QjMBNC07Ip3XgYLt2k1O10mgXL4gk8V7kyfDFp+iNv0thwNTH 4mrEFVp3UZJUOFmbscsjS3fkpOnuD72qVYo6+CmLRHjqo232YaMqF6/tvbDd0/zoUJ9J wxOYsbC1F3oquOMYBCc1XgbDmbkzmnFq6FRj7uxzOpim7N5+LG9tuviWM6YMdp7t/YKf rYAw== X-Gm-Message-State: AOPr4FX/kWq3IsJkrVka4mv++kwCd6JhwkzC8H5QcqWBunknMqWzPy4aJspRK3aVju2Ve5XYTePDiyiDRIxpQQ== MIME-Version: 1.0 X-Received: by 10.107.201.207 with SMTP id z198mr7600237iof.120.1461727611023; Tue, 26 Apr 2016 20:26:51 -0700 (PDT) Received: by 10.107.138.226 with HTTP; Tue, 26 Apr 2016 20:26:50 -0700 (PDT) In-Reply-To: References: Date: Wed, 27 Apr 2016 08:56:50 +0530 Message-ID: Subject: Re: [PATCH] dmaengine: pl330: Fix race in pl330_get_desc() From: Jassi Brar To: Robin Murphy X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160426_202712_191417_C11706B6 X-CRM114-Status: GOOD ( 20.52 ) X-Spam-Score: -2.7 (--) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Vinod Koul , dmaengine@vger.kernel.org, Dan Williams , Linux Kernel Mailing List , "linux-arm-kernel@lists.infradead.org" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Apr 26, 2016 at 10:00 PM, Robin Murphy wrote: > The current logic in pl330_get_desc() contains a clear race condition, > whereby if the descriptor pool is empty, we will create a new > descriptor, add it to the pool with the lock held, *release the lock*, > then try to remove it from the pool again. > > Furthermore, if that second attempt then finds the pool empty because > someone else got there first, we conclude that something must have gone > terribly wrong and it's the absolute end of the world. > We may retry or simply increase the number of descriptors allocated in a batch from 1 to, say, NR_DEFAULT_DESC. /* Try again */ > ... > [ 709.328891] dma-pl330 7ff00000.dma: pl330_get_desc:2459 ALERT! > ** 10 printk messages dropped ** [ 709.352280] dma-pl330 7ff00000.dma: __pl330_prep_dma_memcpy:2493 Unable to fetch desc > ** 11 printk messages dropped ** [ 709.372930] dma-pl330 7ff00000.dma: pl330_get_desc:2459 ALERT! > ** 2 printk messages dropped ** [ 709.372953] dma-pl330 7ff00000.dma: pl330_get_desc:2459 ALERT! > ** 8 printk messages dropped ** [ 709.374157] dma-pl330 7ff00000.dma: __pl330_prep_dma_memcpy:2493 Unable to fetch desc > ** 41 printk messages dropped ** [ 709.393095] dmatest: dma0chan4-copy1: result #4545: 'prep error' with src_off=0x3a32 dst_off=0x46dd len=0xc5c3 (0) > ... > > Down with this sort of thing! The most sensible thing to do is avoid the > allocate-add-remove dance entirely and simply use the freshly-allocated > descriptor straight away. > ... but you still try to first pluck from the list. Instead of churning the code, I would suggest either check in a loop that we have a desc OR allocate 2 or NR_DEFAULT_DESC descriptors there. Probably we get more descriptors at the same cost of memory. > > I'm also seeing what looks like another occasional race under the same > conditions where pl330_tx_submit() blows up from dma_cookie_assign() > dereferencing a bogus tx->chan, but I think that's beyond my ability to > figure out right now. Similarly the storm of WARNs from pl330_issue_pending() > when using a large number of small buffers and dmatest.noverify=1. This > one was some obvious low-hanging fruit. > Sorry, that part of code has changed a lot since I wrote the driver, so more details will help me. Thanks. diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index 372b435..7179a4d 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -2449,7 +2449,7 @@ static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch) /* If the DMAC pool is empty, alloc new */ if (!desc) { - if (!add_desc(pl330, GFP_ATOMIC, 1)) + if (!add_desc(pl330, GFP_ATOMIC, NR_DEFAULT_DESC)) return NULL;