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: 8952051 Return-Path: X-Original-To: patchwork-dmaengine@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 12485BF29F for ; Wed, 27 Apr 2016 03:27:13 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 2AC3A200FE for ; Wed, 27 Apr 2016 03:27:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 37406200CA for ; Wed, 27 Apr 2016 03:27:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752622AbcD0D0x (ORCPT ); Tue, 26 Apr 2016 23:26:53 -0400 Received: from mail-io0-f173.google.com ([209.85.223.173]:32955 "EHLO mail-io0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753051AbcD0D0w (ORCPT ); Tue, 26 Apr 2016 23:26:52 -0400 Received: by mail-io0-f173.google.com with SMTP id f89so35100232ioi.0; 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=kTvpEOFL1VDmxjq1zOemhVjr/c63lzPBjLpdecLGPJpabpfI4AtsKdh2Rc0u/SMtPx jB9hQyMzQTe5hzX+hZIlk+UM/sqJVlUps/oZbfapsSJJJ/7eSSHiLJKX8JZkKMCc6g3/ opoyDN2p7u2ugmWePGRMJBRQYP+WKlfH6GCcpmo2SCfDZS5IyqaNMpdTVInraeKbqxkt m5a/YUtJlYwFqM7STyIAdBaHHUiyE0/FvcPSqpCW5ywalnO3lc5rG7BBjZNQ2BFnDSU0 NUrisVOXVHS59VtPyCAVgLT/swFNYELH8OqNjz0jV0BC5rfJ0+tjPcmhLMpCJ/UE6NLx satA== X-Gm-Message-State: AOPr4FWuSAHqZ1ZVtxD07oiQfI1ZpQU7RcTOxxjLFZg+o/E7nzhskG9YWLNVt49GOGY5higSYYmXAzna6/wU5A== 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 Cc: Vinod Koul , Dan Williams , dmaengine@vger.kernel.org, Linux Kernel Mailing List , "linux-arm-kernel@lists.infradead.org" Sender: dmaengine-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: dmaengine@vger.kernel.org X-Spam-Status: No, score=-7.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, 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. --- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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;