From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E371BC001B0 for ; Wed, 9 Aug 2023 19:38:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=wbACwmFDHD3c9Ym3GRwWfW7Wus5GuoXD1efELZJ4C3o=; b=w3Hb4xaYUg7sXO gFX3owXU+OUsuJIva2N2pnEOqQEmIcbxQ1/j7x3lbha85G/rNXYUq4mSmH9gjKMm11ZXixVMaKeb0 hxIVo1TVnovvKXOywWxpPGNNkd/yP7h6bziDaXTQAiZBDuNdwYu4T2/XRgbXhTwZdFj9aE8rasKJQ s5Cj9o7fhgDAbzOfwPu1xJrBj7mYTkmZkmHm2HgKFzfWKdosuvdy4/aHc/2xZnUCvkTt5DToDbKNc tQEWmbTEAY248fsb1tcmWOMXx4HkoLwen+I0poSfju5adGmSBLstlYaP12Egy6S9X3Nl5lMMZXG1w NpFOZ3mf5SghJepRG4LQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qTp0i-005kQD-2u; Wed, 09 Aug 2023 19:38:24 +0000 Received: from casper.infradead.org ([2001:8b0:10b:1236::1]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qTp0h-005kPw-07 for linux-arm-kernel@bombadil.infradead.org; Wed, 09 Aug 2023 19:38:23 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=NTKUIRyMkZ6gFM5jOazACfpNT+FP8zSBRsd0kaMgCnI=; b=UlQ1FaJbqGtJRlOQ1TXrwJ6j04 KYpdgQxFgRUjBGqqcBdfEICY1VOu5MSVW0hz8HABcprRIn+qmwc78SYVzhmXq82uEGvXkFcnlaqR9 YHd3JE2tvmJ2OMMzgw6kPsCqiMNhY+S5egNPMBhhRvwB9BgGDEbU0AHN+iVNcu5rZeNz553TR/eSK vNqLmXI4GP2RcxWZ7rKZxvuyHeW1ZORgeohFCgJpI6mLQmUy6jzD74Fv/PJdNdubypnqCzgkX3jK8 bq1sRpfWHNtVg4S3ZWcA3clEmqQnYX54MAtsDPyBTATcZPDeTx9oJ7A/jIv2+LIynutMAHm8k09nV zyqrUt6w==; Received: from willy by casper.infradead.org with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1qTp0a-0082MK-8g; Wed, 09 Aug 2023 19:38:16 +0000 Date: Wed, 9 Aug 2023 20:38:16 +0100 From: Matthew Wilcox To: "Russell King (Oracle)" Cc: Marek Szyprowski , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Andrew Morton , Mike Rapoport Subject: Re: [PATCH] arm: dma-mapping: fix potential endless loop in __dma_page_dev_to_cpu() Message-ID: References: <20230807152657.1692414-1-m.szyprowski@samsung.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Aug 07, 2023 at 11:46:05PM +0100, Russell King (Oracle) wrote: > On Mon, Aug 07, 2023 at 11:14:13PM +0100, Matthew Wilcox wrote: > > On Mon, Aug 07, 2023 at 05:26:57PM +0200, Marek Szyprowski wrote: > > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > > > index 70cb7e63a9a5..02250106e5ed 100644 > > > --- a/arch/arm/mm/dma-mapping.c > > > +++ b/arch/arm/mm/dma-mapping.c > > > @@ -718,7 +718,7 @@ static void __dma_page_dev_to_cpu(struct page *page, unsigned long off, > > > folio = folio_next(folio); > > > } > > > > > > - while (left >= (ssize_t)folio_size(folio)) { > > > + while (left && left >= (ssize_t)folio_size(folio)) { > > > set_bit(PG_dcache_clean, &folio->flags); > > > left -= folio_size(folio); > > > folio = folio_next(folio); > > > > I've been thinking about this and I think this is the right fix for the > > wrong reason. I don't understand how it can produce the failure you > > saw, but we shouldn't be calling folio_next() if left is zero, let alone > > calling folio_size() on it. So I'd rather see this fix: > > > > while (left >= (ssize_t)folio_size(folio)) { > > set_bit(PG_dcache_clean, &folio->flags); > > left -= folio_size(folio); > > + if (!left) > > + break; > > Given that set_bit() involves atomics, wouldn't it be better if this > had been written as: > > while (left >= folio_size(folio)) { > left -= folio_size(folio); > set_bit(PG_dcache_clean, &folio->flags); > if (!left) > break; > > folio = folio_next(folio); > > } > > That likely means that folio_size() will only be evaluated once per > loop rather than twice. I may be wrong though, I didn't check the > generated code. I'd really like it if gcc did notice that folio_size() could be CSE. Unfortunately, I don't think it can. +long rmk(struct folio *folio, long size) +{ + while (size >= folio_size(folio)) { + size -= folio_size(folio); + folio_set_workingset(folio); + if (size < 0) + return size; + folio = folio_next(folio); + } + + return size; +} 000039d4 : 39d4: e92d41f0 push {r4, r5, r6, r7, r8, lr} 39d8: e1a04000 mov r4, r0 39dc: e1a05001 mov r5, r1 39e0: e3a06a01 mov r6, #4096 @ 0x1000 39e4: e3a07020 mov r7, #32 39e8: ea000010 b 3a30 39ec: e5943000 ldr r3, [r4] 39f0: e1a01004 mov r1, r4 39f4: e3a00009 mov r0, #9 39f8: e3130040 tst r3, #64 @ 0x40 39fc: 03a03a01 moveq r3, #4096 @ 0x1000 3a00: 15d43020 ldrbne r3, [r4, #32] 3a04: 11a03316 lslne r3, r6, r3 3a08: e0455003 sub r5, r5, r3 3a0c: ebfffffe bl 0 <_set_bit> 3a0c: R_ARM_CALL _set_bit 3a10: e3550000 cmp r5, #0 3a14: ba00000c blt 3a4c 3a18: e5943000 ldr r3, [r4] 3a1c: e3130040 tst r3, #64 @ 0x40 3a20: 03a03020 moveq r3, #32 3a24: 15d43020 ldrbne r3, [r4, #32] 3a28: 11a03317 lslne r3, r7, r3 3a2c: e0844003 add r4, r4, r3 3a30: e5943000 ldr r3, [r4] 3a34: e3130040 tst r3, #64 @ 0x40 3a38: 03a03a01 moveq r3, #4096 @ 0x1000 3a3c: 15d43020 ldrbne r3, [r4, #32] 3a40: 11a03316 lslne r3, r6, r3 3a44: e1550003 cmp r5, r3 3a48: 2affffe7 bcs 39ec 3a4c: e1a00005 mov r0, r5 3a50: e8bd81f0 pop {r4, r5, r6, r7, r8, pc} Certainly seems to me like it's calculating folio_size() twice. And actually it's redone the ordering to put the calculation after the call to set_bit! > Also, I'm wondering what that ssize_t cast is doing there - "left" > is a size_t, which is unsigned. folio_size() returns a size_t, so > is also unsigned. Why convert folio_size() to a signed number to > then be compared with an unsigned number? Because earlier we did: + if (offset) { + left -= folio_size(folio) - offset; + folio = folio_next(folio); + } so left might now be negative. If we did an unsigned comparison, we'd go round this loop. Er. And the fix from Marek didn't accommodate this problem. So we need a fix-fix: if (offset) { left -= folio_size(folio) - offset; + if (left <= 0) + return; folio = folio_next(folio); } Marek, can you do the honours here? > Or did "left" get converted to ssize_t along with the folio > conversion? > > Even if it did, how could "left" be negative (except through casting > a large positive number as "size" that in 2's complement would be > negative after casting to "left") ? > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel