[PATCH net-next] net: repack struct netdev_queue

[PATCH net-next] net: repack struct netdev_queue

[Jakub Kicinski]

Adding the NAPI pointer to struct netdev_queue made it grow into another
cacheline, even though there was 44 bytes of padding available.

The struct was historically grouped as follows:

    /* read-mostly stuff (align) */
    /* ... random control path fields ... */
    /* write-mostly stuff (align) */
    /* ... 40 byte hole ... */
    /* struct dql (align) */

It seems that people want to add control path fields after
the read only fields. struct dql looks pretty innocent
but it forces its own alignment and nothing indicates that
there is a lot of empty space above it.

Move dql above the xmit_lock. This shifts the empty space
to the end of the struct rather than in the middle of it.
Move two example fields there to set an example.
Hopefully people will now add new fields at the end of
the struct. A lot of the read-only stuff is also control
path-only, but if we move it all we'll have another hole
in the middle.

Before:
	/* size: 384, cachelines: 6, members: 16 */
	/* sum members: 284, holes: 3, sum holes: 100 */

After:
        /* size: 320, cachelines: 5, members: 16 */
        /* sum members: 284, holes: 1, sum holes: 8 */

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
---
 include/linux/netdevice.h | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 0ef3eaa23f4b..614ec5d3d75b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -644,9 +644,6 @@ struct netdev_queue {
 	struct Qdisc __rcu	*qdisc_sleeping;
 #ifdef CONFIG_SYSFS
 	struct kobject		kobj;
-#endif
-#if defined(CONFIG_XPS) && defined(CONFIG_NUMA)
-	int			numa_node;
 #endif
 	unsigned long		tx_maxrate;
 	/*
@@ -660,13 +657,13 @@ struct netdev_queue {
 #ifdef CONFIG_XDP_SOCKETS
 	struct xsk_buff_pool    *pool;
 #endif
-	/* NAPI instance for the queue
-	 * Readers and writers must hold RTNL
-	 */
-	struct napi_struct      *napi;
+
 /*
  * write-mostly part
  */
+#ifdef CONFIG_BQL
+	struct dql		dql;
+#endif
 	spinlock_t		_xmit_lock ____cacheline_aligned_in_smp;
 	int			xmit_lock_owner;
 	/*
@@ -676,8 +673,16 @@ struct netdev_queue {
 
 	unsigned long		state;
 
-#ifdef CONFIG_BQL
-	struct dql		dql;
+/*
+ * slow- / control-path part
+ */
+	/* NAPI instance for the queue
+	 * Readers and writers must hold RTNL
+	 */
+	struct napi_struct	*napi;
+
+#if defined(CONFIG_XPS) && defined(CONFIG_NUMA)
+	int			numa_node;
 #endif
 } ____cacheline_aligned_in_smp;
 
-- 
2.46.0

Re: [PATCH net-next] net: repack struct netdev_queue

[Eric Dumazet]

On Tue, Aug 20, 2024 at 10:51 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Adding the NAPI pointer to struct netdev_queue made it grow into another
> cacheline, even though there was 44 bytes of padding available.
>
> The struct was historically grouped as follows:
>
>     /* read-mostly stuff (align) */
>     /* ... random control path fields ... */
>     /* write-mostly stuff (align) */
>     /* ... 40 byte hole ... */
>     /* struct dql (align) */
>
> It seems that people want to add control path fields after
> the read only fields. struct dql looks pretty innocent
> but it forces its own alignment and nothing indicates that
> there is a lot of empty space above it.
>
> Move dql above the xmit_lock. This shifts the empty space
> to the end of the struct rather than in the middle of it.
> Move two example fields there to set an example.
> Hopefully people will now add new fields at the end of
> the struct. A lot of the read-only stuff is also control
> path-only, but if we move it all we'll have another hole
> in the middle.

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next] net: repack struct netdev_queue

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 20 Aug 2024 13:51:19 -0700 you wrote:
> Adding the NAPI pointer to struct netdev_queue made it grow into another
> cacheline, even though there was 44 bytes of padding available.
> 
> The struct was historically grouped as follows:
> 
>     /* read-mostly stuff (align) */
>     /* ... random control path fields ... */
>     /* write-mostly stuff (align) */
>     /* ... 40 byte hole ... */
>     /* struct dql (align) */
> 
> [...]

Here is the summary with links:
  - [net-next] net: repack struct netdev_queue
    https://git.kernel.org/netdev/net-next/c/74b1e94e94ea

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html