From: Grygorii Strashko <grygorii.strashko@ti.com>
To: Ilias Apalodimas <ilias.apalodimas@linaro.org>,
Jesper Dangaard Brouer <brouer@redhat.com>,
Christoph Hellwig <hch@lst.de>
Cc: Matthew Wilcox <willy@infradead.org>,
kernel test robot <lkp@intel.com>, Linux-MM <linux-mm@kvack.org>,
<kbuild-all@lists.01.org>, <clang-built-linux@googlegroups.com>,
open list <linux-kernel@vger.kernel.org>,
<linux-fsdevel@vger.kernel.org>,
Michael Ellerman <mpe@ellerman.id.au>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
<linuxppc-dev@lists.ozlabs.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
"David S. Miller" <davem@davemloft.net>,
Matteo Croce <mcroce@linux.microsoft.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: Bogus struct page layout on 32-bit
Date: Fri, 16 Apr 2021 12:26:44 +0300 [thread overview]
Message-ID: <ab9f1a6c-4099-2b59-457d-fcc45d2396f4@ti.com> (raw)
In-Reply-To: <CAC_iWjLXZ6-hhvmvee6r4R_N64u-hrnLqE_CSS1nQk+YaMQQnA@mail.gmail.com>
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 <brouer@redhat.com> wrote:
>>
>> On Sat, 10 Apr 2021 03:43:13 +0100
>> Matthew Wilcox <willy@infradead.org> 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
next prev parent reply other threads:[~2021-04-16 9:27 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-09 18:50 [PATCH v7 00/28] Memory Folios Matthew Wilcox (Oracle)
2021-04-09 18:50 ` [PATCH v7 01/28] mm: Optimise nth_page for contiguous memmap Matthew Wilcox (Oracle)
2021-04-12 6:08 ` Christoph Hellwig
2021-04-09 18:50 ` [PATCH v7 04/28] mm/vmstat: Add functions to account folio statistics Matthew Wilcox (Oracle)
2021-04-09 18:50 ` [PATCH v7 05/28] mm/debug: Add VM_BUG_ON_FOLIO and VM_WARN_ON_ONCE_FOLIO Matthew Wilcox (Oracle)
2021-04-09 18:50 ` [PATCH v7 07/28] mm: Add put_folio Matthew Wilcox (Oracle)
2021-04-09 18:50 ` [PATCH v7 08/28] mm: Add get_folio Matthew Wilcox (Oracle)
2021-04-09 18:50 ` [PATCH v7 11/28] mm/filemap: Add folio_index, folio_file_page and folio_contains Matthew Wilcox (Oracle)
2021-04-09 18:50 ` [PATCH v7 12/28] mm/filemap: Add folio_next_index Matthew Wilcox (Oracle)
2021-04-09 18:50 ` [PATCH v7 15/28] mm: Add folio_mapcount Matthew Wilcox (Oracle)
2021-04-09 18:50 ` [PATCH v7 16/28] mm/memcg: Add folio wrappers for various functions Matthew Wilcox (Oracle)
2021-04-09 18:51 ` [PATCH v7 25/28] mm/writeback: Add wait_for_stable_folio Matthew Wilcox (Oracle)
2021-04-09 18:51 ` [PATCH v7 27/28] mm/filemap: Convert wake_up_page_bit to wake_up_folio_bit Matthew Wilcox (Oracle)
[not found] ` <20210409185105.188284-3-willy@infradead.org>
2021-04-09 22:45 ` [PATCH v7 02/28] mm: Introduce struct folio kernel test robot
2021-04-10 2:43 ` Bogus struct page layout on 32-bit Matthew Wilcox
2021-04-10 6:21 ` Jesper Dangaard Brouer
2021-04-10 8:52 ` Ilias Apalodimas
2021-04-10 14:06 ` Matthew Wilcox
2021-04-10 15:54 ` Russell King - ARM Linux admin
2021-04-16 9:26 ` Grygorii Strashko [this message]
2021-04-16 14:10 ` Arnd Bergmann
2021-04-17 13:08 ` David Laight
2021-04-10 14:17 ` David Laight
2021-04-10 19:10 ` Arnd Bergmann
2021-04-11 22:35 ` Matthew Wilcox
2021-04-10 2:51 ` [PATCH v7 02/28] mm: Introduce struct folio kernel test robot
2021-04-16 15:55 ` Matthew Wilcox
2021-04-19 9:06 ` Kirill A. Shutemov
[not found] ` <20210409185105.188284-10-willy@infradead.org>
2021-04-19 13:25 ` [PATCH v7 09/28] mm: Create FolioFlags Peter Zijlstra
2021-04-19 13:55 ` Matthew Wilcox
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ab9f1a6c-4099-2b59-457d-fcc45d2396f4@ti.com \
--to=grygorii.strashko@ti.com \
--cc=benh@kernel.crashing.org \
--cc=brouer@redhat.com \
--cc=clang-built-linux@googlegroups.com \
--cc=davem@davemloft.net \
--cc=hch@lst.de \
--cc=ilias.apalodimas@linaro.org \
--cc=kbuild-all@lists.01.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=lkp@intel.com \
--cc=mcroce@linux.microsoft.com \
--cc=mpe@ellerman.id.au \
--cc=netdev@vger.kernel.org \
--cc=paulus@samba.org \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).