netdev - Re: [PATCH net-next 3/3] bnxt_en: Let the page pool manage the DMA mapping

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230731134430.5e7f9960@kernel.org>
Date: Mon, 31 Jul 2023 13:44:30 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Michael Chan <michael.chan@...adcom.com>
Cc: Jesper Dangaard Brouer <hawk@...nel.org>, davem@...emloft.net,
 netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
 gospo@...adcom.com, bpf@...r.kernel.org, somnath.kotur@...adcom.com, Ilias
 Apalodimas <ilias.apalodimas@...aro.org>
Subject: Re: [PATCH net-next 3/3] bnxt_en: Let the page pool manage the DMA
 mapping

On Mon, 31 Jul 2023 13:20:04 -0700 Michael Chan wrote:
> I think I am beginning to understand what the confusion is.  These 32K
> page fragments within the page may not belong to the same (GRO)
> packet.

Right.

> So we cannot dma_sync the whole page at the same time.

I wouldn't phrase it like that.

> Without setting PP_FLAG_DMA_SYNC_DEV, the driver code should be
> something like this:
> 
> mapping = page_pool_get_dma_addr(page) + offset;
> dma_sync_single_for_device(dev, mapping, BNXT_RX_PAGE_SIZE, bp->rx_dir);
> 
> offset may be 0, 32K, etc.
> 
> Since the PP_FLAG_DMA_SYNC_DEV logic is not aware of this offset, we
> actually must do our own dma_sync and not use PP_FLAG_DMA_SYNC_DEV in
> this case.  Does that sound right?

No, no, all I'm saying is that with the current code (in page pool)
you can't be very intelligent about the sync'ing. Every time a page
enters the pool - the whole page should be synced. But that's fine,
it's still better to let page pool do the syncing than trying to
do it manually in the driver (since freshly allocated pages do not 
have to be synced).

I think the confusion comes partially from the fact that the driver
only ever deals with fragments (32k), but internally page pool does
recycling in full pages (64k). And .max_len is part of the recycling
machinery, so to speak, not part of the allocation machinery.

tl;dr just set .max_len = PAGE_SIZE and all will be right.