netif_set_xps_queue() before register_netdev(): correct from an API perspective?

netif_set_xps_queue() before register_netdev(): correct from an API perspective?

[Vladimir Oltean]

Hi,

When drivers/net/ethernet/freescale/dpaa2/ fails to probe (including -EPROBE_DEFER),
I see lots of these:

$ cat /sys/kernel/debug/kmemleak
unreferenced object 0xffff042845fbdac0 (size 64):
  comm "kworker/u16:1", pid 56, jiffies 4294893690 (age 859.844s)
  hex dump (first 32 bytes):
    01 00 00 00 14 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 06 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffc754095a7dfc>] slab_post_alloc_hook+0xa4/0x330
    [<ffffc754095a654c>] __kmem_cache_alloc_node+0x23c/0x308
    [<ffffc7540952fec0>] __kmalloc_node+0xc0/0x240
    [<ffffc7540a976934>] __netif_set_xps_queue+0x32c/0xa78
    [<ffffc7540a9771e4>] netif_set_xps_queue+0x44/0x70
    [<ffffc7540a1c3540>] update_xps+0xb0/0x158
    [<ffffc7540a1c0290>] dpaa2_eth_probe+0xd10/0x1368
    [<ffffc75409b1677c>] fsl_mc_driver_probe+0x2c/0x80
    [<ffffc75409eb764c>] really_probe+0x13c/0x2f8
    [<ffffc75409eb666c>] __driver_probe_device+0xac/0x140
    [<ffffc75409eb7340>] driver_probe_device+0x48/0x218
    [<ffffc75409eb71d8>] __device_attach_driver+0x128/0x158
    [<ffffc75409eb33e4>] bus_for_each_drv+0x12c/0x160
    [<ffffc75409eb63f4>] __device_attach+0xcc/0x1a0
    [<ffffc75409eb64e8>] device_initial_probe+0x20/0x38
    [<ffffc75409eb3620>] bus_probe_device+0xa0/0x118

I see that netif_set_xps_queue() allocates dev->xps_maps[], which is
eventually freed by reset_xps_maps() <- ... <- netif_reset_xps_queues_gt()
from a number of call sites, including from unregister_netdevice_many_notify().

This is nice, but dpaa2 (above) and emulex/benet/be_main.c call
netif_set_xps_queue() prior to registering the net device. So no
deregistration event will take place.

How is the memory supposed to be freed in this case?

Re: netif_set_xps_queue() before register_netdev(): correct from an API perspective?

[Alexander Duyck]

On Wed, Aug 2, 2023 at 7:57 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> Hi,
>
> When drivers/net/ethernet/freescale/dpaa2/ fails to probe (including -EPROBE_DEFER),
> I see lots of these:
>
> $ cat /sys/kernel/debug/kmemleak
> unreferenced object 0xffff042845fbdac0 (size 64):
>   comm "kworker/u16:1", pid 56, jiffies 4294893690 (age 859.844s)
>   hex dump (first 32 bytes):
>     01 00 00 00 14 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 06 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<ffffc754095a7dfc>] slab_post_alloc_hook+0xa4/0x330
>     [<ffffc754095a654c>] __kmem_cache_alloc_node+0x23c/0x308
>     [<ffffc7540952fec0>] __kmalloc_node+0xc0/0x240
>     [<ffffc7540a976934>] __netif_set_xps_queue+0x32c/0xa78
>     [<ffffc7540a9771e4>] netif_set_xps_queue+0x44/0x70
>     [<ffffc7540a1c3540>] update_xps+0xb0/0x158
>     [<ffffc7540a1c0290>] dpaa2_eth_probe+0xd10/0x1368
>     [<ffffc75409b1677c>] fsl_mc_driver_probe+0x2c/0x80
>     [<ffffc75409eb764c>] really_probe+0x13c/0x2f8
>     [<ffffc75409eb666c>] __driver_probe_device+0xac/0x140
>     [<ffffc75409eb7340>] driver_probe_device+0x48/0x218
>     [<ffffc75409eb71d8>] __device_attach_driver+0x128/0x158
>     [<ffffc75409eb33e4>] bus_for_each_drv+0x12c/0x160
>     [<ffffc75409eb63f4>] __device_attach+0xcc/0x1a0
>     [<ffffc75409eb64e8>] device_initial_probe+0x20/0x38
>     [<ffffc75409eb3620>] bus_probe_device+0xa0/0x118
>
> I see that netif_set_xps_queue() allocates dev->xps_maps[], which is
> eventually freed by reset_xps_maps() <- ... <- netif_reset_xps_queues_gt()
> from a number of call sites, including from unregister_netdevice_many_notify().
>
> This is nice, but dpaa2 (above) and emulex/benet/be_main.c call
> netif_set_xps_queue() prior to registering the net device. So no
> deregistration event will take place.
>
> How is the memory supposed to be freed in this case?

We really shouldn't be calling it before the netdev is registered.

That said though the easiest way to clean it up would probably be to
call netdev_reset_tc in case of a failure.

Re: netif_set_xps_queue() before register_netdev(): correct from an API perspective?

[Vladimir Oltean]

On Wed, Aug 02, 2023 at 08:04:01AM -0700, Alexander Duyck wrote:
> We really shouldn't be calling it before the netdev is registered.
> 
> That said though the easiest way to clean it up would probably be to
> call netdev_reset_tc in case of a failure.

Thanks. What else will happen that's bad with XPS being configured on
TXQs of unregistered net devices? The call path has existed since 2017 -
commit 93ddf0b211a0 ("staging: fsl-dpaa2/eth: Flow affinity for
non-forwarded traffic").

Re: netif_set_xps_queue() before register_netdev(): correct from an API perspective?

[Alexander Duyck]

On Wed, Aug 2, 2023 at 8:23 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Wed, Aug 02, 2023 at 08:04:01AM -0700, Alexander Duyck wrote:
> > We really shouldn't be calling it before the netdev is registered.
> >
> > That said though the easiest way to clean it up would probably be to
> > call netdev_reset_tc in case of a failure.
>
> Thanks. What else will happen that's bad with XPS being configured on
> TXQs of unregistered net devices? The call path has existed since 2017 -
> commit 93ddf0b211a0 ("staging: fsl-dpaa2/eth: Flow affinity for
> non-forwarded traffic").

The only risk I can think of would be the potential memory leak.

It might make sense to look at adding code to netif_free_tx_queues()
to free the XPS memory there if it hasn't been freed already. That
would resolve the memory leak issue and allow it to be used earlier.