Re: [PATCH 1/2] meson/mlx5: Suppress -Wunused-value diagnostic

[Stephen Hemminger]

On Fri, 12 Apr 2024 13:13:16 +0200
Sebastian Brzezinka  wrote:

> From: Alexey Marchuk 
> 
> mlx5 common library checks if several symbols/definitions
> are presented in system header files. If some are not
> presented, they will be enabled by mlx5_glue library.
> The problem appears with clang and '-Werror' - code
> generated by meson is not compiled due to unused variable:
> 
> Code:
> 
> #include 
> int main(void) {
> /* If it's not defined as a macro, try to use as a symbol */
> #ifndef mlx5dv_create_flow_action_packet_reformat
> mlx5dv_create_flow_action_packet_reformat;
> #endif
> return 0;
> }
> Compiler stdout:
> 
> Compiler stderr:
>  
> /hpc/local/work/alexeymar/repo/spdk/dpdk/build-tmp/meson-private/tmp5obnak86/testfile.c:6:17:
>  error: expression result unused [-Werror,-Wunused-value]
> mlx5dv_create_flow_action_packet_reformat;
> ^
> 
> As result, almost all symbols are enabled in mlx5_glue while
> they exist is system headers. As result, we get multiple
> symbols redefenitions when we compile mlx5_common.
> As a solution for this problem we can suppress
> -Wunused-vaurable using pragma
> 
> DPDK 23.11 note:
> Starting with commit below, all cflags are passed to the has_header_symbol().
> (33d6694) build: use C11 standard
> To make sure that the symbol is properly detected, the pedantic flags needs to
> be removed.
> 
> Signed-off-by: Alexey Marchuk 
> Signed-off-by: Sebastian Brzezinka 
> ---
>  drivers/common/mlx5/linux/meson.build | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/common/mlx5/linux/meson.build 
> b/drivers/common/mlx5/linux/meson.build
> index cdee40c553..f3b8e5741b 100644
> --- a/drivers/common/mlx5/linux/meson.build
> +++ b/drivers/common/mlx5/linux/meson.build
> @@ -209,7 +209,11 @@ if  libmtcr_ul_found
>  endif
>  
>  foreach arg:has_sym_args
> -mlx5_config.set(arg[0], cc.has_header_symbol(arg[1], arg[2], 
> dependencies: libs, args: cflags))
> +file_prefix = '#pragma clang diagnostic ignored "-Wunused-value"'
> +cflags += [
> +'-Wno-pedantic',
> +]
> +mlx5_config.set(arg[0], cc.has_header_symbol(arg[1], arg[2], prefix : 
> file_prefix, dependencies: libs, args: cflags))
>  endforeach
>  foreach arg:has_member_args
>  file_prefix = '#include <' + arg[1] + '>'


Sigh, never liked that mlx5 decided to use pedantic unlike other drivers.
And the build process for this driver has become more baroque and unique 
compared
to all the other drivers in DPDK. What value to the user is this complexity?

[PATCH] net: improve vlan header type alignment

[Morten Brørup]

Ethernet packets can be VLAN tagged, i.e. an Ethernet header can have a
VLAN tag (a.k.a. VLAN header) embedded.
Since the Ethernet header is 2 byte aligned, and the VLAN tag is directly
related to the Ethernet header, the VLAN tag is also 2 byte aligned, so
packing the VLAN tag structure is not necessary.

Furthermore, the Ethernet header type is implictly 2 byte aligned, so
removed the superfluous explicit 2 byte alignment.

Added static_asserts to verify the size and alignment of the various
Ethernet types.

Signed-off-by: Morten Brørup 
---
 lib/net/rte_ether.h | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/lib/net/rte_ether.h b/lib/net/rte_ether.h
index 403e84f50b..c9a0b536c3 100644
--- a/lib/net/rte_ether.h
+++ b/lib/net/rte_ether.h
@@ -11,6 +11,8 @@
  * Ethernet Helpers in RTE
  */
 
+#include 
+#include 
 #include 
 #include 
 
@@ -75,6 +77,11 @@ struct __rte_aligned(2) rte_ether_addr {
uint8_t addr_bytes[RTE_ETHER_ADDR_LEN]; /**< Addr bytes in tx order */
 };
 
+static_assert(sizeof(struct rte_ether_addr) == 6,
+   "sizeof(struct rte_ether_addr) == 6");
+static_assert(alignof(struct rte_ether_addr) == 2,
+   "alignof(struct rte_ether_addr) == 2");
+
 #define RTE_ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
 #define RTE_ETHER_GROUP_ADDR  0x01 /**< Multicast or broadcast Eth. address. */
 
@@ -290,12 +297,17 @@ rte_ether_unformat_addr(const char *str, struct 
rte_ether_addr *eth_addr);
  * Ethernet header: Contains the destination address, source address
  * and frame type.
  */
-struct __rte_aligned(2) rte_ether_hdr {
+struct rte_ether_hdr {
struct rte_ether_addr dst_addr; /**< Destination address. */
struct rte_ether_addr src_addr; /**< Source address. */
rte_be16_t ether_type; /**< Frame type. */
 };
 
+static_assert(sizeof(struct rte_ether_hdr) == 14,
+   "sizeof(struct rte_ether_hdr) == 14");
+static_assert(alignof(struct rte_ether_hdr) == 2,
+   "alignof(struct rte_ether_hdr) == 2");
+
 /**
  * Ethernet VLAN Header.
  * Contains the 16-bit VLAN Tag Control Identifier and the Ethernet type
@@ -304,7 +316,12 @@ struct __rte_aligned(2) rte_ether_hdr {
 struct rte_vlan_hdr {
rte_be16_t vlan_tci;  /**< Priority (3) + CFI (1) + Identifier Code 
(12) */
rte_be16_t eth_proto; /**< Ethernet type of encapsulated frame. */
-} __rte_packed;
+};
+
+static_assert(sizeof(struct rte_vlan_hdr) == 4,
+   "sizeof(struct rte_vlan_hdr) == 4");
+static_assert(alignof(struct rte_vlan_hdr) == 2,
+   "alignof(struct rte_vlan_hdr) == 2");
 
 
 
-- 
2.43.0

RE: [PATCH] net: improve vlan header type alignment

[Morten Brørup]

> From: Morten Brørup [mailto:m...@smartsharesystems.com]
> Sent: Sunday, 13 October 2024 10.36
> 
> Ethernet packets can be VLAN tagged, i.e. an Ethernet header can have a
> VLAN tag (a.k.a. VLAN header) embedded.
> Since the Ethernet header is 2 byte aligned, and the VLAN tag is
> directly
> related to the Ethernet header, the VLAN tag is also 2 byte aligned, so
> packing the VLAN tag structure is not necessary.

Stephen, Bruce, you missed the VLAN header type in this patch:
https://git.dpdk.org/dpdk/commit/lib/librte_net/rte_ether.h?id=da5350ef29afd35c1adabe76f60832f3092269ad

This patch completes your work.

> 
> Furthermore, the Ethernet header type is implictly 2 byte aligned, so
> removed the superfluous explicit 2 byte alignment.
> 
> Added static_asserts to verify the size and alignment of the various
> Ethernet types.
> 
> Signed-off-by: Morten Brørup 
> ---
>  lib/net/rte_ether.h | 21 +++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/net/rte_ether.h b/lib/net/rte_ether.h
> index 403e84f50b..c9a0b536c3 100644
> --- a/lib/net/rte_ether.h
> +++ b/lib/net/rte_ether.h
> @@ -11,6 +11,8 @@
>   * Ethernet Helpers in RTE
>   */
> 
> +#include 
> +#include 
>  #include 
>  #include 
> 
> @@ -75,6 +77,11 @@ struct __rte_aligned(2) rte_ether_addr {
>   uint8_t addr_bytes[RTE_ETHER_ADDR_LEN]; /**< Addr bytes in tx
> order */
>  };
> 
> +static_assert(sizeof(struct rte_ether_addr) == 6,
> + "sizeof(struct rte_ether_addr) == 6");
> +static_assert(alignof(struct rte_ether_addr) == 2,
> + "alignof(struct rte_ether_addr) == 2");
> +
>  #define RTE_ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth.
> address. */
>  #define RTE_ETHER_GROUP_ADDR  0x01 /**< Multicast or broadcast Eth.
> address. */
> 
> @@ -290,12 +297,17 @@ rte_ether_unformat_addr(const char *str, struct
> rte_ether_addr *eth_addr);
>   * Ethernet header: Contains the destination address, source address
>   * and frame type.
>   */
> -struct __rte_aligned(2) rte_ether_hdr {
> +struct rte_ether_hdr {
>   struct rte_ether_addr dst_addr; /**< Destination address. */
>   struct rte_ether_addr src_addr; /**< Source address. */
>   rte_be16_t ether_type; /**< Frame type. */
>  };
> 
> +static_assert(sizeof(struct rte_ether_hdr) == 14,
> + "sizeof(struct rte_ether_hdr) == 14");
> +static_assert(alignof(struct rte_ether_hdr) == 2,
> + "alignof(struct rte_ether_hdr) == 2");
> +
>  /**
>   * Ethernet VLAN Header.
>   * Contains the 16-bit VLAN Tag Control Identifier and the Ethernet
> type
> @@ -304,7 +316,12 @@ struct __rte_aligned(2) rte_ether_hdr {
>  struct rte_vlan_hdr {
>   rte_be16_t vlan_tci;  /**< Priority (3) + CFI (1) + Identifier
> Code (12) */
>   rte_be16_t eth_proto; /**< Ethernet type of encapsulated frame.
> */
> -} __rte_packed;
> +};
> +
> +static_assert(sizeof(struct rte_vlan_hdr) == 4,
> + "sizeof(struct rte_vlan_hdr) == 4");
> +static_assert(alignof(struct rte_vlan_hdr) == 2,
> + "alignof(struct rte_vlan_hdr) == 2");
> 
> 
> 
> --
> 2.43.0

[PATCH] bitops: fix issue in parallel atomic tests

[Mattias Rönnblom]

The macros generating the parallel test for atomic test-and-
[set|clear|flip] functions used a 64-bit reference word when assuring
no neighbouring bits were modified, even when generating code for the
32-bit version of the test.

This issue causes spurious test failures on GCC 12.2.0 (the default
compiler on for example Debian 12 "bookworm"), when optimization level
2 or higher are used.

The test failures do not occur with GCC 11, 12.3 and 13.2.

To the author, this looks like a promotion-related compiler bug in GCC
12.2.

Fixes: 35326b61aecb ("bitops: add atomic bit operations in new API")

Signed-off-by: Mattias Rönnblom 
---
 app/test/test_bitops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test/test_bitops.c b/app/test/test_bitops.c
index 4200073ae4..681e984037 100644
--- a/app/test/test_bitops.c
+++ b/app/test/test_bitops.c
@@ -227,7 +227,7 @@ test_bit_atomic_parallel_test_and_modify ## size(void) \
bool expected_value = total_flips % 2; \
TEST_ASSERT(expected_value == rte_bit_test(&word, bit), \
"After %"PRId64" flips, the bit value should be %d", 
total_flips, expected_value); \
-   uint64_t expected_word = 0; \
+   uint ## size ## _t expected_word = 0; \
rte_bit_assign(&expected_word, bit, expected_value); \
TEST_ASSERT(expected_word == word, "Untouched bits have changed 
value"); \
return TEST_SUCCESS; \
@@ -275,7 +275,7 @@ test_bit_atomic_parallel_flip ## size(void) \
bool expected_value = total_flips % 2; \
TEST_ASSERT(expected_value == rte_bit_test(&word, bit), \
"After %"PRId64" flips, the bit value should be %d", 
total_flips, expected_value); \
-   uint64_t expected_word = 0; \
+   uint ## size ## _t expected_word = 0; \
rte_bit_assign(&expected_word, bit, expected_value); \
TEST_ASSERT(expected_word == word, "Untouched bits have changed 
value"); \
return TEST_SUCCESS; \
-- 
2.43.0

RE: [PATCH] bitops: fix issue in parallel atomic tests

[Morten Brørup]

> From: Mattias Rönnblom [mailto:mattias.ronnb...@ericsson.com]
> Sent: Sunday, 13 October 2024 13.57
> 
> The macros generating the parallel test for atomic test-and-
> [set|clear|flip] functions used a 64-bit reference word when assuring
> no neighbouring bits were modified, even when generating code for the
> 32-bit version of the test.
> 
> This issue causes spurious test failures on GCC 12.2.0 (the default
> compiler on for example Debian 12 "bookworm"), when optimization level
> 2 or higher are used.
> 
> The test failures do not occur with GCC 11, 12.3 and 13.2.
> 
> To the author, this looks like a promotion-related compiler bug in GCC
> 12.2.

I am curious about the compiler bug...

Did the bug occur when the most significant bit was set, so it sign related?

Maybe this will reveal something:

TEST_ASSERT(expected_word == word,
  "Untouched bits have changed value, %" PRIx ## size
  " should be %" PRIx64,
  word, expected_word);

> 
> Fixes: 35326b61aecb ("bitops: add atomic bit operations in new API")
> 
> Signed-off-by: Mattias Rönnblom 
> ---

I took a deep look into this.

Regardless of what the compiler bug is,

Reviewed-by: Morten Brørup 

Re: [PATCH v10 0/7] Lcore variables

[Mattias Rönnblom]

On 2024-10-11 16:25, Stephen Hemminger wrote:

On Fri, 11 Oct 2024 10:18:54 +0200
Mattias Rönnblom  wrote:


This patch set introduces a new API  for static
per-lcore id data allocation.

Please refer to the  API documentation for both a
rationale for this new API, and a comparison to the alternatives
available.

The adoption of this API would affect many different DPDK modules, but
the author updated only a few, mostly to serve as examples in this
RFC, and to iron out some, but surely not all, wrinkles in the API.

The question on how to best allocate static per-lcore memory has been
up several times on the dev mailing list, for example in the thread on
"random: use per lcore state" RFC by Stephen Hemminger.

Lcore variables are surely not the answer to all your per-lcore-data
needs, since it only allows for more-or-less static allocation. In the
author's opinion, it does however provide a reasonably simple and
clean and seemingly very much performant solution to a real problem.

Mattias Rönnblom (7):
   eal: add static per-lcore memory allocation facility
   eal: add lcore variable functional tests
   eal: add lcore variable performance test
   random: keep PRNG state in lcore variable
   power: keep per-lcore state in lcore variable
   service: keep per-lcore state in lcore variable
   eal: keep per-lcore power intrinsics state in lcore variable

  MAINTAINERS   |   6 +
  app/test/meson.build  |   2 +
  app/test/test_lcore_var.c | 436 ++
  app/test/test_lcore_var_perf.c| 257 +++
  config/rte_config.h   |   1 +
  doc/api/doxy-api-index.md |   1 +
  .../prog_guide/env_abstraction_layer.rst  |  43 +-
  doc/guides/rel_notes/release_24_11.rst|  14 +
  lib/eal/common/eal_common_lcore_var.c |  85 
  lib/eal/common/meson.build|   1 +
  lib/eal/common/rte_random.c   |  28 +-
  lib/eal/common/rte_service.c  | 117 ++---
  lib/eal/include/meson.build   |   1 +
  lib/eal/include/rte_lcore_var.h   | 389 
  lib/eal/version.map   |   3 +
  lib/eal/x86/rte_power_intrinsics.c|  17 +-
  lib/power/rte_power_pmd_mgmt.c|  35 +-
  17 files changed, 1343 insertions(+), 93 deletions(-)
  create mode 100644 app/test/test_lcore_var.c
  create mode 100644 app/test/test_lcore_var_perf.c
  create mode 100644 lib/eal/common/eal_common_lcore_var.c
  create mode 100644 lib/eal/include/rte_lcore_var.h




Are there any trace points in this code? Would be good to have.


No. Yes, for sure.


Also some optional statistics for telemetry use.


I agree. It could potentially expose some of the internals of the 
implementation, subject to change, but that is a risk that we can take.


Who does the above two and when? Is this something that is required 
before 24.11 (assuming this feature will make it)?



I would presume this is not intended as a hot path API; therefore
it would be ok to always keep statistics.


The allocation functions are expected to be used only in the slowest of 
the slow paths.

Re: [PATCH v8 0/7] Lcore variables

[Mattias Rönnblom]

On 2024-10-11 16:23, Stephen Hemminger wrote:

On Thu, 10 Oct 2024 16:13:42 +0200
Mattias Rönnblom  wrote:


This patch set introduces a new API  for static
per-lcore id data allocation.

Please refer to the  API documentation for both a
rationale for this new API, and a comparison to the alternatives
available.

The adoption of this API would affect many different DPDK modules, but
the author updated only a few, mostly to serve as examples in this
RFC, and to iron out some, but surely not all, wrinkles in the API.

The question on how to best allocate static per-lcore memory has been
up several times on the dev mailing list, for example in the thread on
"random: use per lcore state" RFC by Stephen Hemminger.

Lcore variables are surely not the answer to all your per-lcore-data
needs, since it only allows for more-or-less static allocation. In the
author's opinion, it does however provide a reasonably simple and
clean and seemingly very much performant solution to a real problem.


There should be a mention about whether this storage can be
shared with other threads and processes. Is it like huge page
memory or stack or heap?


Sure. I'll mention the memory is not in huge pages in the API docs.

RE: [PATCH] bitops: fix issue in parallel atomic tests

[Morten Brørup]

> From: Mattias Rönnblom [mailto:hof...@lysator.liu.se]
> Sent: Sunday, 13 October 2024 17.20
> 
> On 2024-10-13 15:37, Morten Brørup wrote:
> >> From: Mattias Rönnblom [mailto:mattias.ronnb...@ericsson.com]
> >> Sent: Sunday, 13 October 2024 13.57
> >>
> >> The macros generating the parallel test for atomic test-and-
> >> [set|clear|flip] functions used a 64-bit reference word when
> assuring
> >> no neighbouring bits were modified, even when generating code for
> the
> >> 32-bit version of the test.
> >>
> >> This issue causes spurious test failures on GCC 12.2.0 (the default
> >> compiler on for example Debian 12 "bookworm"), when optimization
> level
> >> 2 or higher are used.
> >>
> >> The test failures do not occur with GCC 11, 12.3 and 13.2.
> >>
> >> To the author, this looks like a promotion-related compiler bug in
> GCC
> >> 12.2.
> >
> > I am curious about the compiler bug...
> >
> > Did the bug occur when the most significant bit was set, so it sign
> related?
> >
> 
> It seems to happen a lot more often than 1/32 times. Also, all involved
> types are unsigned.

OK. I was speculating that the compiler bug might be treating an unsigned as a 
signed, and somehow sign extending the most significant bit of a negative value 
into the higher bits when converting the type to a bigger type.

> 
> If you set the optimization level to "1" (i.e.,
> __attribute__((optimize("O"))) on the
> test_bit_atomic_parallel_test_and_modify32 function, the test passes on
> 12.2.0.
> 
> > Maybe this will reveal something:
> >
> > TEST_ASSERT(expected_word == word,
> >"Untouched bits have changed value, %" PRIx ## size
> >" should be %" PRIx64,
> >word, expected_word);
> >
> 
> Confusingly enough, the failing assertion is the one prior that
> assertion.

Ahh... I misread your "promotion" suspicion as "type promotion", not 
instruction reordering.

> 
> >>
> >> Fixes: 35326b61aecb ("bitops: add atomic bit operations in new API")
> >>
> >> Signed-off-by: Mattias Rönnblom 
> >> ---
> >
> > I took a deep look into this.
> >
> > Regardless of what the compiler bug is,
> >
> > Reviewed-by: Morten Brørup 
> >
> 
> Thanks.
> 
> I'm far from sure it's a compiler bug. Just look at the base rate: how
> often does the code you just wrote fail because of a bug in your code,
> and how often is the root cause to be found in the compiler or the
> standard libraries.
> 

A strong argument for rootcausing exactly what the specific compiler gets wrong 
when compiling the code triggering the error.

Re: [PATCH] bitops: fix issue in parallel atomic tests

[Mattias Rönnblom]

On 2024-10-13 15:37, Morten Brørup wrote:

From: Mattias Rönnblom [mailto:mattias.ronnb...@ericsson.com]
Sent: Sunday, 13 October 2024 13.57

The macros generating the parallel test for atomic test-and-
[set|clear|flip] functions used a 64-bit reference word when assuring
no neighbouring bits were modified, even when generating code for the
32-bit version of the test.

This issue causes spurious test failures on GCC 12.2.0 (the default
compiler on for example Debian 12 "bookworm"), when optimization level
2 or higher are used.

The test failures do not occur with GCC 11, 12.3 and 13.2.

To the author, this looks like a promotion-related compiler bug in GCC
12.2.


I am curious about the compiler bug...

Did the bug occur when the most significant bit was set, so it sign related?



It seems to happen a lot more often than 1/32 times. Also, all involved 
types are unsigned.


If you set the optimization level to "1" (i.e., 
__attribute__((optimize("O"))) on the 
test_bit_atomic_parallel_test_and_modify32 function, the test passes on 
12.2.0.



Maybe this will reveal something:

TEST_ASSERT(expected_word == word,
   "Untouched bits have changed value, %" PRIx ## size
   " should be %" PRIx64,
   word, expected_word);



Confusingly enough, the failing assertion is the one prior that assertion.



Fixes: 35326b61aecb ("bitops: add atomic bit operations in new API")

Signed-off-by: Mattias Rönnblom 
---


I took a deep look into this.

Regardless of what the compiler bug is,

Reviewed-by: Morten Brørup 



Thanks.

I'm far from sure it's a compiler bug. Just look at the base rate: how 
often does the code you just wrote fail because of a bug in your code, 
and how often is the root cause to be found in the compiler or the 
standard libraries.

Re: [PATCH] net/pcap: use pcap_next_ex to track errors

[Ferruh Yigit]

On 9/5/2024 5:10 PM, Stephen Hemminger wrote:
> Use pcap_next_ex rather than just pcap_next because pcap_next
> always blocks if there is no packets to receive.
> 
> Bugzilla ID: 1526
> Fixes: 4c173302c307 ("pcap: add new driver")
> Cc: sta...@dpdk.org
>
> Reported-by: Ofer Dagan 
> Signed-off-by: Stephen Hemminger 
>

According Bugzilla comment:
Tested-by: Ofer Dagan 

Applied to dpdk-next-net/main, thanks.

Re: [PATCH 0/5] Increase minimum meson version

[zhoumin]

Hi Patrick Robb,

Thank you for your reminding. I have set the meson version to 0.57.0 in 
Loongson lab.


On Wed, Oct 9, 2024 at 2:31PM, Patrick Robb wrote:

Hi Min Zhou,

I think it makes sense for you to set your meson version to the 
minimum version supported for DPDK (so, .57 now I suppose) instead of 
.63. The principle that David described above regarding LTS is also 
true for main. So, for the main and next-* branch testing that 
Loongson lab does, we want our CI testing to verify that no change 
relying on post .57 meson features get merged into main.


On Tue, Oct 8, 2024 at 9:07 PM zhoumin > wrote:


Hi David,

On Tues, Oct 8, 2024 at 8:28AM, David Marchand wrote:
> Hello CI guys,
>
> On Fri, Sep 20, 2024 at 2:57 PM Bruce Richardson
> mailto:bruce.richard...@intel.com>>
wrote:
>> This patchset proposed increasing the minimum meson version to 0.57
>> and makes changes to update our build files appropriately for that
>> change: replacing deprecated functions, removing unnecessary
version
>> checks and taking advantage of some new capabilities.
>>
>> Why 0.57? No one particular reason; it's mainly a conservative
version
>> bump that doesn't have many impacts, but still gives us the minimum
>> updates we need to replace the deprecated get_cross_properties fn
>> and have a few extra features guaranteed available.
>>
>> Bruce Richardson (5):
>>    build: increase minimum meson version to 0.57
>>    build: remove version check on compiler links function
>>    build: remove unnecessary version checks
>>    build: use version file support from meson
>>    build: replace deprecated meson function
>>
>>   .ci/linux-setup.sh                        | 2 +-
>>   config/arm/meson.build                    | 4 ++--
>>   config/meson.build                        | 8 
>>   config/riscv/meson.build                  | 4 ++--
>>   doc/api/meson.build                       | 2 +-
>>   doc/guides/linux_gsg/sys_reqs.rst         | 2 +-
>>   doc/guides/prog_guide/build-sdk-meson.rst | 2 +-
>>   drivers/common/qat/meson.build            | 2 +-
>>   drivers/crypto/ipsec_mb/meson.build       | 2 +-
>>   drivers/event/cnxk/meson.build            | 2 +-
>>   drivers/meson.build                       | 7 ++-
>>   drivers/net/cnxk/meson.build              | 2 +-
>>   lib/meson.build                           | 6 --
>>   meson.build                               | 7 ++-
>>   14 files changed, 20 insertions(+), 32 deletions(-)
> This series can't be merged until the (UNH and LoongArch) CI are
ready
> for such a change.
>
> TL;DR: the meson minimum version is being changed from 0.53.2 to
0.57
> in the current release.
>
> @UNH @Min Zhou
> How long would it take for all CI to be ready for this change?
It's OK for Loongson lab. The meson version for DPDK CI in
Loongson lab
is 0.63.0.
> Important note: if relevant to your CI, testing against LTS branches
> must still be done with the 0.53.2 version, so no change relying on
> post 0.53.2 meson feature gets backported.
>
>

[PATCH 0/4] debug enhancement and bug fix

[Chaoyong He]

This patch series includes two bug fix commits and two debug enhancement
commits.

Huaxing Zhu (2):
  net/nfp: add more logs to debug probe process
  net/nfp: clear errors status of aer after soft reset

Shihong Wang (1):
  net/nfp: do not set the IPv6 flag in transport mode

Zerun Fu (1):
  net/nfp: notify flower firmware about PF speed

 .mailmap  |  1 +
 .../net/nfp/flower/nfp_flower_representor.c   |  3 ++
 drivers/net/nfp/nfp_ethdev.c  | 46 +--
 drivers/net/nfp/nfp_ipsec.c   | 15 +-
 drivers/net/nfp/nfp_net_common.c  |  2 +-
 drivers/net/nfp/nfp_net_common.h  |  2 +
 drivers/net/nfp/nfpcore/nfp_mutex.c   | 17 +--
 drivers/net/nfp/nfpcore/nfp_nsp.c | 15 --
 drivers/net/nfp/nfpcore/nfp_nsp_eth.c |  5 +-
 drivers/net/nfp/nfpcore/nfp_resource.c|  4 +-
 10 files changed, 83 insertions(+), 27 deletions(-)

-- 
2.39.1

[PATCH 3/4] net/nfp: add more logs to debug probe process

[Chaoyong He]

From: Huaxing Zhu 

Add more logs to debug probe process, and modify some log level.
Also add an new entry to the mailmap file.

Signed-off-by: Huaxing Zhu 
Reviewed-by: Long Wu 
Reviewed-by: Peng Zhang 
Reviewed-by: Chaoyong He 
---
 .mailmap   |  1 +
 drivers/net/nfp/nfp_ethdev.c   |  6 +-
 drivers/net/nfp/nfpcore/nfp_mutex.c| 17 +
 drivers/net/nfp/nfpcore/nfp_nsp.c  | 15 +++
 drivers/net/nfp/nfpcore/nfp_nsp_eth.c  |  5 -
 drivers/net/nfp/nfpcore/nfp_resource.c |  4 +++-
 6 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/.mailmap b/.mailmap
index ec12c42dd4..a4439c9020 100644
--- a/.mailmap
+++ b/.mailmap
@@ -561,6 +561,7 @@ Hrvoje Habjanic 
 Huaibin Wang 
 Huanle Han 
 Huawei Xie  
+Huaxing Zhu 
 Huichao Cai 
 Huilong Xu 
 Huisong Li 
diff --git a/drivers/net/nfp/nfp_ethdev.c b/drivers/net/nfp/nfp_ethdev.c
index a199988999..8cbbadb4de 100644
--- a/drivers/net/nfp/nfp_ethdev.c
+++ b/drivers/net/nfp/nfp_ethdev.c
@@ -1297,13 +1297,17 @@ nfp_fw_upload(struct nfp_nsp *nsp,
 static void
 nfp_fw_unload(struct nfp_cpp *cpp)
 {
+   int err;
struct nfp_nsp *nsp;
 
nsp = nfp_nsp_open(cpp);
if (nsp == NULL)
return;
 
-   nfp_nsp_device_soft_reset(nsp);
+   err = nfp_nsp_device_soft_reset(nsp);
+   if (err != 0)
+   PMD_DRV_LOG(WARNING, "Failed to do soft reset when nfp fw 
unload.");
+
nfp_nsp_close(nsp);
 }
 
diff --git a/drivers/net/nfp/nfpcore/nfp_mutex.c 
b/drivers/net/nfp/nfpcore/nfp_mutex.c
index edb78dfdc9..9a4dc1e14e 100644
--- a/drivers/net/nfp/nfpcore/nfp_mutex.c
+++ b/drivers/net/nfp/nfpcore/nfp_mutex.c
@@ -308,11 +308,16 @@ nfp_cpp_mutex_trylock(struct nfp_cpp_mutex *mutex)
 
/* Verify that the lock marker is not damaged */
err = nfp_cpp_readl(cpp, mur, mutex->address + 4, &key);
-   if (err < 0)
+   if (err < 0) {
+   PMD_DRV_LOG(ERR, "Failed to read key.");
return err;
+   }
 
-   if (key != mutex->key)
+   if (key != mutex->key) {
+   PMD_DRV_LOG(ERR, "Key: %x is not same with the mutex: %x.",
+   key, mutex->key);
return -EPERM;
+   }
 
/*
 * Compare against the unlocked state, and if true,
@@ -335,8 +340,10 @@ nfp_cpp_mutex_trylock(struct nfp_cpp_mutex *mutex)
 * atomic, which returns the original value.
 */
err = nfp_cpp_readl(cpp, mus, mutex->address, &tmp);
-   if (err < 0)
+   if (err < 0) {
+   PMD_DRV_LOG(ERR, "Failed to read tmp.");
return err;
+   }
 
/* Was it unlocked? */
if (nfp_mutex_is_unlocked(tmp)) {
@@ -350,8 +357,10 @@ nfp_cpp_mutex_trylock(struct nfp_cpp_mutex *mutex)
 * debug and bookkeeping.
 */
err = nfp_cpp_writel(cpp, muw, mutex->address, value);
-   if (err < 0)
+   if (err < 0) {
+   PMD_DRV_LOG(ERR, "Failed to write value.");
return err;
+   }
 
mutex->depth = 1;
return 0;
diff --git a/drivers/net/nfp/nfpcore/nfp_nsp.c 
b/drivers/net/nfp/nfpcore/nfp_nsp.c
index 2ac39b10b5..32f092eda1 100644
--- a/drivers/net/nfp/nfpcore/nfp_nsp.c
+++ b/drivers/net/nfp/nfpcore/nfp_nsp.c
@@ -252,6 +252,7 @@ nfp_nsp_open(struct nfp_cpp *cpp)
 
state = malloc(sizeof(*state));
if (state == NULL) {
+   PMD_DRV_LOG(ERR, "NSP - failed to malloc name %s", 
NFP_RESOURCE_NSP);
nfp_resource_release(res);
return NULL;
}
@@ -369,8 +370,10 @@ nfp_nsp_command_real(struct nfp_nsp *state,
}
 
err = nfp_cpp_writeq(cpp, nsp_cpp, nsp_buffer, arg->buf);
-   if (err < 0)
+   if (err < 0) {
+   PMD_DRV_LOG(ERR, "CPP write buffer failed. err %d", err);
return err;
+   }
 
err = nfp_cpp_writeq(cpp, nsp_cpp, nsp_command,
FIELD_PREP(NSP_COMMAND_OPTION, arg->option) |
@@ -378,8 +381,10 @@ nfp_nsp_command_real(struct nfp_nsp *state,
FIELD_PREP(NSP_COMMAND_CODE, arg->code) |
FIELD_PREP(NSP_COMMAND_DMA_BUF, arg->dma) |
FIELD_PREP(NSP_COMMAND_START, 1));
-   if (err < 0)
+   if (err < 0) {
+   PMD_DRV_LOG(ERR, "CPP write command failed. err %d", err);
return err;
+   }
 
/* Wait for NSP_COMMAND_START to go to 0 */
err = nfp_nsp_wait_reg(cpp, ®, nsp_cpp, nsp_command,
@@ -400,15 +405,17 @@ nfp_nsp_command_real(struct nfp_nsp *state,
}
 
err = nfp_cpp_readq(cpp, nsp_cpp, nsp_command, &ret_val);
-   if (err < 0)
+   if (err < 0) {
+   PMD_DRV_LOG(ERR, "CPP read return value failed. err %d", err);
return err;
+   }
 
ret_val = FIELD_GET(NSP_COMM

[PATCH 2/4] net/nfp: do not set the IPv6 flag in transport mode

[Chaoyong He]

From: Shihong Wang 

The transport only encapsulates the security protocol header,
does not pay attention to the IP protocol type, and need not
to set the IPv6 flag.

Fixes: 3d21da66c06b ("net/nfp: create security session")
Cc: sta...@dpdk.org

Signed-off-by: Shihong Wang 
Reviewed-by: Long Wu 
Reviewed-by: Peng Zhang 
Reviewed-by: Chaoyong He 
---
 drivers/net/nfp/nfp_ipsec.c | 15 ++-
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/net/nfp/nfp_ipsec.c b/drivers/net/nfp/nfp_ipsec.c
index 647bc2bb6d..89116af1b2 100644
--- a/drivers/net/nfp/nfp_ipsec.c
+++ b/drivers/net/nfp/nfp_ipsec.c
@@ -1056,20 +1056,9 @@ nfp_ipsec_msg_build(struct rte_eth_dev *eth_dev,
 
break;
case RTE_SECURITY_IPSEC_SA_MODE_TRANSPORT:
-   type = conf->ipsec.tunnel.type;
cfg->ctrl_word.mode = NFP_IPSEC_MODE_TRANSPORT;
-   if (type == RTE_SECURITY_IPSEC_TUNNEL_IPV4) {
-   memset(&cfg->src_ip, 0, sizeof(cfg->src_ip));
-   memset(&cfg->dst_ip, 0, sizeof(cfg->dst_ip));
-   cfg->ipv6 = 0;
-   } else if (type == RTE_SECURITY_IPSEC_TUNNEL_IPV6) {
-   memset(&cfg->src_ip, 0, sizeof(cfg->src_ip));
-   memset(&cfg->dst_ip, 0, sizeof(cfg->dst_ip));
-   cfg->ipv6 = 1;
-   } else {
-   PMD_DRV_LOG(ERR, "Unsupported address family!");
-   return -EINVAL;
-   }
+   memset(&cfg->src_ip, 0, sizeof(cfg->src_ip));
+   memset(&cfg->dst_ip, 0, sizeof(cfg->dst_ip));
 
break;
default:
-- 
2.39.1

[PATCH 4/4] net/nfp: clear errors status of aer after soft reset

[Chaoyong He]

From: Huaxing Zhu 

Accessing device memory during soft reset may result in some
errors being recorded in PCIE's AER register, which is normal.
Therefore, after the soft reset is completed, these errors
should be cleared.

Signed-off-by: Huaxing Zhu 
Reviewed-by: Long Wu 
Reviewed-by: Peng Zhang 
Reviewed-by: Chaoyong He 
---
 drivers/net/nfp/nfp_ethdev.c | 40 ++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/net/nfp/nfp_ethdev.c b/drivers/net/nfp/nfp_ethdev.c
index 8cbbadb4de..b16fbe7db7 100644
--- a/drivers/net/nfp/nfp_ethdev.c
+++ b/drivers/net/nfp/nfp_ethdev.c
@@ -1338,9 +1338,37 @@ nfp_fw_check_change(struct nfp_cpp *cpp,
return 0;
 }
 
+static void
+nfp_pcie_reg32_write_clear(struct rte_pci_device *pci_dev,
+   int position)
+{
+   int ret;
+   uint32_t capability;
+
+   ret = rte_pci_read_config(pci_dev, &capability, 4, position);
+   if (ret < 0)
+   capability = 0x;
+
+   (void)rte_pci_write_config(pci_dev, &capability, 4, position);
+}
+
+static void
+nfp_pcie_aer_clear(struct rte_pci_device *pci_dev)
+{
+   int pos;
+
+   pos = rte_pci_find_ext_capability(pci_dev, RTE_PCI_EXT_CAP_ID_ERR);
+   if (pos <= 0)
+   return;
+
+   nfp_pcie_reg32_write_clear(pci_dev, pos + RTE_PCI_ERR_UNCOR_STATUS);
+   nfp_pcie_reg32_write_clear(pci_dev, pos + RTE_PCI_ERR_COR_STATUS);
+}
+
 static int
 nfp_fw_reload(struct nfp_nsp *nsp,
char *fw_name,
+   struct rte_pci_device *pci_dev,
int reset)
 {
int err;
@@ -1357,6 +1385,14 @@ nfp_fw_reload(struct nfp_nsp *nsp,
}
}
 
+   /*
+* Accessing device memory during soft reset may result in some
+* errors being recorded in PCIE's AER register, which is normal.
+* Therefore, after the soft reset is completed, these errors
+* should be cleared.
+*/
+   nfp_pcie_aer_clear(pci_dev);
+
err = nfp_fw_upload(nsp, fw_name);
if (err != 0) {
PMD_DRV_LOG(ERR, "NFP firmware load failed");
@@ -1463,7 +1499,7 @@ nfp_fw_reload_for_single_pf_from_disk(struct nfp_nsp *nsp,
if (!fw_changed)
return 0;
 
-   ret = nfp_fw_reload(nsp, fw_name, reset);
+   ret = nfp_fw_reload(nsp, fw_name, pf_dev->pci_dev, reset);
if (ret != 0)
return ret;
 
@@ -1523,7 +1559,7 @@ nfp_fw_reload_for_multi_pf_from_disk(struct nfp_nsp *nsp,
if (skip_load_fw && !reload_fw)
return 0;
 
-   err = nfp_fw_reload(nsp, fw_name, reset);
+   err = nfp_fw_reload(nsp, fw_name, pf_dev->pci_dev, reset);
if (err != 0)
return err;
 
-- 
2.39.1

[PATCH 1/4] net/nfp: notify flower firmware about PF speed

[Chaoyong He]

From: Zerun Fu 

When using flower firmware, the VF speed is obtained from the
firmware and the firmware get the VF speed from the PF.

But the previous logic does not notify the firmware about PF speed,
and this cause VF speed to be unavailable.

Fix this by add the logic of notify firmware about PF speed.

Fixes: e1124c4f8a45 ("net/nfp: add flower representor framework")
Cc: chaoyong...@corigine.com
Cc: sta...@dpdk.org

Signed-off-by: Zerun Fu 
Reviewed-by: Chaoyong He 
Reviewed-by: Long Wu 
Reviewed-by: Peng Zhang 
---
 drivers/net/nfp/flower/nfp_flower_representor.c | 3 +++
 drivers/net/nfp/nfp_net_common.c| 2 +-
 drivers/net/nfp/nfp_net_common.h| 2 ++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c 
b/drivers/net/nfp/flower/nfp_flower_representor.c
index eb0a02874b..eae6ba39e1 100644
--- a/drivers/net/nfp/flower/nfp_flower_representor.c
+++ b/drivers/net/nfp/flower/nfp_flower_representor.c
@@ -37,6 +37,9 @@ nfp_flower_repr_link_update(struct rte_eth_dev *dev,
 
ret = nfp_net_link_update_common(dev, link, link->link_status);
 
+   if (repr->repr_type == NFP_REPR_TYPE_PF)
+   nfp_net_notify_port_speed(repr->app_fw_flower->pf_hw, link);
+
return ret;
 }
 
diff --git a/drivers/net/nfp/nfp_net_common.c b/drivers/net/nfp/nfp_net_common.c
index b986ed4622..f76d5a6895 100644
--- a/drivers/net/nfp/nfp_net_common.c
+++ b/drivers/net/nfp/nfp_net_common.c
@@ -184,7 +184,7 @@ nfp_net_link_speed_nfp2rte_check(uint32_t speed)
return RTE_ETH_SPEED_NUM_NONE;
 }
 
-static void
+void
 nfp_net_notify_port_speed(struct nfp_net_hw *hw,
struct rte_eth_link *link)
 {
diff --git a/drivers/net/nfp/nfp_net_common.h b/drivers/net/nfp/nfp_net_common.h
index 8429db68f0..d4fe8338b9 100644
--- a/drivers/net/nfp/nfp_net_common.h
+++ b/drivers/net/nfp/nfp_net_common.h
@@ -383,6 +383,8 @@ int nfp_net_vf_config_app_init(struct nfp_net_hw *net_hw,
 bool nfp_net_version_check(struct nfp_hw *hw,
struct nfp_pf_dev *pf_dev);
 void nfp_net_ctrl_bar_size_set(struct nfp_pf_dev *pf_dev);
+void nfp_net_notify_port_speed(struct nfp_net_hw *hw,
+   struct rte_eth_link *link);
 
 #define NFP_PRIV_TO_APP_FW_NIC(app_fw_priv)\
((struct nfp_app_fw_nic *)app_fw_priv)
-- 
2.39.1

Re: [PATCH] net: improve vlan header type alignment

[fengchengwen]

Acked-by: Chengwen Feng 

On 2024/10/13 16:35, Morten Brørup wrote:
> Ethernet packets can be VLAN tagged, i.e. an Ethernet header can have a
> VLAN tag (a.k.a. VLAN header) embedded.
> Since the Ethernet header is 2 byte aligned, and the VLAN tag is directly
> related to the Ethernet header, the VLAN tag is also 2 byte aligned, so
> packing the VLAN tag structure is not necessary.
> 
> Furthermore, the Ethernet header type is implictly 2 byte aligned, so
> removed the superfluous explicit 2 byte alignment.
> 
> Added static_asserts to verify the size and alignment of the various
> Ethernet types.
> 
> Signed-off-by: Morten Brørup 

Re: [PATCH v9 1/7] eal: add static per-lcore memory allocation facility

[Mattias Rönnblom]

On 2024-10-11 10:04, Mattias Rönnblom wrote:

On 2024-10-10 23:24, Thomas Monjalon wrote:





+ *
+ * An lcore variable is not tied to the owning thread's lifetime. It's
+ * available for use by any thread immediately after having been
+ * allocated, and continues to be available throughout the lifetime of
+ * the EAL.
+ *
+ * Lcore variables cannot and need not be freed.


I'm curious about that.
If EAL is closed, and the application continues its life,
then we want all this memory to be cleaned as well.
Do you know rte_eal_cleanup()?


I think the primary reason you would like to free the buffers is to 
avoid false positives from tools like valgrind memcheck (if anyone 
managed to get that working with DPDK).


rte_eal_cleanup() freeing the buffers and resetting the offset would 
make sense. That however would require the buffers to be tracked (e.g., 
as a linked list).




I had a quick look at this. Cleaning up the lcore var buffers is very 
straightforward.


One thing though: the rte_eal_cleanup() documentation says "After this 
call, no DPDK function calls may be made.". rte_eal_init() is a "DPDK 
function call". So DPDK/EAL can never be re-initialized, correct?


Cleaning up lcore var buffers would further cement this design, since 
there will be no way to re-initialize them other than changing the 
 API.


 From a footprint point of view, TLS allocations and static arrays also 
aren't freed by rte_eal_cleanup().