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: <6553954141762_1245c529423@willemb.c.googlers.com.notmuch>
Date: Tue, 14 Nov 2023 10:41:53 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Yunsheng Lin <linyunsheng@...wei.com>, 
 Mina Almasry <almasrymina@...gle.com>
Cc: Jakub Kicinski <kuba@...nel.org>, 
 davem@...emloft.net, 
 pabeni@...hat.com, 
 netdev@...r.kernel.org, 
 linux-kernel@...r.kernel.org, 
 Willem de Bruijn <willemb@...gle.com>, 
 Kaiyuan Zhang <kaiyuanz@...gle.com>, 
 Jesper Dangaard Brouer <hawk@...nel.org>, 
 Ilias Apalodimas <ilias.apalodimas@...aro.org>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Christian König <christian.koenig@....com>, 
 Jason Gunthorpe <jgg@...dia.com>, 
 Matthew Wilcox <willy@...radead.org>, 
 Linux-MM <linux-mm@...ck.org>
Subject: Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

Yunsheng Lin wrote:
> On 2023/11/14 20:58, Mina Almasry wrote:
> 
> >>
> >> Yes, as above, skb_frags_not_readable() checking is still needed for
> >> kmap() and page_address().
> >>
> >> get_page, put_page related calling is avoided in page_pool_frag_ref()
> >> and napi_pp_put_page() for devmem page as the above checking is true
> >> for devmem page:
> >> (pp_iov->pp_magic & ~0x3UL) == PP_SIGNATURE
> >>
> > 
> > So, devmem needs special handling with if statement for refcounting,
> > even after using struct pages for devmem, which is not allowed (IIUC
> > the dma-buf maintainer).
> 
> It reuses the already existing checking or optimization, that is the point
> of 'mirror struct'.
> 
> > 
> >>>
> >>>> The main difference between this patchset and RFC v3:
> >>>> 1. It reuses the 'struct page' to have more unified handling between
> >>>>    normal page and devmem page for net stack.
> >>>
> >>> This is what was nacked in RFC v1.
> >>>
> >>>> 2. It relies on the page->pp_frag_count to do reference counting.
> >>>>
> >>>
> >>> I don't see you change any of the page_ref_* calls in page_pool.c, for
> >>> example this one:
> >>>
> >>> https://elixir.bootlin.com/linux/latest/source/net/core/page_pool.c#L601
> >>>
> >>> So the reference the page_pool is seeing is actually page->_refcount,
> >>> not page->pp_frag_count? I'm confused here. Is this a bug in the
> >>> patchset?
> >>
> >> page->_refcount is the same as page_pool_iov->_refcount for devmem, which
> >> is ensured by the 'PAGE_POOL_MATCH(_refcount, _refcount);', and
> >> page_pool_iov->_refcount is set to one in mp_dmabuf_devmem_alloc_pages()
> >> by calling 'refcount_set(&ppiov->_refcount, 1)' and always remains as one.
> >>
> >> So the 'page_ref_count(page) == 1' checking is always true for devmem page.
> > 
> > Which, of course, is a bug in the patchset, and it only works because
> > it's a POC for you. devmem pages (which shouldn't exist according to
> > the dma-buf maintainer, IIUC) can't be recycled all the time. See
> > SO_DEVMEM_DONTNEED patch in my RFC and refcounting needed for devmem.
> 
> I am not sure dma-buf maintainer's concern is still there with this patchset.
> 
> Whatever name you calling it for the struct, however you arrange each field
> in the struct, some metadata is always needed for dmabuf to intergrate into
> page pool.
> 
> If the above is true, why not utilize the 'struct page' to have more unified
> handling?

My understanding is that there is a general preference to simplify struct
page, and at the least not move in the other direction by overloading the
struct in new ways.

If using struct page for something that is not memory, there is ZONE_DEVICE.
But using that correctly is non-trivial:

https://lore.kernel.org/all/ZKyZBbKEpmkFkpWV@ziepe.ca/ 

Since all we need is a handle that does not leave the network stack,
a network specific struct like page_pool_iov entirely avoids this issue.
RFC v3 seems like a good simplification over RFC v1 in that regard to me.
I was also pleasantly surprised how minimal the change to the users of
skb_frag_t actually proved to be.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ