diff mbox series

[v2] net/mlx5: declare size of rte_v128u32_t

Message ID 1748650917-31700-1-git-send-email-andremue@linux.microsoft.com (mailing list archive)
State Superseded
Delegated to: Raslan Darawsheh
Headers
Series [v2] net/mlx5: declare size of rte_v128u32_t |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-compile-amd64-testing pending Testing pending
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-unit-amd64-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS

Commit Message

Andre Muezerie May 31, 2025, 12:21 a.m. UTC 
When compiling with MSVC the error below is hit:

drivers\net\mlx5\mlx5_tx.h(1148): error C2065: 'rte_v128u32_t':
    undeclared identifier

Turns out that with MSVC the data type rte_v128u32_t is not used, but
its size needs to be known. This patch defines a macro to store that
size and replaces instances of sizeof(rte_v128u32_t) with that macro.

A static assert is added to ensure the new macro never diverges from
sizeof(rte_v128u32_t).

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
---
 drivers/net/mlx5/mlx5_defs.h |  2 ++
 drivers/net/mlx5/mlx5_rxtx.c | 11 ++++++++---
 drivers/net/mlx5/mlx5_tx.h   |  2 +-
 3 files changed, 11 insertions(+), 4 deletions(-)

Comments

Dariusz Sosnowski June 4, 2025, 3:28 p.m. UTC | #1 
Hi,

On Fri, May 30, 2025 at 05:21:57PM -0700, Andre Muezerie wrote:
> When compiling with MSVC the error below is hit:
> 
> drivers\net\mlx5\mlx5_tx.h(1148): error C2065: 'rte_v128u32_t':
>     undeclared identifier
> 
> Turns out that with MSVC the data type rte_v128u32_t is not used, but
> its size needs to be known. This patch defines a macro to store that
> size and replaces instances of sizeof(rte_v128u32_t) with that macro.
> 
> A static assert is added to ensure the new macro never diverges from
> sizeof(rte_v128u32_t).
> 
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> ---
>  drivers/net/mlx5/mlx5_defs.h |  2 ++
>  drivers/net/mlx5/mlx5_rxtx.c | 11 ++++++++---
>  drivers/net/mlx5/mlx5_tx.h   |  2 +-
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
> index 9c454983be..c5e2c59309 100644
> --- a/drivers/net/mlx5/mlx5_defs.h
> +++ b/drivers/net/mlx5/mlx5_defs.h
> @@ -196,4 +196,6 @@
>  
>  #define MLX5_CNT_SVC_CYCLE_TIME_DEFAULT 500
>  
> +#define MLX5_SIZEOF_V128U32_T 16
> +
>  #endif /* RTE_PMD_MLX5_DEFS_H_ */
> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
> index 9c075f6a56..3f4942e225 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.c
> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> @@ -28,11 +28,16 @@
>  #include "mlx5_tx.h"
>  
>  /* static asserts */
> +#ifndef RTE_TOOLCHAIN_MSVC
> +static_assert(sizeof(rte_v128u32_t) == MLX5_SIZEOF_V128U32_T,
> +		"MLX5_SIZEOF_V128U32_T must hold sizeof(rte_v128u32_t)");
> +#endif

Thank you for the patch.
This proposal is ok, but I think it can be slightly improved.

The reference to rte_v128u32_t (in code and in names) can be removed, because
the code which relies on MLX5_ESEG_MIN_INLINE_SIZE does not really use
rte_v128u32_t type. The relevant code e.g., in mlx5_tx_eseg_dmin(),
always copies first 2 bytes of the packet (through uint16_t pointer,
these would be first 2 bytes of destination MAC address)
and then does one of the two:

1. Copies following 16 bytes using rte_mov16().
   (Corresponds to first static_assert for MLX5_ESEG_MIN_INLINE_SIZE).
2. If there is a VLAN defined in mbuf, then:
   - the rest of destination MAC address is copied,
   - source MAC address is copied,
   - VLAN is inserted,
   - 2 bytes appearing after VLAN header are copied.
   (Corresponds to 2nd static_assert for MLX5_ESEG_MIN_INLINE_SIZE).

Amount of data copied is inferred from HW arch and packet descriptor
layout, not rte_v128u32_t.

Since rte_mov16() is used in real code and rte_v128u32_t is only used to
carry over the length of the copy, I would propose to:

- Replace instances of sizeof(rte_v128u32_t) with a macro named MLX5_SIZE_MOV16
  to better reflect the real usage in mlx5 PMD.
- Remove static_assert for
  "sizeof(rte_v128u32_t) == MLX5_SIZEOF_V128U32_T".

Would you be able to apply these changes?

> +
>  static_assert(MLX5_CQE_STATUS_HW_OWN < 0, "Must be negative value");
>  static_assert(MLX5_CQE_STATUS_SW_OWN < 0, "Must be negative value");
>  static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
>  		(sizeof(uint16_t) +
> -		 sizeof(rte_v128u32_t)),
> +		MLX5_SIZEOF_V128U32_T),
>  		"invalid Ethernet Segment data size");
>  static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
>  		(sizeof(uint16_t) +
> @@ -41,7 +46,7 @@ static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
>  		"invalid Ethernet Segment data size");
>  static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
>  		(sizeof(uint16_t) +
> -		 sizeof(rte_v128u32_t)),
> +		MLX5_SIZEOF_V128U32_T),
>  		"invalid Ethernet Segment data size");
>  static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
>  		(sizeof(uint16_t) +
> @@ -50,7 +55,7 @@ static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
>  		"invalid Ethernet Segment data size");
>  static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
>  		(sizeof(uint16_t) +
> -		 sizeof(rte_v128u32_t)),
> +		MLX5_SIZEOF_V128U32_T),
>  		"invalid Ethernet Segment data size");
>  static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
>  		(sizeof(uint16_t) +
> diff --git a/drivers/net/mlx5/mlx5_tx.h b/drivers/net/mlx5/mlx5_tx.h
> index 55568c41b1..5647f6a37d 100644
> --- a/drivers/net/mlx5/mlx5_tx.h
> +++ b/drivers/net/mlx5/mlx5_tx.h
> @@ -1145,7 +1145,7 @@ mlx5_tx_eseg_data(struct mlx5_txq_data *__rte_restrict txq,
>  	} else {
>  		/* Fill the gap in the title WQEBB with inline data. */
>  		rte_mov16(pdst, psrc);
> -		psrc += sizeof(rte_v128u32_t);
> +		psrc += MLX5_SIZEOF_V128U32_T;
>  	}
>  	pdst = (uint8_t *)(es + 2);
>  	MLX5_ASSERT(inlen >= MLX5_ESEG_MIN_INLINE_SIZE);
> -- 
> 2.49.0.vfs.0.3
> 

Best regards,
Dariusz Sosnowski
Andre Muezerie July 8, 2025, 3:08 p.m. UTC | #2 
On Wed, Jun 04, 2025 at 05:28:57PM +0200, Dariusz Sosnowski wrote:
> Hi,
> 
> On Fri, May 30, 2025 at 05:21:57PM -0700, Andre Muezerie wrote:
> > When compiling with MSVC the error below is hit:
> > 
> > drivers\net\mlx5\mlx5_tx.h(1148): error C2065: 'rte_v128u32_t':
> >     undeclared identifier
> > 
> > Turns out that with MSVC the data type rte_v128u32_t is not used, but
> > its size needs to be known. This patch defines a macro to store that
> > size and replaces instances of sizeof(rte_v128u32_t) with that macro.
> > 
> > A static assert is added to ensure the new macro never diverges from
> > sizeof(rte_v128u32_t).
> > 
> > Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> > ---
> >  drivers/net/mlx5/mlx5_defs.h |  2 ++
> >  drivers/net/mlx5/mlx5_rxtx.c | 11 ++++++++---
> >  drivers/net/mlx5/mlx5_tx.h   |  2 +-
> >  3 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
> > index 9c454983be..c5e2c59309 100644
> > --- a/drivers/net/mlx5/mlx5_defs.h
> > +++ b/drivers/net/mlx5/mlx5_defs.h
> > @@ -196,4 +196,6 @@
> >  
> >  #define MLX5_CNT_SVC_CYCLE_TIME_DEFAULT 500
> >  
> > +#define MLX5_SIZEOF_V128U32_T 16
> > +
> >  #endif /* RTE_PMD_MLX5_DEFS_H_ */
> > diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
> > index 9c075f6a56..3f4942e225 100644
> > --- a/drivers/net/mlx5/mlx5_rxtx.c
> > +++ b/drivers/net/mlx5/mlx5_rxtx.c
> > @@ -28,11 +28,16 @@
> >  #include "mlx5_tx.h"
> >  
> >  /* static asserts */
> > +#ifndef RTE_TOOLCHAIN_MSVC
> > +static_assert(sizeof(rte_v128u32_t) == MLX5_SIZEOF_V128U32_T,
> > +		"MLX5_SIZEOF_V128U32_T must hold sizeof(rte_v128u32_t)");
> > +#endif
> 
> Thank you for the patch.
> This proposal is ok, but I think it can be slightly improved.
> 
> The reference to rte_v128u32_t (in code and in names) can be removed, because
> the code which relies on MLX5_ESEG_MIN_INLINE_SIZE does not really use
> rte_v128u32_t type. The relevant code e.g., in mlx5_tx_eseg_dmin(),
> always copies first 2 bytes of the packet (through uint16_t pointer,
> these would be first 2 bytes of destination MAC address)
> and then does one of the two:
> 
> 1. Copies following 16 bytes using rte_mov16().
>    (Corresponds to first static_assert for MLX5_ESEG_MIN_INLINE_SIZE).
> 2. If there is a VLAN defined in mbuf, then:
>    - the rest of destination MAC address is copied,
>    - source MAC address is copied,
>    - VLAN is inserted,
>    - 2 bytes appearing after VLAN header are copied.
>    (Corresponds to 2nd static_assert for MLX5_ESEG_MIN_INLINE_SIZE).
> 
> Amount of data copied is inferred from HW arch and packet descriptor
> layout, not rte_v128u32_t.
> 
> Since rte_mov16() is used in real code and rte_v128u32_t is only used to
> carry over the length of the copy, I would propose to:
> 
> - Replace instances of sizeof(rte_v128u32_t) with a macro named MLX5_SIZE_MOV16
>   to better reflect the real usage in mlx5 PMD.
> - Remove static_assert for
>   "sizeof(rte_v128u32_t) == MLX5_SIZEOF_V128U32_T".
> 
> Would you be able to apply these changes?

Yes, I'll send a new patch with the changes suggested.

Thanks,

Andre Muezerie

> 
> > +
> >  static_assert(MLX5_CQE_STATUS_HW_OWN < 0, "Must be negative value");
> >  static_assert(MLX5_CQE_STATUS_SW_OWN < 0, "Must be negative value");
> >  static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> >  		(sizeof(uint16_t) +
> > -		 sizeof(rte_v128u32_t)),
> > +		MLX5_SIZEOF_V128U32_T),
> >  		"invalid Ethernet Segment data size");
> >  static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> >  		(sizeof(uint16_t) +
> > @@ -41,7 +46,7 @@ static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> >  		"invalid Ethernet Segment data size");
> >  static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> >  		(sizeof(uint16_t) +
> > -		 sizeof(rte_v128u32_t)),
> > +		MLX5_SIZEOF_V128U32_T),
> >  		"invalid Ethernet Segment data size");
> >  static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> >  		(sizeof(uint16_t) +
> > @@ -50,7 +55,7 @@ static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> >  		"invalid Ethernet Segment data size");
> >  static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> >  		(sizeof(uint16_t) +
> > -		 sizeof(rte_v128u32_t)),
> > +		MLX5_SIZEOF_V128U32_T),
> >  		"invalid Ethernet Segment data size");
> >  static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
> >  		(sizeof(uint16_t) +
> > diff --git a/drivers/net/mlx5/mlx5_tx.h b/drivers/net/mlx5/mlx5_tx.h
> > index 55568c41b1..5647f6a37d 100644
> > --- a/drivers/net/mlx5/mlx5_tx.h
> > +++ b/drivers/net/mlx5/mlx5_tx.h
> > @@ -1145,7 +1145,7 @@ mlx5_tx_eseg_data(struct mlx5_txq_data *__rte_restrict txq,
> >  	} else {
> >  		/* Fill the gap in the title WQEBB with inline data. */
> >  		rte_mov16(pdst, psrc);
> > -		psrc += sizeof(rte_v128u32_t);
> > +		psrc += MLX5_SIZEOF_V128U32_T;
> >  	}
> >  	pdst = (uint8_t *)(es + 2);
> >  	MLX5_ASSERT(inlen >= MLX5_ESEG_MIN_INLINE_SIZE);
> > -- 
> > 2.49.0.vfs.0.3
> > 
> 
> Best regards,
> Dariusz Sosnowski
diff mbox series

Patch

diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
index 9c454983be..c5e2c59309 100644
--- a/drivers/net/mlx5/mlx5_defs.h
+++ b/drivers/net/mlx5/mlx5_defs.h
@@ -196,4 +196,6 @@ 
 
 #define MLX5_CNT_SVC_CYCLE_TIME_DEFAULT 500
 
+#define MLX5_SIZEOF_V128U32_T 16
+
 #endif /* RTE_PMD_MLX5_DEFS_H_ */
diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index 9c075f6a56..3f4942e225 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -28,11 +28,16 @@ 
 #include "mlx5_tx.h"
 
 /* static asserts */
+#ifndef RTE_TOOLCHAIN_MSVC
+static_assert(sizeof(rte_v128u32_t) == MLX5_SIZEOF_V128U32_T,
+		"MLX5_SIZEOF_V128U32_T must hold sizeof(rte_v128u32_t)");
+#endif
+
 static_assert(MLX5_CQE_STATUS_HW_OWN < 0, "Must be negative value");
 static_assert(MLX5_CQE_STATUS_SW_OWN < 0, "Must be negative value");
 static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
 		(sizeof(uint16_t) +
-		 sizeof(rte_v128u32_t)),
+		MLX5_SIZEOF_V128U32_T),
 		"invalid Ethernet Segment data size");
 static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
 		(sizeof(uint16_t) +
@@ -41,7 +46,7 @@  static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
 		"invalid Ethernet Segment data size");
 static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
 		(sizeof(uint16_t) +
-		 sizeof(rte_v128u32_t)),
+		MLX5_SIZEOF_V128U32_T),
 		"invalid Ethernet Segment data size");
 static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
 		(sizeof(uint16_t) +
@@ -50,7 +55,7 @@  static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
 		"invalid Ethernet Segment data size");
 static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
 		(sizeof(uint16_t) +
-		 sizeof(rte_v128u32_t)),
+		MLX5_SIZEOF_V128U32_T),
 		"invalid Ethernet Segment data size");
 static_assert(MLX5_ESEG_MIN_INLINE_SIZE ==
 		(sizeof(uint16_t) +
diff --git a/drivers/net/mlx5/mlx5_tx.h b/drivers/net/mlx5/mlx5_tx.h
index 55568c41b1..5647f6a37d 100644
--- a/drivers/net/mlx5/mlx5_tx.h
+++ b/drivers/net/mlx5/mlx5_tx.h
@@ -1145,7 +1145,7 @@  mlx5_tx_eseg_data(struct mlx5_txq_data *__rte_restrict txq,
 	} else {
 		/* Fill the gap in the title WQEBB with inline data. */
 		rte_mov16(pdst, psrc);
-		psrc += sizeof(rte_v128u32_t);
+		psrc += MLX5_SIZEOF_V128U32_T;
 	}
 	pdst = (uint8_t *)(es + 2);
 	MLX5_ASSERT(inlen >= MLX5_ESEG_MIN_INLINE_SIZE);