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. 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. -- Thanks, Mina
