netdev - Re: [PATCH iwl-next v1] ixgbevf: ixgbevf_q

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <075eca67-e99d-4aa6-bb04-e5146e019913@intel.com>
Date: Wed, 5 Nov 2025 13:02:50 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Natalia Wochtman <natalia.wochtman@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>
CC: <netdev@...r.kernel.org>, <maciej.fijalkowski@...el.com>, Przemek Kitszel
	<przemyslaw.kitszel@...el.com>, Aleksandr Loktionov
	<aleksandr.loktionov@...el.com>
Subject: Re: [PATCH iwl-next v1] ixgbevf: ixgbevf_q_vector clean up



On 11/5/2025 4:21 AM, Natalia Wochtman wrote:
> Flex array should be at the end of the structure and use [] syntax
> 
> Remove unused fields of ixgbevf_q_vector.
> They aren't used since busy poll was moved to core code in commit
> 508aac6dee02 ("ixgbevf: get rid of custom busy polling code").
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
> Signed-off-by: Natalia Wochtman <natalia.wochtman@...el.com>
> ---
>  drivers/net/ethernet/intel/ixgbevf/ixgbevf.h | 18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
> index 039187607e98..516a6fdd23d0 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
> @@ -241,23 +241,7 @@ struct ixgbevf_q_vector {
>  	char name[IFNAMSIZ + 9];
>  
>  	/* for dynamic allocation of rings associated with this q_vector */
> -	struct ixgbevf_ring ring[0] ____cacheline_internodealigned_in_smp;
> -#ifdef CONFIG_NET_RX_BUSY_POLL
> -	unsigned int state;
> -#define IXGBEVF_QV_STATE_IDLE		0
> -#define IXGBEVF_QV_STATE_NAPI		1    /* NAPI owns this QV */
> -#define IXGBEVF_QV_STATE_POLL		2    /* poll owns this QV */
> -#define IXGBEVF_QV_STATE_DISABLED	4    /* QV is disabled */
> -#define IXGBEVF_QV_OWNED	(IXGBEVF_QV_STATE_NAPI | IXGBEVF_QV_STATE_POLL)
> -#define IXGBEVF_QV_LOCKED	(IXGBEVF_QV_OWNED | IXGBEVF_QV_STATE_DISABLED)
> -#define IXGBEVF_QV_STATE_NAPI_YIELD	8    /* NAPI yielded this QV */
> -#define IXGBEVF_QV_STATE_POLL_YIELD	16   /* poll yielded this QV */
> -#define IXGBEVF_QV_YIELD	(IXGBEVF_QV_STATE_NAPI_YIELD | \
> -				 IXGBEVF_QV_STATE_POLL_YIELD)
> -#define IXGBEVF_QV_USER_PEND	(IXGBEVF_QV_STATE_POLL | \
> -				 IXGBEVF_QV_STATE_POLL_YIELD)
> -	spinlock_t lock;
> -#endif /* CONFIG_NET_RX_BUSY_POLL */
> +	struct ixgbevf_ring ring[] ____cacheline_internodealigned_in_smp;
>  };

How would this have ever worked?? Any access to the state or lock fields
would break the ring structure, so any build with
CONFIG_NET_RX_BUSY_POLL the structure layout should break...

Hmm. It looks like the ring value is placed in 21c046e44861 ("ixgbevf:
allocate the rings as part of q_vector")... But the refactor to allocate
the ring as part of the queue vector was done after busy polling...
which was implemented by commit... c777cdfa4e69 ("ixgbevf: implement
CONFIG_NET_RX_BUSY_POLL") which apparently I wrote.. In the words of
Gandalf "I've no memory of this..." Wow. It turns out that all accesses
to state and lock were removed before this refactor.

So the sequence goes like this:

1) implement busy polling in 2013
2) remove custom busy polling in 2017
3) combine q_vector and ring allocation in 2018

Looking at the structure layout I see:

>         /* --- cacheline 10 boundary (640 bytes) --- */
>         struct ixgbevf_ring        ring[0] __attribute__((__aligned__(64))); /*   640     0 */
>         unsigned int               state;                /*   640     4 */
> 
>         /* XXX 4 bytes hole, try to pack */
> 
>         spinlock_t                 lock;                 /*   648    24 */
> 
>         /* size: 704, cachelines: 11, members: 11 */
>         /* sum members: 625, holes: 3, sum holes: 47 */
>         /* padding: 32 */
>         /* member types with holes: 1, total: 1 */
>         /* paddings: 2, sum paddings: 12 */
>         /* forced alignments: 3, forced holes: 2, sum forced holes: 43 */

We do have a bunch of extra bytes at the end since the struct size is
704 instead of 640, which causes a small amount of over allocation when
allocating the q vectors.

We allocate the q_vector in ixgbevf_alloc_q_vector(), but we don't use
struct_size. IMO this fix should also update the logic to use the
appropriate helper macros. Unfortunately, we don't store the actual ring
count in the q_vector struct so I don't think we can benefit from
__counted_by().

Thanks for fixing this mess.

I would mark this as Fixes: 21c046e44861 ("ixgbevf: allocate the rings
as part of q_vector"). But since we do not touch state or lock, there is
no user-visible bug at present, so I guess a net fix isn't warranted.

Either way:

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>

>  
>  /* microsecond values for various ITR rates shifted by 2 to fit itr register



Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)