On Fri, Aug 9, 2024 at 11:52 PM Jakub Kicinski kuba@kernel.org 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.
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.
Both are fine, I don't see any extremely strong reason to pick one of the other. I prefer Jakub's suggestion, just because it's closer to the page_pool and may be more reusable in the future. I'll err on the side of that unless I hear strong preference to the contrary.
I also think the additional check that Jakub is requesting is easy to implement and unobjectionable. It would let core validate that the driver did actually create the page_pool with the memory provider. I think one of the goals of the queue API was to allow core to do more validation on driver configuration anyway.