Re: [PATCH net-next v18 07/14] memory-provider: dmabuf devmem memory provider

[Pavel Begunkov]

On 8/11/24 03:21, Mina Almasry wrote:
> On Fri, Aug 9, 2024 at 11:52 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> > On Fri, 9 Aug 2024 16:45:50 +0100 Pavel Begunkov wrote:
> > > > I think this is good, and it doesn't seem hacky to me, because we can
> > > > check the page_pools of the netdev while we hold rtnl, so we can be
> > > > sure nothing is messing with the pp configuration in the meantime.
> > > > Like you say below it does validate the driver rather than rely on the
> > > > driver saying it's doing the right thing. I'll look into putting this
> > > > in the next version.
> > >
> > > Why not have a flag set by the driver and advertising whether it
> > > supports providers or not, which should be checked for instance in
> > > netdev_rx_queue_restart()? If set, the driver should do the right
> > > thing. That's in addition to a new pp_params flag explicitly telling
> > > if pp should use providers. It's more explicit and feels a little
> > > less hacky.
> >
> > You mean like I suggested in the previous two emails? :)
> >
> > Given how easy the check is to implement, I think it's worth
> > adding as a sanity check. But the flag should be the main API,
> > if the sanity check starts to be annoying we'll ditch it.
>
> I think we're talking about 2 slightly different flags, AFAIU.>
> Pavel and I are suggesting the driver reports "I support memory
> providers" directly to core (via the queue-api or what not), and we
> check that flag directly in netdev_rx_queue_restart(), and fail
> immediately if the support is not there.

I might've misread Jakub, but yes, I believe it's different. It'd
communicate about support for providers to upper layers, so we can
fail even before attempting to allocate a new queue and init a
page pool.

> Jakub is suggesting a page_pool_params flag which lets the driver
> report "I support memory providers". If the driver doesn't support it
> but core is trying to configure that, then the page_pool_create will
> fail, which will cause the queue API operation
> (ndo_queue_alloc_mem_alloc) to fail, which causes
> netdev_rx_queue_restart() to fail.

And I'm not against this way either if we explicitly get an error
back instead of trying to figure it out post-factum like by
checking the references and possibly reverting the allocation.
Maybe that's where I was confused, and that refcount thing was
suggested as a WARN_ONCE?

FWIW, I think it warrants two flags. The first saying that the
driver supports providers at all:

page_pool_init() {
	if (rxq->mp_params)
		if (!(flags & PP_PROVIDERS_SUPPORTED))
			goto fail;
}

And the second telling whether the driver wants to install
providers for this particular page pool, so if there is a
separate pool for headers we can set it with plain old kernel
pages.

payload_pool = page_pool_create(rqx, PP_PROVIDERS_SUPPORTED);
header_pool = page_pool_create(rqx, PP_PROVIDERS_SUPPORTED |
                                    PP_IGNORE_PROVIDERS);

(or invert the flag). That's assuming page_pool_params::queue is
a generic thing and we don't want to draw equivalence between
it and memory providers.

--
Pavel Begunkov