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 X-Spam-Level: X-Spam-Status: No, score=-12.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 40688C43460 for ; Fri, 16 Apr 2021 09:27:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1B5256113B for ; Fri, 16 Apr 2021 09:27:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240542AbhDPJ2T (ORCPT ); Fri, 16 Apr 2021 05:28:19 -0400 Received: from fllv0015.ext.ti.com ([198.47.19.141]:49772 "EHLO fllv0015.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239117AbhDPJ2R (ORCPT ); Fri, 16 Apr 2021 05:28:17 -0400 Received: from lelv0266.itg.ti.com ([10.180.67.225]) by fllv0015.ext.ti.com (8.15.2/8.15.2) with ESMTP id 13G9QoXq061900; Fri, 16 Apr 2021 04:26:50 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1618565210; bh=iUN+ZAwoZO98VUtAbHnorakjrh71xOPMrXkAMJ4Hqpo=; h=Subject:To:CC:References:From:Date:In-Reply-To; b=q6CHJQ5yHhCUxTvsb7LWnrHojwq4z5fdGhcq05XBA0YlrsKJOlZL44mFXdrMXAW0R wUKB5uUpmeYwcbTk0M0RI9dOZ3CPFoP6cgHnph+X7GsjlxLhWiVAP157Q4A/cGg0Yj nGxzGQi4XUQ1CBnf2HhNuMpn9s6wtsVxUbfTVjAk= Received: from DLEE103.ent.ti.com (dlee103.ent.ti.com [157.170.170.33]) by lelv0266.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 13G9Qotj047995 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 16 Apr 2021 04:26:50 -0500 Received: from DLEE106.ent.ti.com (157.170.170.36) by DLEE103.ent.ti.com (157.170.170.33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2176.2; Fri, 16 Apr 2021 04:26:49 -0500 Received: from fllv0039.itg.ti.com (10.64.41.19) by DLEE106.ent.ti.com (157.170.170.36) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2176.2 via Frontend Transport; Fri, 16 Apr 2021 04:26:49 -0500 Received: from [10.250.100.73] (ileax41-snat.itg.ti.com [10.172.224.153]) by fllv0039.itg.ti.com (8.15.2/8.15.2) with ESMTP id 13G9QjO8101592; Fri, 16 Apr 2021 04:26:46 -0500 Subject: Re: Bogus struct page layout on 32-bit To: Ilias Apalodimas , Jesper Dangaard Brouer , Christoph Hellwig CC: Matthew Wilcox , kernel test robot , Linux-MM , , , open list , , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , , Linux ARM , "David S. Miller" , Matteo Croce , "netdev@vger.kernel.org" References: <20210409185105.188284-3-willy@infradead.org> <202104100656.N7EVvkNZ-lkp@intel.com> <20210410024313.GX2531743@casper.infradead.org> <20210410082158.79ad09a6@carbon> From: Grygorii Strashko Message-ID: Date: Fri, 16 Apr 2021 12:26:44 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ilias, All, On 10/04/2021 11:52, Ilias Apalodimas wrote: > +CC Grygorii for the cpsw part as Ivan's email is not valid anymore > > Thanks for catching this. Interesting indeed... > > On Sat, 10 Apr 2021 at 09:22, Jesper Dangaard Brouer wrote: >> >> On Sat, 10 Apr 2021 03:43:13 +0100 >> Matthew Wilcox wrote: >> >>> On Sat, Apr 10, 2021 at 06:45:35AM +0800, kernel test robot wrote: >>>>>> include/linux/mm_types.h:274:1: error: static_assert failed due to requirement '__builtin_offsetof(struct page, lru) == __builtin_offsetof(struct folio, lru)' "offsetof(struct page, lru) == offsetof(struct folio, lru)" >>>> FOLIO_MATCH(lru, lru); >>>> include/linux/mm_types.h:272:2: note: expanded from macro 'FOLIO_MATCH' >>>> static_assert(offsetof(struct page, pg) == offsetof(struct folio, fl)) >>> >>> Well, this is interesting. pahole reports: >>> >>> struct page { >>> long unsigned int flags; /* 0 4 */ >>> /* XXX 4 bytes hole, try to pack */ >>> union { >>> struct { >>> struct list_head lru; /* 8 8 */ >>> ... >>> struct folio { >>> union { >>> struct { >>> long unsigned int flags; /* 0 4 */ >>> struct list_head lru; /* 4 8 */ >>> >>> so this assert has absolutely done its job. >>> >>> But why has this assert triggered? Why is struct page layout not what >>> we thought it was? Turns out it's the dma_addr added in 2019 by commit >>> c25fff7171be ("mm: add dma_addr_t to struct page"). On this particular >>> config, it's 64-bit, and ppc32 requires alignment to 64-bit. So >>> the whole union gets moved out by 4 bytes. >> >> Argh, good that you are catching this! >> >>> Unfortunately, we can't just fix this by putting an 'unsigned long pad' >>> in front of it. It still aligns the entire union to 8 bytes, and then >>> it skips another 4 bytes after the pad. >>> >>> We can fix it like this ... >>> >>> +++ b/include/linux/mm_types.h >>> @@ -96,11 +96,12 @@ struct page { >>> unsigned long private; >>> }; >>> struct { /* page_pool used by netstack */ >>> + unsigned long _page_pool_pad; >> >> I'm fine with this pad. Matteo is currently proposing[1] to add a 32-bit >> value after @dma_addr, and he could use this area instead. >> >> [1] https://lore.kernel.org/netdev/20210409223801.104657-3-mcroce@linux.microsoft.com/ >> >> When adding/changing this, we need to make sure that it doesn't overlap >> member @index, because network stack use/check page_is_pfmemalloc(). >> As far as my calculations this is safe to add. I always try to keep an >> eye out for this, but I wonder if we could have a build check like yours. >> >> >>> /** >>> * @dma_addr: might require a 64-bit value even on >>> * 32-bit architectures. >>> */ >>> - dma_addr_t dma_addr; >>> + dma_addr_t dma_addr __packed; >>> }; >>> struct { /* slab, slob and slub */ >>> union { >>> >>> but I don't know if GCC is smart enough to realise that dma_addr is now >>> on an 8 byte boundary and it can use a normal instruction to access it, >>> or whether it'll do something daft like use byte loads to access it. >>> >>> We could also do: >>> >>> + dma_addr_t dma_addr __packed __aligned(sizeof(void *)); >>> >>> and I see pahole, at least sees this correctly: >>> >>> struct { >>> long unsigned int _page_pool_pad; /* 4 4 */ >>> dma_addr_t dma_addr __attribute__((__aligned__(4))); /* 8 8 */ >>> } __attribute__((__packed__)) __attribute__((__aligned__(4))); >>> >>> This presumably affects any 32-bit architecture with a 64-bit phys_addr_t >>> / dma_addr_t. Advice, please? >> >> I'm not sure that the 32-bit behavior is with 64-bit (dma) addrs. >> >> I don't have any 32-bit boards with 64-bit DMA. Cc. Ivan, wasn't your >> board (572x ?) 32-bit with driver 'cpsw' this case (where Ivan added >> XDP+page_pool) ? Sry, for delayed reply. The TI platforms am3/4/5 (cpsw) and Keystone 2 (netcp) can do only 32bit DMA even in case of LPAE (dma-ranges are used). Originally, as I remember, CONFIG_ARCH_DMA_ADDR_T_64BIT has not been selected for the LPAE case on TI platforms and the fact that it became set is the result of multi-paltform/allXXXconfig/DMA optimizations and unification. (just checked - not set in 4.14) Probable commit 4965a68780c5 ("arch: define the ARCH_DMA_ADDR_T_64BIT config symbol in lib/Kconfig"). The TI drivers have been updated, finally to accept ARCH_DMA_ADDR_T_64BIT=y by using things like (__force u32) for example. Honestly, I've done sanity check of CPSW with LPAE=y (ARCH_DMA_ADDR_T_64BIT=y) very long time ago. -- Best regards, grygorii 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 X-Spam-Level: X-Spam-Status: No, score=-10.1 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C8E2EC433B4 for ; Sat, 17 Apr 2021 16:40:50 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1DCB460234 for ; Sat, 17 Apr 2021 16:40:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1DCB460234 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=ti.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4FMzNJ4X1Sz3c6C for ; Sun, 18 Apr 2021 02:40:48 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ti.com header.i=@ti.com header.a=rsa-sha256 header.s=ti-com-17Q1 header.b=q6CHJQ5y; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=ti.com (client-ip=198.47.19.141; helo=fllv0015.ext.ti.com; envelope-from=grygorii.strashko@ti.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=ti.com header.i=@ti.com header.a=rsa-sha256 header.s=ti-com-17Q1 header.b=q6CHJQ5y; dkim-atps=neutral X-Greylist: delayed 112366 seconds by postgrey-1.36 at boromir; Sun, 18 Apr 2021 02:40:22 AEST Received: from fllv0015.ext.ti.com (fllv0015.ext.ti.com [198.47.19.141]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4FMzMp5STPz302y for ; Sun, 18 Apr 2021 02:40:21 +1000 (AEST) Received: from lelv0266.itg.ti.com ([10.180.67.225]) by fllv0015.ext.ti.com (8.15.2/8.15.2) with ESMTP id 13G9QoXq061900; Fri, 16 Apr 2021 04:26:50 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1618565210; bh=iUN+ZAwoZO98VUtAbHnorakjrh71xOPMrXkAMJ4Hqpo=; h=Subject:To:CC:References:From:Date:In-Reply-To; b=q6CHJQ5yHhCUxTvsb7LWnrHojwq4z5fdGhcq05XBA0YlrsKJOlZL44mFXdrMXAW0R wUKB5uUpmeYwcbTk0M0RI9dOZ3CPFoP6cgHnph+X7GsjlxLhWiVAP157Q4A/cGg0Yj nGxzGQi4XUQ1CBnf2HhNuMpn9s6wtsVxUbfTVjAk= Received: from DLEE103.ent.ti.com (dlee103.ent.ti.com [157.170.170.33]) by lelv0266.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 13G9Qotj047995 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 16 Apr 2021 04:26:50 -0500 Received: from DLEE106.ent.ti.com (157.170.170.36) by DLEE103.ent.ti.com (157.170.170.33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2176.2; Fri, 16 Apr 2021 04:26:49 -0500 Received: from fllv0039.itg.ti.com (10.64.41.19) by DLEE106.ent.ti.com (157.170.170.36) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2176.2 via Frontend Transport; Fri, 16 Apr 2021 04:26:49 -0500 Received: from [10.250.100.73] (ileax41-snat.itg.ti.com [10.172.224.153]) by fllv0039.itg.ti.com (8.15.2/8.15.2) with ESMTP id 13G9QjO8101592; Fri, 16 Apr 2021 04:26:46 -0500 Subject: Re: Bogus struct page layout on 32-bit To: Ilias Apalodimas , Jesper Dangaard Brouer , Christoph Hellwig References: <20210409185105.188284-3-willy@infradead.org> <202104100656.N7EVvkNZ-lkp@intel.com> <20210410024313.GX2531743@casper.infradead.org> <20210410082158.79ad09a6@carbon> From: Grygorii Strashko Message-ID: Date: Fri, 16 Apr 2021 12:26:44 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kbuild-all@lists.01.org, kernel test robot , clang-built-linux@googlegroups.com, open list , Matthew Wilcox , Linux-MM , "netdev@vger.kernel.org" , Paul Mackerras , linux-fsdevel@vger.kernel.org, Matteo Croce , linuxppc-dev@lists.ozlabs.org, "David S. Miller" , Linux ARM Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" Hi Ilias, All, On 10/04/2021 11:52, Ilias Apalodimas wrote: > +CC Grygorii for the cpsw part as Ivan's email is not valid anymore > > Thanks for catching this. Interesting indeed... > > On Sat, 10 Apr 2021 at 09:22, Jesper Dangaard Brouer wrote: >> >> On Sat, 10 Apr 2021 03:43:13 +0100 >> Matthew Wilcox wrote: >> >>> On Sat, Apr 10, 2021 at 06:45:35AM +0800, kernel test robot wrote: >>>>>> include/linux/mm_types.h:274:1: error: static_assert failed due to requirement '__builtin_offsetof(struct page, lru) == __builtin_offsetof(struct folio, lru)' "offsetof(struct page, lru) == offsetof(struct folio, lru)" >>>> FOLIO_MATCH(lru, lru); >>>> include/linux/mm_types.h:272:2: note: expanded from macro 'FOLIO_MATCH' >>>> static_assert(offsetof(struct page, pg) == offsetof(struct folio, fl)) >>> >>> Well, this is interesting. pahole reports: >>> >>> struct page { >>> long unsigned int flags; /* 0 4 */ >>> /* XXX 4 bytes hole, try to pack */ >>> union { >>> struct { >>> struct list_head lru; /* 8 8 */ >>> ... >>> struct folio { >>> union { >>> struct { >>> long unsigned int flags; /* 0 4 */ >>> struct list_head lru; /* 4 8 */ >>> >>> so this assert has absolutely done its job. >>> >>> But why has this assert triggered? Why is struct page layout not what >>> we thought it was? Turns out it's the dma_addr added in 2019 by commit >>> c25fff7171be ("mm: add dma_addr_t to struct page"). On this particular >>> config, it's 64-bit, and ppc32 requires alignment to 64-bit. So >>> the whole union gets moved out by 4 bytes. >> >> Argh, good that you are catching this! >> >>> Unfortunately, we can't just fix this by putting an 'unsigned long pad' >>> in front of it. It still aligns the entire union to 8 bytes, and then >>> it skips another 4 bytes after the pad. >>> >>> We can fix it like this ... >>> >>> +++ b/include/linux/mm_types.h >>> @@ -96,11 +96,12 @@ struct page { >>> unsigned long private; >>> }; >>> struct { /* page_pool used by netstack */ >>> + unsigned long _page_pool_pad; >> >> I'm fine with this pad. Matteo is currently proposing[1] to add a 32-bit >> value after @dma_addr, and he could use this area instead. >> >> [1] https://lore.kernel.org/netdev/20210409223801.104657-3-mcroce@linux.microsoft.com/ >> >> When adding/changing this, we need to make sure that it doesn't overlap >> member @index, because network stack use/check page_is_pfmemalloc(). >> As far as my calculations this is safe to add. I always try to keep an >> eye out for this, but I wonder if we could have a build check like yours. >> >> >>> /** >>> * @dma_addr: might require a 64-bit value even on >>> * 32-bit architectures. >>> */ >>> - dma_addr_t dma_addr; >>> + dma_addr_t dma_addr __packed; >>> }; >>> struct { /* slab, slob and slub */ >>> union { >>> >>> but I don't know if GCC is smart enough to realise that dma_addr is now >>> on an 8 byte boundary and it can use a normal instruction to access it, >>> or whether it'll do something daft like use byte loads to access it. >>> >>> We could also do: >>> >>> + dma_addr_t dma_addr __packed __aligned(sizeof(void *)); >>> >>> and I see pahole, at least sees this correctly: >>> >>> struct { >>> long unsigned int _page_pool_pad; /* 4 4 */ >>> dma_addr_t dma_addr __attribute__((__aligned__(4))); /* 8 8 */ >>> } __attribute__((__packed__)) __attribute__((__aligned__(4))); >>> >>> This presumably affects any 32-bit architecture with a 64-bit phys_addr_t >>> / dma_addr_t. Advice, please? >> >> I'm not sure that the 32-bit behavior is with 64-bit (dma) addrs. >> >> I don't have any 32-bit boards with 64-bit DMA. Cc. Ivan, wasn't your >> board (572x ?) 32-bit with driver 'cpsw' this case (where Ivan added >> XDP+page_pool) ? Sry, for delayed reply. The TI platforms am3/4/5 (cpsw) and Keystone 2 (netcp) can do only 32bit DMA even in case of LPAE (dma-ranges are used). Originally, as I remember, CONFIG_ARCH_DMA_ADDR_T_64BIT has not been selected for the LPAE case on TI platforms and the fact that it became set is the result of multi-paltform/allXXXconfig/DMA optimizations and unification. (just checked - not set in 4.14) Probable commit 4965a68780c5 ("arch: define the ARCH_DMA_ADDR_T_64BIT config symbol in lib/Kconfig"). The TI drivers have been updated, finally to accept ARCH_DMA_ADDR_T_64BIT=y by using things like (__force u32) for example. Honestly, I've done sanity check of CPSW with LPAE=y (ARCH_DMA_ADDR_T_64BIT=y) very long time ago. -- Best regards, grygorii 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 X-Spam-Level: X-Spam-Status: No, score=-10.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 79C66C433B4 for ; Fri, 16 Apr 2021 09:44:54 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0C5816113B for ; Fri, 16 Apr 2021 09:44:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0C5816113B Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=ti.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:CC:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=SjDrVtAsiQZs+6VXsA3+kvW0k1d1T2qdafYGxJig9ms=; b=TIq2mI0nirgtJ23RtCOfJor5e KOvcgM/MCmMb+Yfu6s33jVN5zx4KrWzL6lnl+UviosLdqbLOH07D4z+ihrLc9Mc29wSPrXHRu7mLA kWmfSavgEolMITPKApM0ccrrW5PCpyqXJhk8oou9oY09W4ELRHSrCeQ+3OCYdiIcR+9tBbhsj4WOI 53g90HUUqYpb/zZy1nvRjIrsrtSaV0CZtgM2o9tZIx9rHh4UyfwwoqQftR7pmUlOdTvsBkG4WLDlj 1IW1jKJXCIuCbEYHbgq1FkrGHMAgzXaaRg9ow0BLNBeC/9eQSVKjStyf7nlcgv0d2q5s9N4gtO/0k Sh+BQ1YLQ==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lXKz6-001aK7-4i; Fri, 16 Apr 2021 09:41:56 +0000 Received: from bombadil.infradead.org ([2607:7c80:54:e::133]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lXKl9-001XyS-KU for linux-arm-kernel@desiato.infradead.org; Fri, 16 Apr 2021 09:27:32 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Content-Transfer-Encoding: Content-Type:In-Reply-To:MIME-Version:Date:Message-ID:From:References:CC:To: Subject:Sender:Reply-To:Content-ID:Content-Description; bh=iUN+ZAwoZO98VUtAbHnorakjrh71xOPMrXkAMJ4Hqpo=; b=bk5gNTWApnVvqsEcEwp7Jkm/eJ KYIbNPXaLi66wVK3dAuESkwfaK64maReiqhkJOQkzqEu5JArimAKYfI4d/tq82JmnjO9GrdHg3Xvx 4+uBs1BH3lXXuYxeSk2C2PFXtUZNVU5BdROeVpkHYovkCIJX4n1ildAL1tXhSUDqUcl5zPmfzNsMs Cmdpjs1xbnSOcGv3FAdSYTzJQgqWdXbIoht/fDjrVNbS4aImQ/zUG44mxnl9dWkd0gdoRWs8JBl4a GPnd7q2BC2xcTr1YGdw2aXFB49ZfwUZravsIWDfcTVExEUPpY7tw4rBaIakZF3pVO+jeVmaebJMrC QUfV2hoA==; Received: from fllv0015.ext.ti.com ([198.47.19.141]) by bombadil.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lXKl6-009Eya-Ju for linux-arm-kernel@lists.infradead.org; Fri, 16 Apr 2021 09:27:30 +0000 Received: from lelv0266.itg.ti.com ([10.180.67.225]) by fllv0015.ext.ti.com (8.15.2/8.15.2) with ESMTP id 13G9QoXq061900; Fri, 16 Apr 2021 04:26:50 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1618565210; bh=iUN+ZAwoZO98VUtAbHnorakjrh71xOPMrXkAMJ4Hqpo=; h=Subject:To:CC:References:From:Date:In-Reply-To; b=q6CHJQ5yHhCUxTvsb7LWnrHojwq4z5fdGhcq05XBA0YlrsKJOlZL44mFXdrMXAW0R wUKB5uUpmeYwcbTk0M0RI9dOZ3CPFoP6cgHnph+X7GsjlxLhWiVAP157Q4A/cGg0Yj nGxzGQi4XUQ1CBnf2HhNuMpn9s6wtsVxUbfTVjAk= Received: from DLEE103.ent.ti.com (dlee103.ent.ti.com [157.170.170.33]) by lelv0266.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 13G9Qotj047995 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 16 Apr 2021 04:26:50 -0500 Received: from DLEE106.ent.ti.com (157.170.170.36) by DLEE103.ent.ti.com (157.170.170.33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2176.2; Fri, 16 Apr 2021 04:26:49 -0500 Received: from fllv0039.itg.ti.com (10.64.41.19) by DLEE106.ent.ti.com (157.170.170.36) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2176.2 via Frontend Transport; Fri, 16 Apr 2021 04:26:49 -0500 Received: from [10.250.100.73] (ileax41-snat.itg.ti.com [10.172.224.153]) by fllv0039.itg.ti.com (8.15.2/8.15.2) with ESMTP id 13G9QjO8101592; Fri, 16 Apr 2021 04:26:46 -0500 Subject: Re: Bogus struct page layout on 32-bit To: Ilias Apalodimas , Jesper Dangaard Brouer , Christoph Hellwig CC: Matthew Wilcox , kernel test robot , Linux-MM , , , open list , , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , , Linux ARM , "David S. Miller" , Matteo Croce , "netdev@vger.kernel.org" References: <20210409185105.188284-3-willy@infradead.org> <202104100656.N7EVvkNZ-lkp@intel.com> <20210410024313.GX2531743@casper.infradead.org> <20210410082158.79ad09a6@carbon> From: Grygorii Strashko Message-ID: Date: Fri, 16 Apr 2021 12:26:44 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210416_022728_748831_217EE002 X-CRM114-Status: GOOD ( 26.51 ) 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Ilias, All, On 10/04/2021 11:52, Ilias Apalodimas wrote: > +CC Grygorii for the cpsw part as Ivan's email is not valid anymore > > Thanks for catching this. Interesting indeed... > > On Sat, 10 Apr 2021 at 09:22, Jesper Dangaard Brouer wrote: >> >> On Sat, 10 Apr 2021 03:43:13 +0100 >> Matthew Wilcox wrote: >> >>> On Sat, Apr 10, 2021 at 06:45:35AM +0800, kernel test robot wrote: >>>>>> include/linux/mm_types.h:274:1: error: static_assert failed due to requirement '__builtin_offsetof(struct page, lru) == __builtin_offsetof(struct folio, lru)' "offsetof(struct page, lru) == offsetof(struct folio, lru)" >>>> FOLIO_MATCH(lru, lru); >>>> include/linux/mm_types.h:272:2: note: expanded from macro 'FOLIO_MATCH' >>>> static_assert(offsetof(struct page, pg) == offsetof(struct folio, fl)) >>> >>> Well, this is interesting. pahole reports: >>> >>> struct page { >>> long unsigned int flags; /* 0 4 */ >>> /* XXX 4 bytes hole, try to pack */ >>> union { >>> struct { >>> struct list_head lru; /* 8 8 */ >>> ... >>> struct folio { >>> union { >>> struct { >>> long unsigned int flags; /* 0 4 */ >>> struct list_head lru; /* 4 8 */ >>> >>> so this assert has absolutely done its job. >>> >>> But why has this assert triggered? Why is struct page layout not what >>> we thought it was? Turns out it's the dma_addr added in 2019 by commit >>> c25fff7171be ("mm: add dma_addr_t to struct page"). On this particular >>> config, it's 64-bit, and ppc32 requires alignment to 64-bit. So >>> the whole union gets moved out by 4 bytes. >> >> Argh, good that you are catching this! >> >>> Unfortunately, we can't just fix this by putting an 'unsigned long pad' >>> in front of it. It still aligns the entire union to 8 bytes, and then >>> it skips another 4 bytes after the pad. >>> >>> We can fix it like this ... >>> >>> +++ b/include/linux/mm_types.h >>> @@ -96,11 +96,12 @@ struct page { >>> unsigned long private; >>> }; >>> struct { /* page_pool used by netstack */ >>> + unsigned long _page_pool_pad; >> >> I'm fine with this pad. Matteo is currently proposing[1] to add a 32-bit >> value after @dma_addr, and he could use this area instead. >> >> [1] https://lore.kernel.org/netdev/20210409223801.104657-3-mcroce@linux.microsoft.com/ >> >> When adding/changing this, we need to make sure that it doesn't overlap >> member @index, because network stack use/check page_is_pfmemalloc(). >> As far as my calculations this is safe to add. I always try to keep an >> eye out for this, but I wonder if we could have a build check like yours. >> >> >>> /** >>> * @dma_addr: might require a 64-bit value even on >>> * 32-bit architectures. >>> */ >>> - dma_addr_t dma_addr; >>> + dma_addr_t dma_addr __packed; >>> }; >>> struct { /* slab, slob and slub */ >>> union { >>> >>> but I don't know if GCC is smart enough to realise that dma_addr is now >>> on an 8 byte boundary and it can use a normal instruction to access it, >>> or whether it'll do something daft like use byte loads to access it. >>> >>> We could also do: >>> >>> + dma_addr_t dma_addr __packed __aligned(sizeof(void *)); >>> >>> and I see pahole, at least sees this correctly: >>> >>> struct { >>> long unsigned int _page_pool_pad; /* 4 4 */ >>> dma_addr_t dma_addr __attribute__((__aligned__(4))); /* 8 8 */ >>> } __attribute__((__packed__)) __attribute__((__aligned__(4))); >>> >>> This presumably affects any 32-bit architecture with a 64-bit phys_addr_t >>> / dma_addr_t. Advice, please? >> >> I'm not sure that the 32-bit behavior is with 64-bit (dma) addrs. >> >> I don't have any 32-bit boards with 64-bit DMA. Cc. Ivan, wasn't your >> board (572x ?) 32-bit with driver 'cpsw' this case (where Ivan added >> XDP+page_pool) ? Sry, for delayed reply. The TI platforms am3/4/5 (cpsw) and Keystone 2 (netcp) can do only 32bit DMA even in case of LPAE (dma-ranges are used). Originally, as I remember, CONFIG_ARCH_DMA_ADDR_T_64BIT has not been selected for the LPAE case on TI platforms and the fact that it became set is the result of multi-paltform/allXXXconfig/DMA optimizations and unification. (just checked - not set in 4.14) Probable commit 4965a68780c5 ("arch: define the ARCH_DMA_ADDR_T_64BIT config symbol in lib/Kconfig"). The TI drivers have been updated, finally to accept ARCH_DMA_ADDR_T_64BIT=y by using things like (__force u32) for example. Honestly, I've done sanity check of CPSW with LPAE=y (ARCH_DMA_ADDR_T_64BIT=y) very long time ago. -- Best regards, grygorii _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============4136611314334098529==" MIME-Version: 1.0 From: Grygorii Strashko To: kbuild-all@lists.01.org Subject: Re: Bogus struct page layout on 32-bit Date: Fri, 16 Apr 2021 12:26:44 +0300 Message-ID: In-Reply-To: List-Id: --===============4136611314334098529== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Hi Ilias, All, On 10/04/2021 11:52, Ilias Apalodimas wrote: > +CC Grygorii for the cpsw part as Ivan's email is not valid anymore > = > Thanks for catching this. Interesting indeed... > = > On Sat, 10 Apr 2021 at 09:22, Jesper Dangaard Brouer wrote: >> >> On Sat, 10 Apr 2021 03:43:13 +0100 >> Matthew Wilcox wrote: >> >>> On Sat, Apr 10, 2021 at 06:45:35AM +0800, kernel test robot wrote: >>>>>> include/linux/mm_types.h:274:1: error: static_assert failed due to r= equirement '__builtin_offsetof(struct page, lru) =3D=3D __builtin_offsetof(= struct folio, lru)' "offsetof(struct page, lru) =3D=3D offsetof(struct foli= o, lru)" >>>> FOLIO_MATCH(lru, lru); >>>> include/linux/mm_types.h:272:2: note: expanded from macro 'FOLIO_M= ATCH' >>>> static_assert(offsetof(struct page, pg) =3D=3D offsetof(st= ruct folio, fl)) >>> >>> Well, this is interesting. pahole reports: >>> >>> struct page { >>> long unsigned int flags; /* 0 = 4 */ >>> /* XXX 4 bytes hole, try to pack */ >>> union { >>> struct { >>> struct list_head lru; /* 8 = 8 */ >>> ... >>> struct folio { >>> union { >>> struct { >>> long unsigned int flags; /* 0 = 4 */ >>> struct list_head lru; /* 4 = 8 */ >>> >>> so this assert has absolutely done its job. >>> >>> But why has this assert triggered? Why is struct page layout not what >>> we thought it was? Turns out it's the dma_addr added in 2019 by commit >>> c25fff7171be ("mm: add dma_addr_t to struct page"). On this particular >>> config, it's 64-bit, and ppc32 requires alignment to 64-bit. So >>> the whole union gets moved out by 4 bytes. >> >> Argh, good that you are catching this! >> >>> Unfortunately, we can't just fix this by putting an 'unsigned long pad' >>> in front of it. It still aligns the entire union to 8 bytes, and then >>> it skips another 4 bytes after the pad. >>> >>> We can fix it like this ... >>> >>> +++ b/include/linux/mm_types.h >>> @@ -96,11 +96,12 @@ struct page { >>> unsigned long private; >>> }; >>> struct { /* page_pool used by netstack */ >>> + unsigned long _page_pool_pad; >> >> I'm fine with this pad. Matteo is currently proposing[1] to add a 32-bit >> value after @dma_addr, and he could use this area instead. >> >> [1] https://lore.kernel.org/netdev/20210409223801.104657-3-mcroce(a)linu= x.microsoft.com/ >> >> When adding/changing this, we need to make sure that it doesn't overlap >> member @index, because network stack use/check page_is_pfmemalloc(). >> As far as my calculations this is safe to add. I always try to keep an >> eye out for this, but I wonder if we could have a build check like yours. >> >> >>> /** >>> * @dma_addr: might require a 64-bit value eve= n on >>> * 32-bit architectures. >>> */ >>> - dma_addr_t dma_addr; >>> + dma_addr_t dma_addr __packed; >>> }; >>> struct { /* slab, slob and slub */ >>> union { >>> >>> but I don't know if GCC is smart enough to realise that dma_addr is now >>> on an 8 byte boundary and it can use a normal instruction to access it, >>> or whether it'll do something daft like use byte loads to access it. >>> >>> We could also do: >>> >>> + dma_addr_t dma_addr __packed __aligned(sizeof(v= oid *)); >>> >>> and I see pahole, at least sees this correctly: >>> >>> struct { >>> long unsigned int _page_pool_pad; /* 4 = 4 */ >>> dma_addr_t dma_addr __attribute__((__aligned__= (4))); /* 8 8 */ >>> } __attribute__((__packed__)) __attribute__((__aligned= __(4))); >>> >>> This presumably affects any 32-bit architecture with a 64-bit phys_addr= _t >>> / dma_addr_t. Advice, please? >> >> I'm not sure that the 32-bit behavior is with 64-bit (dma) addrs. >> >> I don't have any 32-bit boards with 64-bit DMA. Cc. Ivan, wasn't your >> board (572x ?) 32-bit with driver 'cpsw' this case (where Ivan added >> XDP+page_pool) ? Sry, for delayed reply. The TI platforms am3/4/5 (cpsw) and Keystone 2 (netcp) can do only 32bit DM= A even in case of LPAE (dma-ranges are used). Originally, as I remember, CONFIG_ARCH_DMA_ADDR_T_64BIT has not been select= ed for the LPAE case on TI platforms and the fact that it became set is the result of multi-palt= form/allXXXconfig/DMA optimizations and unification. (just checked - not set in 4.14) Probable commit 4965a68780c5 ("arch: define the ARCH_DMA_ADDR_T_64BIT confi= g symbol in lib/Kconfig"). The TI drivers have been updated, finally to accept ARCH_DMA_ADDR_T_64BIT= =3Dy by using things like (__force u32) for example. Honestly, I've done sanity check of CPSW with LPAE=3Dy (ARCH_DMA_ADDR_T_64B= IT=3Dy) very long time ago. -- = Best regards, grygorii --===============4136611314334098529==--