diff mbox series

net/mlx5: declare size of rte_v128u32_t

Message ID 1746457823-11135-1-git-send-email-andremue@linux.microsoft.com (mailing list archive)
State Superseded
Delegated to: Raslan Darawsheh
Headers
Series 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/github-robot: build success github build: passed
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/aws-unit-testing success Unit Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS

Commit Message

Andre Muezerie May 5, 2025, 3:10 p.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.

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

Comments

Maayan Kashani May 25, 2025, 8:47 a.m. UTC | #1 
Hi, Andre,
Thanks for your contribution and patches,
This fix should be handled diff and not by const, 
as the typedef might change in future and the const value will stay and cause a bug.

Regards,
Maayan Kashani

> -----Original Message-----
> From: Andre Muezerie <andremue@linux.microsoft.com>
> Sent: Monday, 5 May 2025 18:10
> To: Dariusz Sosnowski <dsosnowski@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Bing Zhao <bingz@nvidia.com>; Ori Kam
> <orika@nvidia.com>; Suanming Mou <suanmingm@nvidia.com>; Matan
> Azrad <matan@nvidia.com>
> Cc: dev@dpdk.org; Andre Muezerie <andremue@linux.microsoft.com>
> Subject: [PATCH] net/mlx5: declare size of rte_v128u32_t
> 
> External email: Use caution opening links or attachments
> 
> 
> 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.
> 
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> ---
>  drivers/net/mlx5/mlx5_defs.h | 2 ++
>  drivers/net/mlx5/mlx5_rxtx.c | 6 +++---
>  drivers/net/mlx5/mlx5_tx.h   | 2 +-
>  3 files changed, 6 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..b30d620f54 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.c
> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> @@ -32,7 +32,7 @@ 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 +41,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 +50,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.0
Andre Muezerie May 31, 2025, 12:20 a.m. UTC | #2 
On Sun, May 25, 2025 at 08:47:31AM +0000, Maayan Kashani wrote:
> Hi, Andre,
> Thanks for your contribution and patches,
> This fix should be handled diff and not by const, 
> as the typedef might change in future and the const value will stay and cause a bug.
> 
Hi Maayan,

I agree that in general it's better to not hardcode the size of a
structure because as you said, structures can change over time.

This specific structure is defined as

typedef uint32_t rte_v128u32_t __attribute__((vector_size(16), aligned(16)));
	
which uses non-standard __attribute__, which MSVC does not implement.
We could create a MSVC-specific definition, but then we would have the
same problem again: when updating the old definition, this new one
could be missed.

That makes me think that a good approach would be to have a static assert
that ensures that MLX5_SIZEOF_V128U32_T holds the correct value. This would
avoid the need to create the MSVC-specific definition, which has no other
use for now. This should address the concern you raised.

It should also be noted that rte_v128u32_t has a rigid size - its name
implies that it is 128 bits long. If that size ever changed, the name of the
structure would likely be changed as well.

I'll send out the updated patch adding the assert. Let me know what you think
about it.

Regards,
Andre

> Regards,
> Maayan Kashani
> 
> > -----Original Message-----
> > From: Andre Muezerie <andremue@linux.microsoft.com>
> > Sent: Monday, 5 May 2025 18:10
> > To: Dariusz Sosnowski <dsosnowski@nvidia.com>; Slava Ovsiienko
> > <viacheslavo@nvidia.com>; Bing Zhao <bingz@nvidia.com>; Ori Kam
> > <orika@nvidia.com>; Suanming Mou <suanmingm@nvidia.com>; Matan
> > Azrad <matan@nvidia.com>
> > Cc: dev@dpdk.org; Andre Muezerie <andremue@linux.microsoft.com>
> > Subject: [PATCH] net/mlx5: declare size of rte_v128u32_t
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > 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.
> > 
> > Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> > ---
> >  drivers/net/mlx5/mlx5_defs.h | 2 ++
> >  drivers/net/mlx5/mlx5_rxtx.c | 6 +++---
> >  drivers/net/mlx5/mlx5_tx.h   | 2 +-
> >  3 files changed, 6 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..b30d620f54 100644
> > --- a/drivers/net/mlx5/mlx5_rxtx.c
> > +++ b/drivers/net/mlx5/mlx5_rxtx.c
> > @@ -32,7 +32,7 @@ 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 +41,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 +50,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.0
>
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..b30d620f54 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -32,7 +32,7 @@  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 +41,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 +50,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);