[PATCH iwl-next v1] ixgbevf: ixgbevf_q_vector clean up

[PATCH iwl-next v1] ixgbevf: ixgbevf_q_vector clean up

[Natalia Wochtman]

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@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Natalia Wochtman <natalia.wochtman@intel.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;
 };
 
 /* microsecond values for various ITR rates shifted by 2 to fit itr register
-- 
2.45.2

Re: [PATCH iwl-next v1] ixgbevf: ixgbevf_q_vector clean up

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 4510 bytes --]



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@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Natalia Wochtman <natalia.wochtman@intel.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@intel.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

RE: [Intel-wired-lan] [PATCH iwl-next v1] ixgbevf: ixgbevf_q_vector clean up

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Jacob Keller
> Sent: Wednesday, November 5, 2025 10:03 PM
> To: Wochtman, Natalia <natalia.wochtman@intel.com>; intel-wired-
> lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Fijalkowski, Maciej
> <maciej.fijalkowski@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>
> Subject: Re: [Intel-wired-lan] [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@intel.com>
> > Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> > Signed-off-by: Natalia Wochtman <natalia.wochtman@intel.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 {

Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>