Re: [net-next v1 08/16] memory-provider: dmabuf devmem memory provider

From: Mina Almasry <almasrymina@google.com>
To: Pavel Begunkov <asml.silence@gmail.com>
Cc: "Shailend Chand" <shailend@google.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-arch@vger.kernel.org,
	linux-kselftest@vger.kernel.org, bpf@vger.kernel.org,
	linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
	"David S. Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Paolo Abeni" <pabeni@redhat.com>,
	"Jonathan Corbet" <corbet@lwn.net>,
	"Jeroen de Borst" <jeroendb@google.com>,
	"Praveen Kaligineedi" <pkaligineedi@google.com>,
	"Jesper Dangaard Brouer" <hawk@kernel.org>,
	"Ilias Apalodimas" <ilias.apalodimas@linaro.org>,
	"Arnd Bergmann" <arnd@arndb.de>,
	"David Ahern" <dsahern@kernel.org>,
	"Willem de Bruijn" <willemdebruijn.kernel@gmail.com>,
	"Shuah Khan" <shuah@kernel.org>,
	"Sumit Semwal" <sumit.semwal@linaro.org>,
	"Christian König" <christian.koenig@amd.com>,
	"Yunsheng Lin" <linyunsheng@huawei.com>,
	"Harshitha Ramamurthy" <hramamurthy@google.com>,
	"Shakeel Butt" <shakeelb@google.com>,
	"Willem de Bruijn" <willemb@google.com>,
	"Kaiyuan Zhang" <kaiyuanz@google.com>
Subject: Re: [net-next v1 08/16] memory-provider: dmabuf devmem memory provider
Date: Fri, 8 Dec 2023 15:25:00 -0800	[thread overview]
Message-ID: <CAHS8izNVFx6oHoo7y86P8Di9VCVe8A_n_9UZFkg5Wnt=A=YcNQ@mail.gmail.com> (raw)
In-Reply-To: <b07a4eca-0c3d-4620-9f97-b1d2c76642c2@gmail.com>

On Fri, Dec 8, 2023 at 2:56 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 12/8/23 00:52, Mina Almasry wrote:
...
> > +     if (pool->p.queue)
> > +             binding = READ_ONCE(pool->p.queue->binding);
> > +
> > +     if (binding) {
> > +             pool->mp_ops = &dmabuf_devmem_ops;
> > +             pool->mp_priv = binding;
> > +     }
>
> Hmm, I don't understand why would we replace a nice transparent
> api with page pool relying on a queue having devmem specific
> pointer? It seemed more flexible and cleaner in the last RFC.
>

Jakub requested this change and may chime in, but I suspect it's to
further abstract the devmem changes from driver. In this iteration,
the driver grabs the netdev_rx_queue and passes it to the page_pool,
and any future configurations between the net stack and page_pool can
be passed this way with the driver unbothered.

> > +
> >       if (pool->mp_ops) {
> >               err = pool->mp_ops->init(pool);
> >               if (err) {
> > @@ -1020,3 +1033,77 @@ void page_pool_update_nid(struct page_pool *pool, int new_nid)
> >       }
> >   }
> >   EXPORT_SYMBOL(page_pool_update_nid);
> > +
> > +void __page_pool_iov_free(struct page_pool_iov *ppiov)
> > +{
> > +     if (WARN_ON(ppiov->pp->mp_ops != &dmabuf_devmem_ops))
> > +             return;
> > +
> > +     netdev_free_dmabuf(ppiov);
> > +}
> > +EXPORT_SYMBOL_GPL(__page_pool_iov_free);
>
> I didn't look too deep but I don't think I immediately follow
> the pp refcounting. It increments pages_state_hold_cnt on
> allocation, but IIUC doesn't mark skbs for recycle? Then, they all
> will be put down via page_pool_iov_put_many() bypassing
> page_pool_return_page() and friends. That will call
> netdev_free_dmabuf(), which doesn't bump pages_state_release_cnt.
>
> At least I couldn't make it work with io_uring, and for my purposes,
> I forced all puts to go through page_pool_return_page(), which calls
> the ->release_page callback. The callback will put the reference and
> ask its page pool to account release_cnt. It also gets rid of
> __page_pool_iov_free(), as we'd need to add a hook there for
> customization otherwise.
>
> I didn't care about overhead because the hot path for me is getting
> buffers from a ring, which is somewhat analogous to sock_devmem_dontneed(),
> but done on pp allocations under napi, and it's done separately.
>
> Completely untested with TCP devmem:
>
> https://github.com/isilence/linux/commit/14bd56605183dc80b540999e8058c79ac92ae2d8
>

This was a mistake in the last RFC, which should be fixed in v1. In
the RFC I was not marking the skbs as skb_mark_for_recycle(), so the
unreffing path wasn't as expected.

In this iteration, that should be completely fixed. I suspect since I
just posted this you're actually referring to the issue tested on the
last RFC? Correct me if wrong.

In this iteration, the reffing story:

- memory provider allocs ppiov and returns it to the page pool with
ppiov->refcount == 1.
- The page_pool gives the page to the driver. The driver may
obtain/release references with page_pool_page_[get|put]_many(), but
the driver is likely not doing that unless it's doing its own page
recycling.
- The net stack obtains references via skb_frag_ref() ->
page_pool_page_get_many()
- The net stack drops references via skb_frag_unref() ->
napi_pp_put_page() -> page_pool_return_page() and friends.

Thus, the issue where the unref path was skipping
page_pool_return_page() and friends should be resolved in this
iteration, let me know if you think otherwise, but I think this was an
issue limited to the last RFC.

> > +
> > +/*** "Dmabuf devmem memory provider" ***/
> > +
> > +static int mp_dmabuf_devmem_init(struct page_pool *pool)
> > +{
> > +     struct netdev_dmabuf_binding *binding = pool->mp_priv;
> > +
> > +     if (!binding)
> > +             return -EINVAL;
> > +
> > +     if (!(pool->p.flags & PP_FLAG_DMA_MAP))
> > +             return -EOPNOTSUPP;
> > +
> > +     if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
> > +             return -EOPNOTSUPP;
> > +
> > +     netdev_dmabuf_binding_get(binding);
> > +     return 0;
> > +}
> > +
> > +static struct page *mp_dmabuf_devmem_alloc_pages(struct page_pool *pool,
> > +                                              gfp_t gfp)
> > +{
> > +     struct netdev_dmabuf_binding *binding = pool->mp_priv;
> > +     struct page_pool_iov *ppiov;
> > +
> > +     ppiov = netdev_alloc_dmabuf(binding);
> > +     if (!ppiov)
> > +             return NULL;
> > +
> > +     ppiov->pp = pool;
> > +     pool->pages_state_hold_cnt++;
> > +     trace_page_pool_state_hold(pool, (struct page *)ppiov,
> > +                                pool->pages_state_hold_cnt);
> > +     return (struct page *)((unsigned long)ppiov | PP_IOV);
> > +}
> > +
> > +static void mp_dmabuf_devmem_destroy(struct page_pool *pool)
> > +{
> > +     struct netdev_dmabuf_binding *binding = pool->mp_priv;
> > +
> > +     netdev_dmabuf_binding_put(binding);
> > +}
> > +
> > +static bool mp_dmabuf_devmem_release_page(struct page_pool *pool,
> > +                                       struct page *page)
> > +{
> > +     struct page_pool_iov *ppiov;
> > +
> > +     if (WARN_ON_ONCE(!page_is_page_pool_iov(page)))
> > +             return false;
> > +
> > +     ppiov = page_to_page_pool_iov(page);
> > +     page_pool_iov_put_many(ppiov, 1);
> > +     /* We don't want the page pool put_page()ing our page_pool_iovs. */
> > +     return false;
> > +}
> > +
> > +const struct memory_provider_ops dmabuf_devmem_ops = {
> > +     .init                   = mp_dmabuf_devmem_init,
> > +     .destroy                = mp_dmabuf_devmem_destroy,
> > +     .alloc_pages            = mp_dmabuf_devmem_alloc_pages,
> > +     .release_page           = mp_dmabuf_devmem_release_page,
> > +};
> > +EXPORT_SYMBOL(dmabuf_devmem_ops);
>
> --
> Pavel Begunkov

-- 
Thanks,
Mina