[PATCH] net/txgbe: fix a mass of interrupts
Since firmware version 0x20010, GPIO interrupt enable is set to 0xd by
default, which means enable bit 0 'tx_fault'. And GPIO interrupt polarity
is set to 0xd by default too, which means these interrupts are rising-edge
sensitive.
So when unplug the SFP module, GPIO line 0 'tx_fault' is 0 -> 1 triggers
the interrupt. However, the interrupt is not cleared. And GPIO interrupt
mask is enabled and disabled to trigger the MISC interrupt repeatedly.
Since this 'tx_fault' interrupt does not make much sense, simply clear it
to fix the issue.
Signed-off-by: Jiawen Wu
---
drivers/net/txgbe/txgbe_ethdev.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/net/txgbe/txgbe_ethdev.c b/drivers/net/txgbe/txgbe_ethdev.c
index a956216abb..ea9faba2c0 100644
--- a/drivers/net/txgbe/txgbe_ethdev.c
+++ b/drivers/net/txgbe/txgbe_ethdev.c
@@ -1555,6 +1555,9 @@ static void txgbe_reinit_gpio_intr(struct txgbe_hw *hw)
wr32(hw, TXGBE_GPIOINTMASK, 0xFF);
reg = rd32(hw, TXGBE_GPIORAWINTSTAT);
+ if (reg & TXGBE_GPIOBIT_0)
+ wr32(hw, TXGBE_GPIOEOI, TXGBE_GPIOBIT_0);
+
if (reg & TXGBE_GPIOBIT_2)
wr32(hw, TXGBE_GPIOEOI, TXGBE_GPIOBIT_2);
@@ -2796,6 +2799,8 @@ txgbe_dev_sfp_event(struct rte_eth_dev *dev)
wr32(hw, TXGBE_GPIOINTMASK, 0xFF);
reg = rd32(hw, TXGBE_GPIORAWINTSTAT);
+ if (reg & TXGBE_GPIOBIT_0)
+ wr32(hw, TXGBE_GPIOEOI, TXGBE_GPIOBIT_0);
if (reg & TXGBE_GPIOBIT_2) {
wr32(hw, TXGBE_GPIOEOI, TXGBE_GPIOBIT_2);
rte_eal_alarm_set(1000 * 100, txgbe_dev_detect_sfp, dev);
--
2.27.0
Re: [PATCH v2 01/10] app/test: do not duplicated loop variable
On Thu, Nov 14, 2024 at 11:24:59AM -0800, Stephen Hemminger wrote:
> Do not use same variable for outer and inner loop in bonding test.
> Since the loop is just freeing the resulting burst use bulk free.
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: 92073ef961ee ("bond: unit tests")
> Cc: declan.dohe...@intel.com
> Cc: sta...@dpdk.org
>
> Signed-off-by: Stephen Hemminger
Acked-by: Bruce Richardson
Re: [PATCH v2 03/10] app/test: fix paren typo
On Thu, Nov 14, 2024 at 11:25:01AM -0800, Stephen Hemminger wrote:
> The parenthesis were in the wrong place so that comparison
> took precedence over assignment in handling IPv6 extension
> headers. Break up the loop condition to avoid the problem.
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: 15ccc647526e ("test/security: test inline reassembly with
> multi-segment")
> Cc: ndabilpu...@marvell.com
> Cc: sta...@dpdk.org
>
> Signed-off-by: Stephen Hemminger
Acked-by: Bruce Richardson
Re: [PATCH v2 04/10] app/test: avoid duplicate initialization
On Thu, Nov 14, 2024 at 11:25:02AM -0800, Stephen Hemminger wrote:
> The event_dev_config initialization had duplicate assignments
> to the same element. Change to use structure initialization
> so that compiler will catch this type of bug.
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: f8f9d233ea0e ("test/eventdev: add unit tests")
> Cc: jerin.ja...@caviumnetworks.com
>
> Signed-off-by: Stephen Hemminger
Acked-by: Bruce Richardson
Re: [PATCH v2 06/10] app/test: fix operator precedence bug
On Thu, Nov 14, 2024 at 11:25:04AM -0800, Stephen Hemminger wrote:
> The test loop was much shorter than desired because when
> MAX_NUM is defined with out paren's the divide operator /
> takes precedence over shift.
>
> But when MAX_NUM is fixed, some tests take too long
> and have to modified to avoid running over full N^2
> space of 1<<20.
>
> Note: this is a very old bug, goes back to 2013.
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: 1fb8b07ee511 ("app: add some tests")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Stephen Hemminger
Acked-by: Bruce Richardson
Re: [PATCH v2 07/10] test/eal: fix core check in c flag test
On Thu, Nov 14, 2024 at 11:25:05AM -0800, Stephen Hemminger wrote:
> The expression for checking which lcore is enabled for 0-7
> was wrong (missing case for 6).
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: b0209034f2bb ("test/eal: check number of cores before running
> subtests")
> Cc: msant...@redhat.com
> Cc: sta...@dpdk.org
>
> Signed-off-by: Stephen Hemminger
Just wondering would it not be better/safer to put in an actual loop check
here?
However, I'm also ok with keeping the fix as-is, so:
Acked-by: Bruce Richardson
> ---
> app/test/test_eal_flags.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> index d37d6b8627..e32f83d3c8 100644
> --- a/app/test/test_eal_flags.c
> +++ b/app/test/test_eal_flags.c
> @@ -677,8 +677,8 @@ test_missing_c_flag(void)
>
> if (rte_lcore_is_enabled(0) && rte_lcore_is_enabled(1) &&
> rte_lcore_is_enabled(2) && rte_lcore_is_enabled(3) &&
> - rte_lcore_is_enabled(3) && rte_lcore_is_enabled(5) &&
> - rte_lcore_is_enabled(4) && rte_lcore_is_enabled(7) &&
> + rte_lcore_is_enabled(4) && rte_lcore_is_enabled(5) &&
> + rte_lcore_is_enabled(6) && rte_lcore_is_enabled(7) &&
> launch_proc(argv29) != 0) {
> printf("Error - "
> "process did not run ok with valid corelist value\n");
> --
> 2.45.2
>
Re: [PATCH v2 08/10] app/test-pmd: remove redundant condition
On Thu, Nov 14, 2024 at 11:25:06AM -0800, Stephen Hemminger wrote:
> The loop over policy actions will always exit when it sees
> the flow end action, so the next check is redundant.
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: f29fa2c59b85 ("app/testpmd: support policy actions per color")
> Cc: haif...@nvidia.com
> Signed-off-by: Stephen Hemminger
> ---
Acked-by: Bruce Richardson
Re: [PATCH v2 10/10] app/test-dma-perf: fix parsing of DMA address
On Thu, Nov 14, 2024 at 11:25:08AM -0800, Stephen Hemminger wrote:
> There was useless loop when looking at the DMA address.
> It looks like it was meant to skip whitespace before
> calling strtok.
>
> Good time to replace strtok with strtok_r as well.
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: 623dc9364dc6 ("app/dma-perf: introduce DMA performance test")
> Cc: cheng1.ji...@intel.com
> Cc: sta...@dpdk.org
>
> Signed-off-by: Stephen Hemminger
One comment inline below. With that fixed:
Acked-by: Bruce Richardson
> ---
> app/test-dma-perf/main.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/app/test-dma-perf/main.c b/app/test-dma-perf/main.c
> index 18219918cc..dccb0a3541 100644
> --- a/app/test-dma-perf/main.c
> +++ b/app/test-dma-perf/main.c
> @@ -217,27 +217,27 @@ parse_lcore_dma(struct test_configure *test_case, const
> char *value)
> struct lcore_dma_map_t *lcore_dma_map;
> char *input, *addrs;
> char *ptrs[2];
> - char *start, *end, *substr;
> + char *start, *end, *substr, *saveptr;
> uint16_t lcore_id;
> int ret = 0;
>
> if (test_case == NULL || value == NULL)
> return -1;
>
> - input = strndup(value, strlen(value) + 1);
> + input = strdup(value);
> if (input == NULL)
> return -1;
> - addrs = input;
>
> - while (*addrs == '\0')
> - addrs++;
> + addrs = input;
> + while (isspace(*addrs))
> + ++addrs;
> if (*addrs == '\0') {
> fprintf(stderr, "No input DMA addresses\n");
> ret = -1;
> goto out;
> }
>
> - substr = strtok(addrs, ",");
> + substr = strtok_r(addrs, ",", &saveptr);
Don't need to use strtok here at all. Just use strchr, and then no need for
a new temporary variable.
> if (substr == NULL) {
> fprintf(stderr, "No input DMA address\n");
> ret = -1;
> --
> 2.45.2
>
Re: [PATCH v2 09/10] app/test-pmd: avoid potential outside of array reference
On Thu, Nov 14, 2024 at 11:25:07AM -0800, Stephen Hemminger wrote:
> The order of comparison is wrong, and potentially allows
> referencing past the array.
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: 3e3edab530a1 ("ethdev: add flow quota")
> Cc: getel...@nvidia.com
> Cc: sta...@dpdk.org
>
> Signed-off-by: Stephen Hemminger
Acked-by: Bruce Richardson
RE: [EXTERNAL] Re: [PATCH v15 4/4] eal: add PMU support to tracing library
>-Original Message- >From: Stephen Hemminger >Sent: Wednesday, November 13, 2024 12:10 AM >To: Tomasz Duszynski >Cc: Jerin Jacob ; Sunil Kumar Kori ; >Tyler Retzlaff >; ruifeng.w...@arm.com; >bruce.richard...@intel.com; >david.march...@redhat.com; dev@dpdk.org; konstantin.v.anan...@yandex.ru; >mattias.ronnb...@ericsson.com; m...@smartsharesystems.com; >tho...@monjalon.net; zhou...@loongson.cn >Subject: [EXTERNAL] Re: [PATCH v15 4/4] eal: add PMU support to tracing library > >On Fri, 25 Oct 2024 10: 54: 14 +0200 Tomasz Duszynski com> wrote: > In order >to profile app one needs to store significant amount of samples > somewhere >for an analysis later >on. Since trace library supports > >On Fri, 25 Oct 2024 10:54:14 +0200 >Tomasz Duszynski wrote: > >> In order to profile app one needs to store significant amount of >> samples somewhere for an analysis later on. Since trace library >> supports storing data in a CTF format lets take advantage of that and >> add a dedicated PMU tracepoint. >> >> Signed-off-by: Tomasz Duszynski >> --- >> app/test/test_trace_perf.c | 10 >> doc/guides/prog_guide/profile_app.rst| 5 ++ >> doc/guides/prog_guide/trace_lib.rst | 32 +++ >> lib/eal/common/eal_common_trace.c| 5 +- >> lib/eal/common/eal_common_trace_pmu.c| 38 ++ >> lib/eal/common/eal_common_trace_points.c | 5 ++ >> lib/eal/common/eal_trace.h | 4 ++ >> lib/eal/common/meson.build | 1 + >> lib/eal/include/rte_eal_trace.h | 11 >> lib/eal/version.map | 1 + >> lib/pmu/rte_pmu.c| 67 +++- >> lib/pmu/rte_pmu.h| 24 +++-- >> lib/pmu/version.map | 1 + >> 13 files changed, 198 insertions(+), 6 deletions(-) create mode >> 100644 lib/eal/common/eal_common_trace_pmu.c > > >There is an issue with calling a rte_experimental function. > >---BEGIN LOGS > > [Begin job log] "ubuntu-22.04-gcc-debug+doc+examples+tests" at step Build >and test > >[3384/6468] Compiling C object >buildtools/chkincs/chkincs.p/meson-generated_rte_pmu.c.o >FAILED: buildtools/chkincs/chkincs.p/meson-generated_rte_pmu.c.o >ccache gcc -Ibuildtools/chkincs/chkincs.p -Ibuildtools/chkincs >-I../buildtools/chkincs - >Iexamples/l3fwd -I../examples/l3fwd -I../examples/common -Idrivers/bus/vdev >-I../drivers/bus/vdev - >I. -I.. -Iconfig -I../config -Ilib/eal/include -I../lib/eal/include >-Ilib/eal/linux/include - >I../lib/eal/linux/include -Ilib/eal/x86/include -I../lib/eal/x86/include >-Ilib/eal/common - >I../lib/eal/common -Ilib/eal -I../lib/eal -Ilib/kvargs -I../lib/kvargs >-Ilib/log -I../lib/log - >Ilib/metrics -I../lib/metrics -Ilib/telemetry -I../lib/telemetry -Ilib/pmu >-I../lib/pmu - >Idrivers/bus/pci -I../drivers/bus/pci -I../drivers/bus/pci/linux -Ilib/pci >-I../lib/pci - >Idrivers/bus/vmbus -I../drivers/bus/vmbus -I../drivers/bus/vmbus/linux >-Ilib/argparse - >I../lib/argparse -Ilib/ptr_compress -I../lib/ptr_compress -Ilib/ring >-I../lib/ring -Ilib/rcu - >I../lib/rcu -Ilib/mempool -I../lib/mempool -Ilib/mbuf -I../lib/mbuf -Ilib/net >-I../lib/net - >Ilib/meter -I../lib/meter -Ilib/ethdev -I../lib/ethdev -Ilib/cmdline >-I../lib/cmdline -Ilib/hash - >I../lib/hash -Ilib/timer -I../lib/timer -Ilib/acl -I../lib/acl -Ilib/bbdev >-I../lib/bbdev - >Ilib/bitratestats -I../lib/bitratestats -Ilib/bpf -I../lib/bpf -Ilib/cfgfile >-I../lib/cfgfile - >Ilib/compressdev -I../lib/compressdev -Ilib/cryptodev -I../lib/cryptodev >-Ilib/distributor - >I../lib/distributor -Ilib/dmadev -I../lib/dmadev -Ilib/efd -I../lib/efd >-Ilib/eventdev - >I../lib/eventdev -Ilib/dispatcher -I../lib/dispatcher -Ilib/gpudev >-I../lib/gpudev -Ilib/gro - >I../lib/gro -Ilib/gso -I../lib/gso -Ilib/ip_frag -I../lib/ip_frag >-Ilib/jobstats -I../lib/jobstats >-Ilib/latencystats -I../lib/latencystats -Ilib/lpm -I../lib/lpm -Ilib/member >-I../lib/member - >Ilib/pcapng -I../lib/pcapng -Ilib/power -I../lib/power -Ilib/rawdev >-I../lib/rawdev -Ilib/regexdev >-I../lib/regexdev -Ilib/mldev -I../lib/mldev -Ilib/rib -I../lib/rib >-Ilib/reorder -I../lib/reorder >-Ilib/sched -I../lib/sched -Ilib/security -I../lib/security -Ilib/stack >-I../lib/stack -Ilib/vhost >-I../lib/vhost -Ilib/ipsec -I../lib/ipsec -Ilib/pdcp -I../lib/pdcp -Ilib/fib >-I../lib/fib - >Ilib/port -I../lib/port -Ilib/pdump -I../lib/pdump -Ilib/table -I../lib/table >-Ilib/pipeline - >I../lib/pipeline -Ilib/graph -I../lib/graph -Ilib/node -I../lib/node >-fdiagnostics-color=always - >pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -std=c11 -g >-include rte_config.h - >Wcast-qual -Wdeprecated -Wformat -Wformat-nonliteral -Wformat-security >-Wmissi
Re: [PATCH 05/16] net/i40e: remove duplicate code
On Thu, Nov 14, 2024 at 10:05:42PM -0800, Stephen Hemminger wrote:
> There are two branches in the cascading if/else that have same
> condition and code; remove one. Update the code to follow DPDK
> style where all statements in if should have brackets if any
> leg requires them.
>
Not actually DPDK style, that is just something that checkpatch recommends
because it is kernel style. DPDK style guide says[1] "Braces that are not
necessary should be left out."
That said, most legs of this if-else block have it so ok to have that
change included for consistency.
[1]
https://doc.dpdk.org/guides/contributing/coding_style.html#control-statements-and-loops
> Link: https://pvs-studio.com/en/blog/posts/cpp/1183/
Fixes: 2ab5c84605f0 ("net/i40e: fix ESP flow creation")
Acked-by: Bruce Richardson
>
> Signed-off-by: Stephen Hemminger
> ---
> drivers/net/i40e/i40e_fdir.c | 10 --
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_fdir.c b/drivers/net/i40e/i40e_fdir.c
> index 47f79ecf11..6861bea99a 100644
> --- a/drivers/net/i40e/i40e_fdir.c
> +++ b/drivers/net/i40e/i40e_fdir.c
> @@ -599,18 +599,16 @@ i40e_flow_fdir_fill_eth_ip_head(struct i40e_pf *pf,
> } else if (cus_pctype->index == I40E_CUSTOMIZED_ESP_IPV4_UDP) {
> len = fill_ip4_head(fdir_input, raw_pkt, IPPROTO_UDP,
> len, ether_type);
> - } else if (cus_pctype->index == I40E_CUSTOMIZED_ESP_IPV4_UDP) {
> - len = fill_ip4_head(fdir_input, raw_pkt, IPPROTO_UDP,
> - len, ether_type);
> - } else if (cus_pctype->index == I40E_CUSTOMIZED_ESP_IPV6)
> + } else if (cus_pctype->index == I40E_CUSTOMIZED_ESP_IPV6) {
> len = fill_ip6_head(fdir_input, raw_pkt, IPPROTO_ESP,
> len, ether_type);
> - else if (cus_pctype->index == I40E_CUSTOMIZED_ESP_IPV6_UDP)
> + } else if (cus_pctype->index == I40E_CUSTOMIZED_ESP_IPV6_UDP) {
> len = fill_ip6_head(fdir_input, raw_pkt, IPPROTO_UDP,
> len, ether_type);
> - else if (cus_pctype->index == I40E_CUSTOMIZED_IPV6_L2TPV3)
> + } else if (cus_pctype->index == I40E_CUSTOMIZED_IPV6_L2TPV3) {
> len = fill_ip6_head(fdir_input, raw_pkt, IPPROTO_L2TP,
> len, ether_type);
> + }
> } else {
> PMD_DRV_LOG(ERR, "unknown pctype %u.", fdir_input->pctype);
> return -1;
> --
> 2.45.2
>
Re: [PATCH v2] eal/linux: fix fbarray name with multiple secondaryprocesses
On Fri, 15 Nov 2024, Stephen Hemminger wrote: > Need to include to get the prototype rte_get_tsc_cycles() When I compile locally (on x86), it compiles successfully whether or not is included. However, cloud testing indicates that compilation fails on other architectures when is not included. Is the cause of the compilation failure? If so, I'm curious why this is the case.
RE: [PATCH v1 1/2] app/testpmd: fix flow update
Hi,
> -Original Message-
> From: Danylo Vodopianov
> Sent: Thursday, October 31, 2024 16:00
> To: NBU-Contact-Thomas Monjalon (EXTERNAL) ;
> aman.deep.si...@intel.com; yuying.zh...@intel.com; Ori Kam
> ; mko-...@napatech.com; c...@napatech.com; sil-
> p...@napatech.com
> Cc: dev@dpdk.org; ferruh.yi...@amd.com
> Subject: [PATCH v1 1/2] app/testpmd: fix flow update
>
> The testpmd command “flow update…“ provides a nullptr as the context variable.
I would propose the following phrasing, which I think would be more accurate:
If actions provided to “flow update…“ command contained an age action,
Then testpmd did not update the age action context accordingly.
> Thus "flow aged destroy" command can not execute successfully.
>
> Fix was done with next steps
> 1. Generate new port flow entry to add/replace action(s).
> 2. Set age contest if exists.
s/contest if exists/context if age action is present/
> 3. Replace flow in the flow list.
>
> Fixes: 2d9c7e56e52c ("app/testpmd: support updating flow rule actions")
>
> Signed-off-by: Danylo Vodopianov
> ---
> app/test-pmd/config.c | 20 +++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> 88770b4dfc..bf50f6adef 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -3882,7 +3882,8 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
> const struct rte_flow_action *actions, bool is_user_id) {
> struct rte_port *port;
> - struct port_flow **flow_list;
> + struct port_flow **flow_list, *uf;
> + struct rte_flow_action_age *age = age_action_get(actions);
>
> if (port_id_is_invalid(port_id, ENABLED_WARN) ||
> port_id == (portid_t)RTE_PORT_ALL) @@ -3897,6 +3898,16 @@
> port_flow_update(portid_t port_id, uint32_t rule_id,
> flow_list = &flow->next;
> continue;
> }
> +
> + /* Update flow action(s) with new action(s) */
> + uf = port_flow_new(flow->rule.attr_ro, flow->rule.pattern_ro,
> actions,
> &error);
> + if (!uf)
> + return port_flow_complain(&error);
> + if (age) {
> + flow->age_type = ACTION_AGE_CONTEXT_TYPE_FLOW;
> + age->context = &flow->age_type;
> + }
> +
> /*
> * Poisoning to make sure PMDs update it in case
> * of error.
> @@ -3913,6 +3924,13 @@ port_flow_update(portid_t port_id, uint32_t
> rule_id,
> printf("Flow rule #%"PRIu64
>" updated with new actions\n",
>flow->id);
> +
> + uf->next = flow->next;
> + uf->id = flow->id;
Please copy user_id as well.
> + uf->flow = flow->flow;
> + *flow_list = uf;
> +
> + free(flow);
> return 0;
> }
> printf("Failed to find flow %"PRIu32"\n", rule_id);
> --
> 2.43.5
Best regards,
Dariusz Sosnowski
RE: [PATCH v1 2/2] app/testpmd: fix flow destroy
Hi,
> -Original Message-
> From: Danylo Vodopianov
> Sent: Thursday, October 31, 2024 16:00
> To: NBU-Contact-Thomas Monjalon (EXTERNAL) ;
> aman.deep.si...@intel.com; yuying.zh...@intel.com; Ori Kam
> ; mko-...@napatech.com; c...@napatech.com; sil-
> p...@napatech.com
> Cc: dev@dpdk.org; ferruh.yi...@amd.com
> Subject: [PATCH v1 2/2] app/testpmd: fix flow destroy
I think it would be better to rename the commit to: "app/testpmd: fix aged flow
destroy"
>
> Avoid removal of additional flows after requested number of flows has been
> already removed.
>
> Issue with removal of multiple flows is internal testpmd bug at
> port_flow_destroy(). This function goes through all flows and compares given
> flow ‘id’ with them. However in some cases it can advance pointer with “given
> ID”
> and thus remove additional flow.
I'm not sure that the issue with port_flow_destroy() is really a bug.
port_flow_destroy() function never assumed that rule array can be freed when
it's executing,
and port_flow_aged() just violated that assumption.
Could you please rephrase the commit message to include that info?
>
> Fixes: de956d5ecf08 ("app/testpmd: support age shared action context")
This fix will have to be taken into LTS releases. Please add "Cc:
sta...@dpdk.org"
>
> Signed-off-by: Danylo Vodopianov
> ---
> app/test-pmd/config.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> bf50f6adef..50c4b018c1 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -4170,8 +4170,12 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
>ctx.pf->rule.attr->ingress ? 'i' : '-',
>ctx.pf->rule.attr->egress ? 'e' : '-',
>ctx.pf->rule.attr->transfer ? 't' : '-');
> + /* use local copy of id as ctx.pf is freed by
> +* port_flow_destroy() during processing
> +*/
After the commit message is rephrased, I don't think this comment will be
needed.
> + uint64_t flow_id = ctx.pf->id;
Please move the flow_id variable declaration to the beginning of the case.
Also, please enclose the case's body in braces, so that flow_id declaration
does not leak to the following cases.
> if (destroy && !port_flow_destroy(port_id, 1,
> - &ctx.pf->id, false))
> + &flow_id,
> + false))
> total++;
> break;
> case ACTION_AGE_CONTEXT_TYPE_INDIRECT_ACTION:
> --
> 2.43.5
Best regards,
Dariusz Sosnowski
[PATCH v3 09/10] app/test-pmd: avoid potential outside of array reference
The order of comparison is wrong, and potentially allows
referencing past the array.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: 3e3edab530a1 ("ethdev: add flow quota")
Cc: getel...@nvidia.com
Cc: sta...@dpdk.org
Signed-off-by: Stephen Hemminger
Acked-by: Bruce Richardson
---
app/test-pmd/cmdline_flow.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 1e4f2ebc55..9e4fc2d95d 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -12892,7 +12892,7 @@ comp_names_to_index(struct context *ctx, const struct
token *token,
RTE_SET_USED(token);
if (!buf)
return names_size;
- if (names[ent] && ent < names_size)
+ if (ent < names_size && names[ent] != NULL)
return rte_strscpy(buf, names[ent], size);
return -1;
--
2.45.2
[PATCH 1/2] net/bnxt: update HWRM API
Update HWRM API to select ring profile.
Signed-off-by: Ajit Khaparde
---
drivers/net/bnxt/hsi_struct_def_dpdk.h | 71 +-
1 file changed, 69 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bnxt/hsi_struct_def_dpdk.h
b/drivers/net/bnxt/hsi_struct_def_dpdk.h
index 8f348c20fb..737bf2693b 100644
--- a/drivers/net/bnxt/hsi_struct_def_dpdk.h
+++ b/drivers/net/bnxt/hsi_struct_def_dpdk.h
@@ -15617,7 +15617,46 @@ struct hwrm_func_qcaps_output {
* (SR-IOV) disabled or on a VF.
*/
uint32_troce_vf_max_gid;
- uint8_t unused_3[3];
+ uint32_tflags_ext3;
+ /*
+* When this bit is '1', firmware supports the driver using
+* FUNC_CFG (or FUNC_VF_CFG) to decrease resource reservations
+* while some resources are still allocated. An error is returned
+* if the driver tries to set the reservation to be less than the
+* number of allocated resources.
+*/
+ #define HWRM_FUNC_QCAPS_OUTPUT_FLAGS_EXT3_RM_RSV_WHILE_ALLOC_CAP \
+ UINT32_C(0x1)
+ /*
+* When this bit is '1', the PF requires an L2 filter to be
+* allocated by the driver using HWRM_CFA_L2_FILTER_ALLOC after
+* bringing the interface up, before traffic is sent.
+*/
+ #define HWRM_FUNC_QCAPS_OUTPUT_FLAGS_EXT3_REQUIRE_L2_FILTER \
+ UINT32_C(0x2)
+ /*
+* When set to 1, indicates the field max_roce_vfs in the structure
+* is valid. If this bit is 0, the driver should not use the
+* 'max_roce_vfs' field.
+*/
+ #define HWRM_FUNC_QCAPS_OUTPUT_FLAGS_EXT3_MAX_ROCE_VFS_SUPPORTED \
+ UINT32_C(0x4)
+ /*
+* When set to 1, indicates the field 'rx_rate_profile_sel' in
+* RING_ALLOC can specify a valid RX rate profile when allocating
+* RX or RX aggregation rings. If this bit is 0, the driver
+* should not use the 'rx_rate_profile_sel' field.
+*/
+ #define HWRM_FUNC_QCAPS_OUTPUT_FLAGS_EXT3_RX_RATE_PROFILE_SEL_SUPPORTED
\
+ UINT32_C(0x8)
+ /*
+* The number of VFs that can be used for RoCE on the function. If less
+* than max_vfs, roce vfs will be assigned to the first VF of the
+* function and be contiguous.
+* This is valid only on the PF with SR-IOV and RDMA enabled.
+*/
+ uint16_tmax_roce_vfs;
+ uint8_t unused_3[5];
/*
* This field is used in Output records to indicate that the output
* is completely written to RAM. This field should be read as '1'
@@ -45026,6 +45065,14 @@ struct hwrm_ring_alloc_input {
*/
#define HWRM_RING_ALLOC_INPUT_ENABLES_STEERING_TAG_VALID \
UINT32_C(0x800)
+ /*
+* This bit must be '1' for the rx_rate_profile_sel field to
+* be configured. This should only be used when
+* 'rx_rate_profile_sel_supported' bit is set in flags_ext3
+* field of FUNC_QCAPS response.
+*/
+ #define HWRM_RING_ALLOC_INPUT_ENABLES_RX_RATE_PROFILE_VALID \
+ UINT32_C(0x1000)
/* Ring Type. */
uint8_t ring_type;
/* L2 Completion Ring (CR) */
@@ -45362,7 +45409,27 @@ struct hwrm_ring_alloc_input {
#define HWRM_RING_ALLOC_INPUT_MPC_CHNLS_TYPE_PRIMATE UINT32_C(0x4)
#define HWRM_RING_ALLOC_INPUT_MPC_CHNLS_TYPE_LAST \
HWRM_RING_ALLOC_INPUT_MPC_CHNLS_TYPE_PRIMATE
- uint8_t unused_4[2];
+ /* RX rate profile select */
+ uint8_t rx_rate_profile_sel;
+ /*
+* Indicate default RX rate profile when allocating
+* RX or RX aggregation rings. This should only be
+* used when 'rx_rate_profile_sel_supported' bit is
+* set in flags_ext3 field of FUNC_QCAPS response.
+*/
+ #define HWRM_RING_ALLOC_INPUT_RX_RATE_PROFILE_SEL_DEFAULT \
+ UINT32_C(0x0)
+ /*
+* Indicate poll_mode RX rate profile when allocating
+* RX or RX aggregation rings. This should only be
+* used when 'rx_rate_profile_sel_supported' bit is
+* set in flags_ext3 field of FUNC_QCAPS response.
+*/
+ #define HWRM_RING_ALLOC_INPUT_RX_RATE_PROFILE_SEL_POLL_MODE \
+ UINT32_C(0x1)
+ #define HWRM_RING_ALLOC_INPUT_RX_RATE_PROFILE_SEL_LAST \
+ HWRM_RING_ALLOC_INPUT_RX_RATE_PROFILE_SEL_POLL_MODE
+ uint8_t unused_4;
/*
* The cq_handle is specified when allocating a completion ring. For
* devices that support NQs, this cq_handle will be included in the
--
2.39.5 (Apple Git-154)
[PATCH 2/2] net/bnxt: add support for Rx profile selection
Some firmware versions can support the selection of Rx profile
during Rx and AGG ring allocation.
Check if the firmware sets the
HWRM_FUNC_QCAPS_OUTPUT_FLAGS_EXT3_RX_RATE_PROFILE_SEL_SUPPORTED flag
and set the new Rx profile.
Signed-off-by: Ajit Khaparde
Reviewed-by: Andy Gospodarek
---
drivers/net/bnxt/bnxt.h | 1 +
drivers/net/bnxt/bnxt_hwrm.c | 15 ++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/net/bnxt/bnxt.h b/drivers/net/bnxt/bnxt.h
index 771349de6c..0402de3eb9 100644
--- a/drivers/net/bnxt/bnxt.h
+++ b/drivers/net/bnxt/bnxt.h
@@ -866,6 +866,7 @@ struct bnxt {
#define BNXT_FW_CAP_TX_COAL_CMPL BIT(10)
#define BNXT_FW_CAP_RX_ALL_PKT_TS BIT(11)
#define BNXT_FW_CAP_BACKING_STORE_V2 BIT(12)
+#define BNXT_FW_CAP_RX_RATE_PROFILEBIT(17)
#define BNXT_FW_BACKING_STORE_V2_EN(bp)\
((bp)->fw_cap & BNXT_FW_CAP_BACKING_STORE_V2)
#define BNXT_FW_BACKING_STORE_V1_EN(bp)\
diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index 351effb28f..d015ba2b9c 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -1139,8 +1139,8 @@ static int __bnxt_hwrm_func_qcaps(struct bnxt *bp)
int rc = 0;
struct hwrm_func_qcaps_input req = {.req_type = 0 };
struct hwrm_func_qcaps_output *resp = bp->hwrm_cmd_resp_addr;
+ uint32_t flags, flags_ext2, flags_ext3;
uint16_t new_max_vfs;
- uint32_t flags, flags_ext2;
HWRM_PREP(&req, HWRM_FUNC_QCAPS, BNXT_USE_CHIMP_MB);
@@ -1153,6 +1153,7 @@ static int __bnxt_hwrm_func_qcaps(struct bnxt *bp)
bp->max_ring_grps = rte_le_to_cpu_32(resp->max_hw_ring_grps);
flags = rte_le_to_cpu_32(resp->flags);
flags_ext2 = rte_le_to_cpu_32(resp->flags_ext2);
+ flags_ext3 = rte_le_to_cpu_32(resp->flags_ext3);
if (BNXT_PF(bp)) {
bp->pf->port_id = resp->port_id;
@@ -1259,6 +1260,8 @@ static int __bnxt_hwrm_func_qcaps(struct bnxt *bp)
bp->fw_cap |= BNXT_FW_CAP_RX_ALL_PKT_TS;
if (flags_ext2 & HWRM_FUNC_QCAPS_OUTPUT_FLAGS_EXT2_UDP_GSO_SUPPORTED)
bp->fw_cap |= BNXT_FW_CAP_UDP_GSO;
+ if (flags_ext3 &
HWRM_FUNC_QCAPS_OUTPUT_FLAGS_EXT3_RX_RATE_PROFILE_SEL_SUPPORTED)
+ bp->fw_cap |= BNXT_FW_CAP_RX_RATE_PROFILE;
unlock:
HWRM_UNLOCK();
@@ -2227,6 +2230,11 @@ int bnxt_hwrm_ring_alloc(struct bnxt *bp,
if (stats_ctx_id != INVALID_STATS_CTX_ID)
enables |=
HWRM_RING_ALLOC_INPUT_ENABLES_STAT_CTX_ID_VALID;
+ if (bp->fw_cap & BNXT_FW_CAP_RX_RATE_PROFILE) {
+ req.rx_rate_profile_sel =
+
HWRM_RING_ALLOC_INPUT_RX_RATE_PROFILE_SEL_POLL_MODE;
+ enables |=
HWRM_RING_ALLOC_INPUT_ENABLES_RX_RATE_PROFILE_VALID;
+ }
break;
case HWRM_RING_ALLOC_INPUT_RING_TYPE_L2_CMPL:
req.ring_type = ring_type;
@@ -2257,6 +2265,11 @@ int bnxt_hwrm_ring_alloc(struct bnxt *bp,
enables |= HWRM_RING_ALLOC_INPUT_ENABLES_RX_RING_ID_VALID |
HWRM_RING_ALLOC_INPUT_ENABLES_RX_BUF_SIZE_VALID |
HWRM_RING_ALLOC_INPUT_ENABLES_STAT_CTX_ID_VALID;
+ if (bp->fw_cap & BNXT_FW_CAP_RX_RATE_PROFILE) {
+ req.rx_rate_profile_sel =
+
HWRM_RING_ALLOC_INPUT_RX_RATE_PROFILE_SEL_POLL_MODE;
+ enables |=
HWRM_RING_ALLOC_INPUT_ENABLES_RX_RATE_PROFILE_VALID;
+ }
break;
default:
PMD_DRV_LOG_LINE(ERR, "hwrm alloc invalid ring type %d",
--
2.39.5 (Apple Git-154)
[PATCH] member: fix choice of bucket for displacement
Because of misuse of & vs && operator, the member code would
always use the primary bucket.
Fixes: 904ec78a239c ("member: implement HT mode")
Cc: yipeng1.w...@intel.com
Cc: sta...@dpdk.org
Signed-off-by: Stephen Hemminger
---
lib/member/rte_member_ht.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/member/rte_member_ht.c b/lib/member/rte_member_ht.c
index 357097ff4b..738471b378 100644
--- a/lib/member/rte_member_ht.c
+++ b/lib/member/rte_member_ht.c
@@ -494,7 +494,7 @@ rte_member_add_ht(const struct rte_member_setsum *ss,
return ret;
/* Random pick prim or sec for recursive displacement */
- uint32_t select_bucket = (tmp_sig && 1U) ? prim_bucket : sec_bucket;
+ uint32_t select_bucket = (tmp_sig & 1U) ? prim_bucket : sec_bucket;
if (ss->cache) {
ret = evict_from_bucket();
buckets[select_bucket].sigs[ret] = tmp_sig;
--
2.45.2
[DPDK/other Bug 1583] lib/mldev: conditional expression always true
https://bugs.dpdk.org/show_bug.cgi?id=1583
Bug ID: 1583
Summary: lib/mldev: conditional expression always true
Product: DPDK
Version: 24.11
Hardware: All
OS: All
Status: UNCONFIRMED
Severity: normal
Priority: Normal
Component: other
Assignee: dev@dpdk.org
Reporter: step...@networkplumber.org
Target Milestone: ---
PVS Studio giving warning here.
lib/mldev/mldev_utils_scalar.c:495:1: warning: V560 A part of conditional
expression is always true.
} else if ((be_16 >= 1) && (be_16 < (int)(FP16_MASK_E >>
FP16_LSB_E))) {
lib/mldev/mldev_utils_scalar.c:518:1: warning: V560 A part of conditional
expression is always true: (be_16 < 1).
} else if ((be_16 >= -(int)(FP16_MSB_M)) && (be_16 < 1)) {
--
You are receiving this mail because:
You are the assignee for the bug.
[PATCH] dts: remove redundant test suite
The test suite served as a demonstration of the Scapy traffic generator
implementation. Now that we have a test suite that uses DPDK code (via
testpmd), there is no reason to keep the test suite, as there's no
expectation it'll be actually used in any setup.
Signed-off-by: Juraj Linkeš
Signed-off-by: Paul Szczepanek
---
dts/framework/test_suite.py | 36 -
dts/framework/testbed_model/linux_session.py | 22 +---
dts/framework/testbed_model/node.py | 2 -
dts/framework/testbed_model/os_session.py| 34
dts/framework/testbed_model/sut_node.py | 8 ---
dts/tests/TestSuite_os_udp.py| 54
6 files changed, 1 insertion(+), 155 deletions(-)
delete mode 100644 dts/tests/TestSuite_os_udp.py
diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
index 7a75334cfa..adee01f031 100644
--- a/dts/framework/test_suite.py
+++ b/dts/framework/test_suite.py
@@ -209,42 +209,6 @@ def tear_down_test_case(self) -> None:
This is done after *each* test case.
"""
-def configure_testbed_ipv4(self, restore: bool = False) -> None:
-"""Configure IPv4 addresses on all testbed ports.
-
-The configured ports are:
-
-* SUT ingress port,
-* SUT egress port,
-* TG ingress port,
-* TG egress port.
-
-Args:
-restore: If :data:`True`, will remove the configuration instead.
-"""
-delete = True if restore else False
-enable = False if restore else True
-self._configure_ipv4_forwarding(enable)
-self.sut_node.configure_port_ip_address(
-self._sut_ip_address_egress, self._sut_port_egress, delete
-)
-self.sut_node.configure_port_state(self._sut_port_egress, enable)
-self.sut_node.configure_port_ip_address(
-self._sut_ip_address_ingress, self._sut_port_ingress, delete
-)
-self.sut_node.configure_port_state(self._sut_port_ingress, enable)
-self.tg_node.configure_port_ip_address(
-self._tg_ip_address_ingress, self._tg_port_ingress, delete
-)
-self.tg_node.configure_port_state(self._tg_port_ingress, enable)
-self.tg_node.configure_port_ip_address(
-self._tg_ip_address_egress, self._tg_port_egress, delete
-)
-self.tg_node.configure_port_state(self._tg_port_egress, enable)
-
-def _configure_ipv4_forwarding(self, enable: bool) -> None:
-self.sut_node.configure_ipv4_forwarding(enable)
-
def send_packet_and_capture(
self,
packet: Packet,
diff --git a/dts/framework/testbed_model/linux_session.py
b/dts/framework/testbed_model/linux_session.py
index 544a665b83..f87efb8f18 100644
--- a/dts/framework/testbed_model/linux_session.py
+++ b/dts/framework/testbed_model/linux_session.py
@@ -10,8 +10,7 @@
"""
import json
-from ipaddress import IPv4Interface, IPv6Interface
-from typing import TypedDict, Union
+from typing import TypedDict
from typing_extensions import NotRequired
@@ -179,25 +178,6 @@ def _update_port_attr(self, port: Port, attr_value: str |
None, attr_name: str)
f"Attempted to get '{attr_name}' of port {port.pci}, but it
doesn't exist."
)
-def configure_port_state(self, port: Port, enable: bool) -> None:
-"""Overrides :meth:`~.os_session.OSSession.configure_port_state`."""
-state = "up" if enable else "down"
-self.send_command(f"ip link set dev {port.logical_name} {state}",
privileged=True)
-
-def configure_port_ip_address(
-self,
-address: Union[IPv4Interface, IPv6Interface],
-port: Port,
-delete: bool,
-) -> None:
-"""Overrides
:meth:`~.os_session.OSSession.configure_port_ip_address`."""
-command = "del" if delete else "add"
-self.send_command(
-f"ip address {command} {address} dev {port.logical_name}",
-privileged=True,
-verify=True,
-)
-
def configure_port_mtu(self, mtu: int, port: Port) -> None:
"""Overrides :meth:`~.os_session.OSSession.configure_port_mtu`."""
self.send_command(
diff --git a/dts/framework/testbed_model/node.py
b/dts/framework/testbed_model/node.py
index 6031eaf937..85144f6f4e 100644
--- a/dts/framework/testbed_model/node.py
+++ b/dts/framework/testbed_model/node.py
@@ -96,8 +96,6 @@ def __init__(self, node_config: NodeConfiguration):
def _init_ports(self) -> None:
self.ports = [Port(self.name, port_config) for port_config in
self.config.ports]
self.main_session.update_ports(self.ports)
-for port in self.ports:
-self.configure_port_state(port)
def set_up_test_run(
self,
diff --git a/dts/framework/testbed_model/os_session.py
b/dts/framework/testbed_model/os_session.py
index 294f5b36ba..62add7a4df 100644
--- a/dts/framework/testbed_model/os_session.py
+++ b/d
Re: [PATCH] dts: remove redundant test suite
Reviewed-by: Patrick Robb
Re: [PATCH v10 2/6] power: refactor uncore power management library
On Mon, 28 Oct 2024 19:55:52 +
Sivaprasad Tummala wrote:
> + /* Auto Detect Environment */
> + RTE_TAILQ_FOREACH(ops, &uncore_ops_list, next)
> + if (ops) {
> + POWER_LOG(INFO,
> + "Attempting to initialise %s power
> management...",
> + ops->name);
> + ret = ops->init(pkg, die);
> + if (ret == 0) {
> + for (env = 0; env < RTE_DIM(uncore_env_str);
> env++)
> + if (strncmp(ops->name,
> uncore_env_str[env],
> + RTE_POWER_UNCORE_DRIVER_NAMESZ)
> == 0) {
> + rte_power_set_uncore_env(env);
> + goto out;
> + }
> + }
> + }
> out:
Static analyzer complains:
lib/power/rte_power_uncore.c:113:1: warning: V547 Expression 'ops' is always
true.
Since the macro RTE_TAILQ_FOREACH() iterates until ops is NULL, that whole if()
part
can be removed.
[DPDK/ethdev Bug 1582] virtio: reader/writer lock mismatch
https://bugs.dpdk.org/show_bug.cgi?id=1582
Bug ID: 1582
Summary: virtio: reader/writer lock mismatch
Product: DPDK
Version: 24.11
Hardware: All
OS: All
Status: UNCONFIRMED
Severity: major
Priority: Normal
Component: ethdev
Assignee: dev@dpdk.org
Reporter: step...@networkplumber.org
Target Milestone: ---
The code virtio_dev_rx_async_submit has mismatch between locking the
access_lock for write, and then releases it with read_unlock. This could cause
all sorts of deadlock depending on the implementation of rwlock.
static __rte_always_inline uint32_t
virtio_dev_rx_async_submit(struct virtio_net *dev, struct vhost_virtqueue *vq,
struct rte_mbuf **pkts, uint32_t count, int16_t dma_id, uint16_t
vchan_id)
{
uint32_t nb_tx = 0;
VHOST_DATA_LOG(dev->ifname, DEBUG, "%s", __func__);
if (unlikely(!dma_copy_track[dma_id].vchans ||
!dma_copy_track[dma_id].vchans[vchan_id].pkts_cmpl_flag_addr)) {
VHOST_DATA_LOG(dev->ifname, ERR,
"%s: invalid channel %d:%u.",
__func__, dma_id, vchan_id);
return 0;
}
rte_rwlock_write_lock(&vq->access_lock);
if (unlikely(!vq->enabled || !vq->async))
goto out_access_unlock;
vhost_user_iotlb_rd_lock(vq);
if (unlikely(!vq->access_ok)) {
vhost_user_iotlb_rd_unlock(vq);
rte_rwlock_read_unlock(&vq->access_lock); HERE
virtio_dev_vring_translate(dev, vq);
goto out_no_unlock;
}
--
You are receiving this mail because:
You are the assignee for the bug.
Re: [PATCH v2] eal/linux: fix fbarray name with multiple secondaryprocesses
On Fri, 15 Nov 2024, Stephen Hemminger wrote: > Need to include to get the prototype rte_get_tsc_cycles() For the code without could run on x86, I think I have found the reason. Through the path -> -> , the is included. is independent on each architecture. And on x86 the includes its own "rte_cycles.h", while other architectures do not.
Re: rte_fib network order bug
Morten Brørup, Nov 14, 2024 at 15:35: RTE_IPV4 is only useful to define addresses in unit tests. There are plenty of special IP addresses and subnets, where a shortcut macro makes the address easier readable in the code. OK, let me reformulate. I didn't mean to say that RTE_IPV4 is useless. But it will always generate addresses in *host order*. Which means they cannot be used in IPv4 headers without passing them through htonl(). This is weird in my opinion. Why would control plane use a different representation of addresses compared to data plane? Excellent question. Old habit? Growing up using big endian CPUs, we have come to think of IPv4 addresses as 32 bit numbers, so we keep treating them as such. With this old way of thinking, the only reason to use network endian in the fast path with little endian CPUs is for performance reasons (to save the byte swap) - if not, we would still prefer using host endian in the fast path too. I understand the implementation reasons why you would prefer working with host order integers. But the APIs that deal with IPv4 addresses should not reflect implementation details. Also for consistency with IPv6, I really think that *all* addresses should be dealt in their network form. Food for thought! Vladimir, could we at least consider adding a real network order mode for the rib and fib libraries? So that we can have consistent APIs between IPv4 and IPv6? On that same topic, I wonder if it would make sense to change the API parameters to use an opaque rte_ipv4_addr_t type instead of a native uint32_t to avoid any confusion. Thanks!
Re: bug in cryptodev enqueue/dequeue callbacks?
> On Nov 14, 2024, at 8:41 AM, Konstantin Ananyev
> wrote:
>
> Hi everyone,
>
> Looking at implementation of cryptodev callbacks
> (it uses DPDK RCU), it seems like there is a bug here:
>
> at init time we don't call rte_rcu_qsbr_thread_register().
> As I understand without it rte_rcu_qsbr_check() wouldn't
> work properly for that thread.
Yes, this understanding is correct. However, the responsibility of calling the
rte_rcu_qsbr_thread_register lies with the application. The roles and
responsibilities are documented at [1]
[1]
https://doc.dpdk.org/guides/prog_guide/rcu_lib.html#resource-reclamation-framework-for-dpdk
>
> Probably need to add:
> static int
> cryptodev_cb_init(struct rte_cryptodev *dev)
> {
>
> if (rte_rcu_qsbr_init(qsbr, max_threads)) {...}
> + rte_rcu_qsbr_thread_register(qsbr, 0);
>
> Unless I am missing something obvious here?
> Konstantin
>
[PATCH v3] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion
../lib/rcu/rte_rcu_qsbr.c(101): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
../lib/rcu/rte_rcu_qsbr.c(107): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
../lib/rcu/rte_rcu_qsbr.c(145): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
These warnings are being issued by the MSVC compiler. Since the result is
being stored in a variable of type uint64_t, it makes sense to shift a
64-bit number instead of shifting a 32-bit number and then having the
compiler to convert the result implicitly to 64 bits.
UINT64_C was used in the fix as it is the portable way to define a 64-bit
constant (ULL suffix is architecture dependent).
>From reading the code this is also a bugfix:
(1 << id), where id = thread_id & 0x3f, was wrong when thread_id > 0x1f.
Fixes: 64994b56cfd7 ("rcu: add RCU library supporting QSBR mechanism")
Signed-off-by: Andre Muezerie
Reviewed-by: Honnappa Nagarahalli
Reviewed-by: Morten Brørup
---
lib/rcu/rte_rcu_qsbr.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/lib/rcu/rte_rcu_qsbr.c b/lib/rcu/rte_rcu_qsbr.c
index 40d7c566c8..dbf31501a6 100644
--- a/lib/rcu/rte_rcu_qsbr.c
+++ b/lib/rcu/rte_rcu_qsbr.c
@@ -99,12 +99,12 @@ rte_rcu_qsbr_thread_register(struct rte_rcu_qsbr *v,
unsigned int thread_id)
/* Add the thread to the bitmap of registered threads */
old_bmap = rte_atomic_fetch_or_explicit(__RTE_QSBR_THRID_ARRAY_ELM(v,
i),
- (1UL << id),
rte_memory_order_release);
+ RTE_BIT64(id),
rte_memory_order_release);
/* Increment the number of threads registered only if the thread was
not already
* registered
*/
- if (!(old_bmap & (1UL << id)))
+ if (!(old_bmap & RTE_BIT64(id)))
rte_atomic_fetch_add_explicit(&v->num_threads, 1,
rte_memory_order_relaxed);
return 0;
@@ -137,12 +137,12 @@ rte_rcu_qsbr_thread_unregister(struct rte_rcu_qsbr *v,
unsigned int thread_id)
* reporting threads.
*/
old_bmap = rte_atomic_fetch_and_explicit(__RTE_QSBR_THRID_ARRAY_ELM(v,
i),
-~(1UL << id),
rte_memory_order_release);
+~RTE_BIT64(id),
rte_memory_order_release);
/* Decrement the number of threads unregistered only if the thread was
not already
* unregistered
*/
- if (old_bmap & (1UL << id))
+ if (old_bmap & RTE_BIT64(id))
rte_atomic_fetch_sub_explicit(&v->num_threads, 1,
rte_memory_order_relaxed);
return 0;
@@ -198,7 +198,7 @@ rte_rcu_qsbr_dump(FILE *f, struct rte_rcu_qsbr *v)
t = rte_ctz64(bmap);
fprintf(f, "%u ", id + t);
- bmap &= ~(1UL << t);
+ bmap &= ~RTE_BIT64(t);
}
}
@@ -225,7 +225,7 @@ rte_rcu_qsbr_dump(FILE *f, struct rte_rcu_qsbr *v)
rte_atomic_load_explicit(
&v->qsbr_cnt[id + t].lock_cnt,
rte_memory_order_relaxed));
- bmap &= ~(1UL << t);
+ bmap &= ~RTE_BIT64(t);
}
}
--
2.34.1
Re: [PATCH v2] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion
On Fri, Nov 15, 2024 at 03:21:42PM +0100, David Marchand wrote:
> On Wed, Nov 13, 2024 at 5:24 PM Andre Muezerie
> wrote:
> >
> > ../lib/rcu/rte_rcu_qsbr.c(101): warning C4334: '<<': result of 32-bit
> > shift implicitly converted to 64 bits (was 64-bit shift intended?)
> > ../lib/rcu/rte_rcu_qsbr.c(107): warning C4334: '<<': result of 32-bit
> > shift implicitly converted to 64 bits (was 64-bit shift intended?)
> > ../lib/rcu/rte_rcu_qsbr.c(145): warning C4334: '<<': result of 32-bit
> > shift implicitly converted to 64 bits (was 64-bit shift intended?)
> >
> > These warnings are being issued by the MSVC compiler. Since the result is
> > being stored in a variable of type uint64_t, it makes sense to shift a
> > 64-bit number instead of shifting a 32-bit number and then having the
> > compiler to convert the result implicitly to 64 bits.
> > UINT64_C was used in the fix as it is the portable way to define a 64-bit
> > constant (ULL suffix is architecture dependent).
> >
> > From reading the code this is also a bugfix:
> > (1 << id), where id = thread_id & 0x3f, was wrong when thread_id > 0x1f.
> >
> > Fixes: 64994b56cfd7 ("rcu: add RCU library supporting QSBR mechanism")
> >
> > Signed-off-by: Andre Muezerie
> > Reviewed-by: Honnappa Nagarahalli
> > Reviewed-by: Morten Brørup
>
> Just a nit, EAL provides a macro:
> lib/eal/include/rte_bitops.h:#define RTE_BIT64(nr) (UINT64_C(1) << (nr))
>
Ah, nice! Thanks for letting me know. I'll update the patch.
>
> Does this change allow to build the rcu library with MSVC?
> I see it is disabled in meson.
You're correct. The rcu is currently not being built for MSVC. I'm fixing the
issues and want to include rcu in the MSVC built. I'm doing the same for other
parts of the DPDK code and I appreciate the help in reviewing the changes.
Thanks!
Andre Muezerie
>
>
> --
> David Marchand
Re: [PATCH] argparse: enable code to be compiled with MSVC compiler
On Tue, Nov 12, 2024 at 11:48 PM Andre Muezerie wrote: > > The issues that were preventing argparser from getting compiled with > MSVC were fixed, so now it should not be excluded from the > compilation anymore. Just to be sure, it relates to fixes of bz 1409, right? It's cool no new regression was introduced in the mean time. Enabling libraries the sooner is better for getting a larger MSVC support, I see no problem in taking this patch for rc3. > > Signed-off-by: Andre Muezerie -- David Marchand
RE: rte_fib network order bug
> From: Robin Jarry [mailto:rja...@redhat.com] > Sent: Friday, 15 November 2024 14.02 > > Morten Brørup, Nov 14, 2024 at 15:35: > >> RTE_IPV4 is only useful to define addresses in unit tests. > > > > There are plenty of special IP addresses and subnets, where a > shortcut > > macro makes the address easier readable in the code. > > OK, let me reformulate. I didn't mean to say that RTE_IPV4 is useless. > But it will always generate addresses in *host order*. Which means they > cannot be used in IPv4 headers without passing them through htonl(). > This is weird in my opinion. Robin, you've totally won me over on this endian discussion. :-) Especially the IPv6 comparison make it clear why IPv4 should also be network byte order. API/ABI stability is a pain... we're stuck with host endian IPv4 addresses; e.g. for the RTE_IPV4() macro, which I now agree produces the wrong endian value (on little endian CPUs). > > >> Why would control plane use a different representation of addresses > >> compared to data plane? > > > > Excellent question. > > Old habit? Growing up using big endian CPUs, we have come to think of > > IPv4 addresses as 32 bit numbers, so we keep treating them as such. > > With this old way of thinking, the only reason to use network endian > > in the fast path with little endian CPUs is for performance reasons > > (to save the byte swap) - if not, we would still prefer using host > > endian in the fast path too. > > I understand the implementation reasons why you would prefer working > with host order integers. But the APIs that deal with IPv4 addresses > should not reflect implementation details. They were probably designed based on the same way of thinking I was used to (until you convinced me I was wrong). > > >> Also for consistency with IPv6, I really think > >> that *all* addresses should be dealt in their network form. > > > > Food for thought! > > Vladimir, could we at least consider adding a real network order mode > for the rib and fib libraries? So that we can have consistent APIs > between IPv4 and IPv6? And/or rename RTE_FIB_F_NETWORK_ORDER to RTE_FIB_F_NETWORK_ORDER_LOOKUP or similar. This is important if real network order mode is added (now or later)! > > On that same topic, I wonder if it would make sense to change the API > parameters to use an opaque rte_ipv4_addr_t type instead of a native > uint32_t to avoid any confusion. It could be considered an IPv4 address type (like the IPv6 address type) (which should be in network endian), which it is not, so I don't like this idea. What the API really should offer is a choice (or a union) of uint32_t and rte_be32_t, but that's not possible, so also using uint32_t for big endian values seems like a viable compromise. Another alternative, using void* for the IPv4 address array, seems overkill to me, since compilers don't warn about mixing uint32_t with rte_be32_t values (like mixing signed and unsigned emits warnings). > > Thanks!
Re: rte_fib network order bug
Morten Brørup, Nov 15, 2024 at 14:52: Robin, you've totally won me over on this endian discussion. :-) Especially the IPv6 comparison make it clear why IPv4 should also be network byte order. API/ABI stability is a pain... we're stuck with host endian IPv4 addresses; e.g. for the RTE_IPV4() macro, which I now agree produces the wrong endian value (on little endian CPUs). At least for 24.11 it is too late. But maybe we could make it right for the next LTS? Vladimir, could we at least consider adding a real network order mode for the rib and fib libraries? So that we can have consistent APIs between IPv4 and IPv6? And/or rename RTE_FIB_F_NETWORK_ORDER to RTE_FIB_F_NETWORK_ORDER_LOOKUP or similar. This is important if real network order mode is added (now or later)! Maybe we could revert that patch and defer a complete change of the rib/fib APIs to only expose network order addresses? It would be an ABI breakage but if properly announced in advance, it should be possible. Thinking about it some more. Having a flag for such a drastic change in behaviour does not seem right. On that same topic, I wonder if it would make sense to change the API parameters to use an opaque rte_ipv4_addr_t type instead of a native uint32_t to avoid any confusion. It could be considered an IPv4 address type (like the IPv6 address type) (which should be in network endian), which it is not, so I don't like this idea. What the API really should offer is a choice (or a union) of uint32_t and rte_be32_t, but that's not possible, so also using uint32_t for big endian values seems like a viable compromise. Another alternative, using void* for the IPv4 address array, seems overkill to me, since compilers don't warn about mixing uint32_t with rte_be32_t values (like mixing signed and unsigned emits warnings). If what I proposed above is possible, then all these APIs could be using rte_be32_t values (or even better, an rte_ipv4_addr_t alias for consistency with IPv6). That would make everything much simpler. Thoughts?
RE: [PATCH v15 1/4] lib: add generic support for reading PMU events
>-Original Message-
>From: Konstantin Ananyev
>Sent: Tuesday, November 5, 2024 11:58 AM
>To: Tomasz Duszynski ; Thomas Monjalon
>
>Cc: ruifeng.w...@arm.com; bruce.richard...@intel.com;
>david.march...@redhat.com; dev@dpdk.org;
>Jerin Jacob ; konstantin.v.anan...@yandex.ru;
>mattias.ronnb...@ericsson.com;
>m...@smartsharesystems.com; roret...@linux.microsoft.com;
>step...@networkplumber.org;
>zhou...@loongson.cn
>Subject: [EXTERNAL] RE: [PATCH v15 1/4] lib: add generic support for reading
>PMU events
>
>> Add support for programming PMU counters and reading their values > in
>> runtime bypassing kernel completely. > > This is especially useful in
>> cases where CPU cores are isolated > i. e run dedicated tasks. In such
>> cases one cannot
>
>
>
>> Add support for programming PMU counters and reading their values in
>> runtime bypassing kernel completely.
>>
>> This is especially useful in cases where CPU cores are isolated i.e
>> run dedicated tasks. In such cases one cannot use standard perf
>> utility without sacrificing latency and performance.
>
>LGTM in general, just few questions, nits - majority about docs/comments.
>
>> Signed-off-by: Tomasz Duszynski
>> ---
>> MAINTAINERS| 5 +
>> app/test/meson.build | 1 +
>> app/test/test_pmu.c| 49 +++
>> doc/api/doxy-api-index.md | 3 +-
>> doc/api/doxy-api.conf.in | 1 +
>> doc/guides/prog_guide/profile_app.rst | 29 ++
>> doc/guides/rel_notes/release_24_11.rst | 7 +
>> lib/eal/meson.build| 3 +
>> lib/meson.build| 1 +
>> lib/pmu/meson.build| 13 +
>> lib/pmu/pmu_private.h | 32 ++
>> lib/pmu/rte_pmu.c | 473 +
>> lib/pmu/rte_pmu.h | 205 +++
>> lib/pmu/version.map| 13 +
>> 14 files changed, 834 insertions(+), 1 deletion(-) create mode
>> 100644 app/test/test_pmu.c create mode 100644 lib/pmu/meson.build
>> create mode 100644 lib/pmu/pmu_private.h create mode 100644
>> lib/pmu/rte_pmu.c create mode 100644 lib/pmu/rte_pmu.h create mode
>> 100644 lib/pmu/version.map
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS index cd78bc7db1..077efe41cf
>> 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1816,6 +1816,11 @@ M: Nithin Dabilpuram
>> M: Pavan Nikhilesh
>> F: lib/node/
>>
>> +PMU - EXPERIMENTAL
>> +M: Tomasz Duszynski
>> +F: lib/pmu/
>> +F: app/test/test_pmu*
>> +
>>
>> Test Applications
>> -
>> diff --git a/app/test/meson.build b/app/test/meson.build index
>> 0f7e11969a..5f1622ecab 100644
>> --- a/app/test/meson.build
>> +++ b/app/test/meson.build
>> @@ -141,6 +141,7 @@ source_file_deps = {
>> 'test_pmd_perf.c': ['ethdev', 'net'] + packet_burst_generator_deps,
>> 'test_pmd_ring.c': ['net_ring', 'ethdev', 'bus_vdev'],
>> 'test_pmd_ring_perf.c': ['ethdev', 'net_ring', 'bus_vdev'],
>> +'test_pmu.c': ['pmu'],
>> 'test_power.c': ['power'],
>> 'test_power_cpufreq.c': ['power'],
>> 'test_power_intel_uncore.c': ['power'], diff --git
>> a/app/test/test_pmu.c b/app/test/test_pmu.c new file mode 100644 index
>> 00..464e5068ec
>> --- /dev/null
>> +++ b/app/test/test_pmu.c
>> @@ -0,0 +1,49 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + * Copyright(C) 2024 Marvell International Ltd.
>> + */
>> +
>> +#include
>> +
>> +#include "test.h"
>> +
>> +static int
>> +test_pmu_read(void)
>> +{
>> +const char *name = NULL;
>> +int tries = 10, event;
>> +uint64_t val = 0;
>> +
>> +if (name == NULL) {
>> +printf("PMU not supported on this arch\n");
>> +return TEST_SKIPPED;
>> +}
>> +
>> +if (rte_pmu_init() < 0)
>> +return TEST_FAILED;
>> +
>> +event = rte_pmu_add_event(name);
>> +while (tries--)
>> +val += rte_pmu_read(event);
>> +
>> +rte_pmu_fini();
>> +
>> +return val ? TEST_SUCCESS : TEST_FAILED; }
>> +
>> +static struct unit_test_suite pmu_tests = {
>> +.suite_name = "pmu autotest",
>> +.setup = NULL,
>> +.teardown = NULL,
>> +.unit_test_cases = {
>> +TEST_CASE(test_pmu_read),
>> +TEST_CASES_END()
>> +}
>> +};
>> +
>> +static int
>> +test_pmu(void)
>> +{
>> +return unit_test_suite_runner(&pmu_tests);
>> +}
>> +
>> +REGISTER_FAST_TEST(pmu_autotest, true, true, test_pmu);
>> diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
>> index 266c8b90dc..3e6020f367 100644
>> --- a/doc/api/doxy-api-index.md
>> +++ b/doc/api/doxy-api-index.md
>> @@ -240,7 +240,8 @@ The public API headers are grouped by topics:
>>[log](@ref rte_log.h),
>>[errno](@ref rte_errno.h),
>>[trace](@ref rte_trace.h),
>> - [trace_point](@ref rte_trace_point.h)
>> + [trace_point](@ref rte_trace_point.h), [pmu](@ref rte_pmu.h)
>>
>> - **misc**:
>>[EAL config](@ref rte_eal.h)
Re: rte_fib network order bug
On Fri, Nov 15, 2024 at 02:52:57PM +0100, Morten Brørup wrote: > > From: Robin Jarry [mailto:rja...@redhat.com] > > Sent: Friday, 15 November 2024 14.02 > > > > Morten Brørup, Nov 14, 2024 at 15:35: > > > > On that same topic, I wonder if it would make sense to change the API > > parameters to use an opaque rte_ipv4_addr_t type instead of a native > > uint32_t to avoid any confusion. > > It could be considered an IPv4 address type (like the IPv6 address type) > (which should be in network endian), which it is not, so I don't like this > idea. Can you clarify your objection to this idea? For me, the idea of having IPv4 addresses as a 4-byte array seems to offer a lot of advantages over treating it as a single 32-bit value. We don't need to worry about packing or alignment of the values, and everything would always be treated in network-byte order. The main downside I see is compatibility - we'd need a whole new set of definitions and functions in libs to make the change. /Bruce
Re: [PATCH v2] rcu: shift 64-bit constant to avoid implicit 32 to 64 bit conversion
On Wed, Nov 13, 2024 at 5:24 PM Andre Muezerie
wrote:
>
> ../lib/rcu/rte_rcu_qsbr.c(101): warning C4334: '<<': result of 32-bit
> shift implicitly converted to 64 bits (was 64-bit shift intended?)
> ../lib/rcu/rte_rcu_qsbr.c(107): warning C4334: '<<': result of 32-bit
> shift implicitly converted to 64 bits (was 64-bit shift intended?)
> ../lib/rcu/rte_rcu_qsbr.c(145): warning C4334: '<<': result of 32-bit
> shift implicitly converted to 64 bits (was 64-bit shift intended?)
>
> These warnings are being issued by the MSVC compiler. Since the result is
> being stored in a variable of type uint64_t, it makes sense to shift a
> 64-bit number instead of shifting a 32-bit number and then having the
> compiler to convert the result implicitly to 64 bits.
> UINT64_C was used in the fix as it is the portable way to define a 64-bit
> constant (ULL suffix is architecture dependent).
>
> From reading the code this is also a bugfix:
> (1 << id), where id = thread_id & 0x3f, was wrong when thread_id > 0x1f.
>
> Fixes: 64994b56cfd7 ("rcu: add RCU library supporting QSBR mechanism")
>
> Signed-off-by: Andre Muezerie
> Reviewed-by: Honnappa Nagarahalli
> Reviewed-by: Morten Brørup
Just a nit, EAL provides a macro:
lib/eal/include/rte_bitops.h:#define RTE_BIT64(nr) (UINT64_C(1) << (nr))
Does this change allow to build the rcu library with MSVC?
I see it is disabled in meson.
--
David Marchand
Re: [PATCH v5 1/1] graph: improve node layout
Is it good to go?
15/11/2024 02:55, Huichao Cai:
> The members "dispatch" and "xstat_off" of the structure "rte_node"
> can be min cache aligned to make room for future expansion and to
> make sure have better performance. Add corresponding comments.
>
> Signed-off-by: Huichao Cai
> ---
> doc/guides/rel_notes/release_24_11.rst | 2 ++
> lib/graph/rte_graph_worker_common.h| 10 +++---
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/doc/guides/rel_notes/release_24_11.rst
> b/doc/guides/rel_notes/release_24_11.rst
> index 5063badf39..32800e8cb0 100644
> --- a/doc/guides/rel_notes/release_24_11.rst
> +++ b/doc/guides/rel_notes/release_24_11.rst
> @@ -491,6 +491,8 @@ ABI Changes
>added new structure ``rte_node_xstats`` to ``rte_node_register`` and
>added ``xstat_off`` to ``rte_node``.
>
> +* graph: The members ``dispatch`` and ``xstat_off`` of the structure
> ``rte_node`` have been
> + marked as RTE_CACHE_LINE_MIN_SIZE bytes aligned.
>
> Known Issues
>
> diff --git a/lib/graph/rte_graph_worker_common.h
> b/lib/graph/rte_graph_worker_common.h
> index a518af2b2a..d3ec88519d 100644
> --- a/lib/graph/rte_graph_worker_common.h
> +++ b/lib/graph/rte_graph_worker_common.h
> @@ -104,16 +104,20 @@ struct __rte_cache_aligned rte_node {
> /** Original process function when pcap is enabled. */
> rte_node_process_t original_process;
>
> + /** Fast schedule area for mcore dispatch model. */
> union {
> - /* Fast schedule area for mcore dispatch model */
> - struct {
> + alignas(RTE_CACHE_LINE_MIN_SIZE) struct {
> unsigned int lcore_id; /**< Node running lcore. */
> uint64_t total_sched_objs; /**< Number of objects
> scheduled. */
> uint64_t total_sched_fail; /**< Number of scheduled
> failure. */
> } dispatch;
> };
> +
> + /** Fast path area cache line 1. */
> + alignas(RTE_CACHE_LINE_MIN_SIZE)
> rte_graph_off_t xstat_off; /**< Offset to xstat counters. */
> - /* Fast path area */
> +
> + /** Fast path area cache line 2. */
> __extension__ struct __rte_cache_aligned {
> #define RTE_NODE_CTX_SZ 16
> union {
>
[DPDK/ethdev Bug 1580] Nthw: array overrun
https://bugs.dpdk.org/show_bug.cgi?id=1580 Stephen Hemminger (step...@networkplumber.org) changed: What|Removed |Added Resolution|--- |FIXED Status|UNCONFIRMED |RESOLVED --- Comment #2 from Stephen Hemminger (step...@networkplumber.org) --- The scan was done by the owner of the PVS studio blog (not me) and appears to have been rung against github main branch early in 24.11. -- You are receiving this mail because: You are the assignee for the bug.
[PATCH] net/i40e: fix read register return status check
'i40e_get_outer_vlan()' does not check 'i40e_aq_debug_read_register()'
return value. This patch fixes this issue.
Coverity issue: 445518
Fixes: 86eb05d6350b ("net/i40e: add flow validate function")
Cc: beilei.x...@intel.com
Cc: sta...@dpdk.org
Signed-off-by: Vladimir Medvedkin
---
drivers/net/i40e/i40e_flow.c | 78 ++--
1 file changed, 66 insertions(+), 12 deletions(-)
diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c
index c6857727e8..5015eda461 100644
--- a/drivers/net/i40e/i40e_flow.c
+++ b/drivers/net/i40e/i40e_flow.c
@@ -1263,27 +1263,31 @@ i40e_flow_parse_attr(const struct rte_flow_attr *attr,
return 0;
}
-static uint16_t
-i40e_get_outer_vlan(struct rte_eth_dev *dev)
+static int
+i40e_get_outer_vlan(struct rte_eth_dev *dev, uint16_t *tpid)
{
struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
int qinq = dev->data->dev_conf.rxmode.offloads &
RTE_ETH_RX_OFFLOAD_VLAN_EXTEND;
uint64_t reg_r = 0;
uint16_t reg_id;
- uint16_t tpid;
+ int ret;
if (qinq)
reg_id = 2;
else
reg_id = 3;
- i40e_aq_debug_read_register(hw, I40E_GL_SWT_L2TAGCTRL(reg_id),
+ ret = i40e_aq_debug_read_register(hw, I40E_GL_SWT_L2TAGCTRL(reg_id),
®_r, NULL);
+ if (ret != I40E_SUCCESS) {
+ PMD_DRV_LOG(ERR, "Fail to debug read from c[%d]", reg_id);
+ return -EIO;
+ }
- tpid = (reg_r >> I40E_GL_SWT_L2TAGCTRL_ETHERTYPE_SHIFT) & 0x;
+ *tpid = (reg_r >> I40E_GL_SWT_L2TAGCTRL_ETHERTYPE_SHIFT) & 0x;
- return tpid;
+ return 0;
}
/* 1. Last in item should be NULL as range is not supported.
@@ -1303,6 +1307,8 @@ i40e_flow_parse_ethertype_pattern(struct rte_eth_dev *dev,
const struct rte_flow_item_eth *eth_spec;
const struct rte_flow_item_eth *eth_mask;
enum rte_flow_item_type item_type;
+ int ret;
+ uint16_t tpid;
for (; item->type != RTE_FLOW_ITEM_TYPE_END; item++) {
if (item->last) {
@@ -1361,8 +1367,24 @@ i40e_flow_parse_ethertype_pattern(struct rte_eth_dev
*dev,
if (filter->ether_type == RTE_ETHER_TYPE_IPV4 ||
filter->ether_type == RTE_ETHER_TYPE_IPV6 ||
- filter->ether_type == RTE_ETHER_TYPE_LLDP ||
- filter->ether_type == i40e_get_outer_vlan(dev)) {
+ filter->ether_type == RTE_ETHER_TYPE_LLDP) {
+ rte_flow_error_set(error, EINVAL,
+ RTE_FLOW_ERROR_TYPE_ITEM,
+ item,
+ "Unsupported ether_type in"
+ " control packet filter.");
+ return -rte_errno;
+ }
+
+ ret = i40e_get_outer_vlan(dev, &tpid);
+ if (ret != 0) {
+ rte_flow_error_set(error, EIO,
+ RTE_FLOW_ERROR_TYPE_ITEM,
+ item,
+ "Can not get the Ethertype
identifying the L2 tag");
+ return -rte_errno;
+ }
+ if (filter->ether_type == tpid) {
rte_flow_error_set(error, EINVAL,
RTE_FLOW_ERROR_TYPE_ITEM,
item,
@@ -1370,6 +1392,7 @@ i40e_flow_parse_ethertype_pattern(struct rte_eth_dev *dev,
" control packet filter.");
return -rte_errno;
}
+
break;
default:
break;
@@ -1641,6 +1664,7 @@ i40e_flow_parse_fdir_pattern(struct rte_eth_dev *dev,
bool outer_ip = true;
uint8_t field_idx;
int ret;
+ uint16_t tpid;
memset(off_arr, 0, sizeof(off_arr));
memset(len_arr, 0, sizeof(len_arr));
@@ -1709,14 +1733,29 @@ i40e_flow_parse_fdir_pattern(struct rte_eth_dev *dev,
ether_type =
rte_be_to_cpu_16(eth_spec->hdr.ether_type);
if (ether_type == RTE_ETHER_TYPE_IPV4 ||
- ether_type == RTE_ETHER_TYPE_IPV6 ||
- ether_type == i40e_get_outer_vlan(dev)) {
+ ether_type == RTE_ETHER_TYPE_IPV6) {
rte_flow_error_set(error, EINVAL,
RTE_FLOW_ERROR_TYPE_ITEM,
[PATCH v3 08/10] app/test-pmd: remove redundant condition
The loop over policy actions will always exit when it sees
the flow end action, so the next check is redundant.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: f29fa2c59b85 ("app/testpmd: support policy actions per color")
Cc: haif...@nvidia.com
Signed-off-by: Stephen Hemminger
Acked-by: Bruce Richardson
---
app/test-pmd/config.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 88770b4dfc..32c4e86c84 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2288,7 +2288,7 @@ port_meter_policy_add(portid_t port_id, uint32_t
policy_id,
for (act_n = 0, start = act;
act->type != RTE_FLOW_ACTION_TYPE_END; act++)
act_n++;
- if (act_n && act->type == RTE_FLOW_ACTION_TYPE_END)
+ if (act_n > 0)
policy.actions[i] = start;
else
policy.actions[i] = NULL;
@@ -7316,4 +7316,3 @@ show_mcast_macs(portid_t port_id)
printf(" %s\n", buf);
}
}
-
--
2.45.2
[PATCH v3 10/10] app/test-dma-perf: fix parsing of DMA address
There was useless loop when looking at the DMA address.
It looks like it was meant to skip whitespace before
calling strtok.
Good time to replace strtok with strtok_r as well.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: 623dc9364dc6 ("app/dma-perf: introduce DMA performance test")
Cc: cheng1.ji...@intel.com
Cc: sta...@dpdk.org
Signed-off-by: Stephen Hemminger
Acked-by: Bruce Richardson
---
app/test-dma-perf/main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/app/test-dma-perf/main.c b/app/test-dma-perf/main.c
index 18219918cc..8d46b1258b 100644
--- a/app/test-dma-perf/main.c
+++ b/app/test-dma-perf/main.c
@@ -229,8 +229,8 @@ parse_lcore_dma(struct test_configure *test_case, const
char *value)
return -1;
addrs = input;
- while (*addrs == '\0')
- addrs++;
+ while (isspace(*addrs))
+ ++addrs;
if (*addrs == '\0') {
fprintf(stderr, "No input DMA addresses\n");
ret = -1;
--
2.45.2
[PATCH v3 07/10] test/eal: fix core check in c flag test
The expression for checking which lcore is enabled for 0-7
was wrong (missing case for 6).
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: b0209034f2bb ("test/eal: check number of cores before running subtests")
Cc: msant...@redhat.com
Cc: sta...@dpdk.org
Signed-off-by: Stephen Hemminger
Acked-by: Bruce Richardson
---
app/test/test_eal_flags.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
index d37d6b8627..e32f83d3c8 100644
--- a/app/test/test_eal_flags.c
+++ b/app/test/test_eal_flags.c
@@ -677,8 +677,8 @@ test_missing_c_flag(void)
if (rte_lcore_is_enabled(0) && rte_lcore_is_enabled(1) &&
rte_lcore_is_enabled(2) && rte_lcore_is_enabled(3) &&
- rte_lcore_is_enabled(3) && rte_lcore_is_enabled(5) &&
- rte_lcore_is_enabled(4) && rte_lcore_is_enabled(7) &&
+ rte_lcore_is_enabled(4) && rte_lcore_is_enabled(5) &&
+ rte_lcore_is_enabled(6) && rte_lcore_is_enabled(7) &&
launch_proc(argv29) != 0) {
printf("Error - "
"process did not run ok with valid corelist value\n");
--
2.45.2
[PATCH v3 06/10] app/test: fix operator precedence bug
The test loop was much shorter than desired because when
MAX_NUM is defined with out paren's the divide operator /
takes precedence over shift.
But when MAX_NUM is fixed, some tests take too long
and have to be modified to avoid running over full N^2
space of 1<<20.
Note: this is a very old bug, goes back to 2013.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: 1fb8b07ee511 ("app: add some tests")
Cc: sta...@dpdk.org
Signed-off-by: Stephen Hemminger
Acked-by: Bruce Richardson
---
app/test/test_common.c | 31 +--
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/app/test/test_common.c b/app/test/test_common.c
index 21eb2285e1..6dbd7fc9a9 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -9,11 +9,12 @@
#include
#include
#include
+#include
#include
#include "test.h"
-#define MAX_NUM 1 << 20
+#define MAX_NUM (1 << 20)
#define FAIL(x)\
{printf(x "() test failed!\n");\
@@ -218,19 +219,21 @@ test_align(void)
}
}
- for (p = 1; p <= MAX_NUM / 2; p++) {
- for (i = 1; i <= MAX_NUM / 2; i++) {
- val = RTE_ALIGN_MUL_CEIL(i, p);
- if (val % p != 0 || val < i)
- FAIL_ALIGN("RTE_ALIGN_MUL_CEIL", i, p);
- val = RTE_ALIGN_MUL_FLOOR(i, p);
- if (val % p != 0 || val > i)
- FAIL_ALIGN("RTE_ALIGN_MUL_FLOOR", i, p);
- val = RTE_ALIGN_MUL_NEAR(i, p);
- if (val % p != 0 || ((val != RTE_ALIGN_MUL_CEIL(i, p))
- & (val != RTE_ALIGN_MUL_FLOOR(i, p
- FAIL_ALIGN("RTE_ALIGN_MUL_NEAR", i, p);
- }
+ /* testing the whole space of 2^20^2 takes too long. */
+ for (j = 1; j <= MAX_NUM ; j++) {
+ i = rte_rand_max(MAX_NUM - 1) + 1;
+ p = rte_rand_max(MAX_NUM - 1) + 1;
+
+ val = RTE_ALIGN_MUL_CEIL(i, p);
+ if (val % p != 0 || val < i)
+ FAIL_ALIGN("RTE_ALIGN_MUL_CEIL", i, p);
+ val = RTE_ALIGN_MUL_FLOOR(i, p);
+ if (val % p != 0 || val > i)
+ FAIL_ALIGN("RTE_ALIGN_MUL_FLOOR", i, p);
+ val = RTE_ALIGN_MUL_NEAR(i, p);
+ if (val % p != 0 || ((val != RTE_ALIGN_MUL_CEIL(i, p))
+& (val != RTE_ALIGN_MUL_FLOOR(i, p
+ FAIL_ALIGN("RTE_ALIGN_MUL_NEAR", i, p);
}
return 0;
--
2.45.2
[PATCH v3 01/10] app/test: do not duplicate loop variable
Do not use same variable for outer and inner loop in bonding test.
Since the loop is just freeing the resulting burst use bulk free.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: 92073ef961ee ("bond: unit tests")
Cc: declan.dohe...@intel.com
Cc: sta...@dpdk.org
Signed-off-by: Stephen Hemminger
Acked-by: Bruce Richardson
---
app/test/test_link_bonding.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/app/test/test_link_bonding.c b/app/test/test_link_bonding.c
index 4d54706c21..805613d7dd 100644
--- a/app/test/test_link_bonding.c
+++ b/app/test/test_link_bonding.c
@@ -2288,12 +2288,7 @@ test_activebackup_rx_burst(void)
}
/* free mbufs */
- for (i = 0; i < MAX_PKT_BURST; i++) {
- if (rx_pkt_burst[i] != NULL) {
- rte_pktmbuf_free(rx_pkt_burst[i]);
- rx_pkt_burst[i] = NULL;
- }
- }
+ rte_pktmbuf_free_bulk(rx_pkt_burst, burst_size);
/* reset bonding device stats */
rte_eth_stats_reset(test_params->bonding_port_id);
--
2.45.2
[PATCH v3 02/10] app/test: fix typo in address compare
The first argument of 'memcmp' function was equal to the second argument.
Therefore ASSERT would always be true.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: 92073ef961ee ("bond: unit tests")
Cc: declan.dohe...@intel.com
Cc: sta...@dpdk.org
Signed-off-by: Stephen Hemminger
---
app/test/test_link_bonding.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/app/test/test_link_bonding.c b/app/test/test_link_bonding.c
index 805613d7dd..b752a5ecbf 100644
--- a/app/test/test_link_bonding.c
+++ b/app/test/test_link_bonding.c
@@ -792,7 +792,7 @@ test_set_primary_member(void)
&read_mac_addr),
"Failed to get mac address (port %d)",
test_params->bonding_port_id);
- TEST_ASSERT_SUCCESS(memcmp(&read_mac_addr, &read_mac_addr,
+ TEST_ASSERT_SUCCESS(memcmp(expected_mac_addr, &read_mac_addr,
sizeof(read_mac_addr)),
"bonding port mac address not set to that of
primary port\n");
--
2.45.2
[PATCH v3 03/10] app/test: fix paren typo
The parenthesis were in the wrong place so that comparison
took precedence over assignment in handling IPv6 extension
headers. Break up the loop condition to avoid the problem.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: 15ccc647526e ("test/security: test inline reassembly with multi-segment")
Cc: ndabilpu...@marvell.com
Cc: sta...@dpdk.org
Signed-off-by: Stephen Hemminger
Acked-by: Bruce Richardson
---
app/test/test_security_inline_proto_vectors.h | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/app/test/test_security_inline_proto_vectors.h
b/app/test/test_security_inline_proto_vectors.h
index b3d724bac6..86dfa54777 100644
--- a/app/test/test_security_inline_proto_vectors.h
+++ b/app/test/test_security_inline_proto_vectors.h
@@ -519,10 +519,12 @@ test_vector_payload_populate(struct
ip_reassembly_test_packet *pkt,
if (extra_data_sum) {
proto = hdr->proto;
p += sizeof(struct rte_ipv6_hdr);
- while (proto != IPPROTO_FRAGMENT &&
- (proto = rte_ipv6_get_next_ext(p, proto,
&ext_len) >= 0))
+ while (proto != IPPROTO_FRAGMENT) {
+ proto = rte_ipv6_get_next_ext(p, proto,
&ext_len);
+ if (proto < 0)
+ break;
p += ext_len;
-
+ }
/* Found fragment header, update the frag
offset */
if (proto == IPPROTO_FRAGMENT) {
frag_ext = (struct
rte_ipv6_fragment_ext *)p;
--
2.45.2
[PATCH v3 05/10] app/test: fix TLS zero length record
The code was duplicating the same condition three times?
Reading the commit message, the intention was:
Add unit tests to verify the zero len TLS records. Zero len packets are
allowed when content type is app data while zero packet length with
other content type (such as handshake) would result in an error.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: 79a58624369a ("test/security: verify zero length TLS records")
Cc: vvelum...@marvell.com
Cc: sta...@dpdk.org
Signed-off-by: Stephen Hemminger
---
app/test/test_cryptodev.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index c647baeee1..a33ef574cc 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -12253,10 +12253,7 @@ test_tls_record_proto_all(const struct
tls_record_test_flags *flags)
if (flags->skip_sess_destroy && sec_session_outb == NULL)
sec_session_outb = ut_params->sec_session;
- if (flags->zero_len &&
- ((flags->content_type ==
TLS_RECORD_TEST_CONTENT_TYPE_HANDSHAKE) ||
- (flags->content_type ==
TLS_RECORD_TEST_CONTENT_TYPE_HANDSHAKE) ||
- (flags->content_type ==
TLS_RECORD_TEST_CONTENT_TYPE_HANDSHAKE))) {
+ if (flags->zero_len && flags->content_type !=
TLS_RECORD_TEST_CONTENT_TYPE_APP) {
if (ret == TEST_SUCCESS)
return TEST_FAILED;
goto skip_decrypt;
--
2.45.2
[PATCH v3 00/10] bug fixes for unit tests
Recent blog post from PVS studio referenced lots bugs found by their analyzer against DPDK. This set addresses the ones in the test suite. v3 - minimize change to test-dma-perf Stephen Hemminger (10): app/test: do not duplicate loop variable app/test: fix typo in address compare app/test: fix paren typo app/test: avoid duplicate initialization app/test: fix TLS zero length record app/test: fix operator precedence bug test/eal: fix core check in c flag test app/test-pmd: remove redundant condition app/test-pmd: avoid potential outside of array reference app/test-dma-perf: fix parsing of DMA address app/test-dma-perf/main.c | 4 +-- app/test-pmd/cmdline_flow.c | 2 +- app/test-pmd/config.c | 3 +- app/test/test_common.c| 31 ++- app/test/test_cryptodev.c | 5 +-- app/test/test_eal_flags.c | 4 +-- app/test/test_event_crypto_adapter.c | 24 ++ app/test/test_link_bonding.c | 9 ++ app/test/test_security_inline_proto_vectors.h | 8 +++-- 9 files changed, 41 insertions(+), 49 deletions(-) -- 2.45.2
Re: rte_fib network order bug
On Fri, 15 Nov 2024 15:28:33 +0100 "Robin Jarry" wrote: > Morten Brørup, Nov 15, 2024 at 14:52: > > Robin, you've totally won me over on this endian discussion. :-) > > Especially the IPv6 comparison make it clear why IPv4 should also be > > network byte order. > > > > API/ABI stability is a pain... we're stuck with host endian IPv4 > > addresses; e.g. for the RTE_IPV4() macro, which I now agree produces > > the wrong endian value (on little endian CPUs). > > At least for 24.11 it is too late. But maybe we could make it right for > the next LTS? > > >> Vladimir, could we at least consider adding a real network order mode > >> for the rib and fib libraries? So that we can have consistent APIs > >> between IPv4 and IPv6? > > > > And/or rename RTE_FIB_F_NETWORK_ORDER to > > RTE_FIB_F_NETWORK_ORDER_LOOKUP or similar. This is important if real > > network order mode is added (now or later)! > > Maybe we could revert that patch and defer a complete change of the > rib/fib APIs to only expose network order addresses? It would be an ABI > breakage but if properly announced in advance, it should be possible. > > Thinking about it some more. Having a flag for such a drastic change in > behaviour does not seem right. It was a mistake for DPDK to define its own data structures for IP addresses. Would have been much better to stick with the legacy what BSD, Linux (and Windows) uses in API. 'struct in_addr' and 'struct in6_addr' Reinvention did not help users.
RE: [EXTERNAL] Re: [PATCH v5 1/1] graph: improve node layout
> -Original Message-
> From: Thomas Monjalon
> Sent: Friday, November 15, 2024 7:54 PM
> To: Jerin Jacob ; Nithin Kumar Dabilpuram
>
> Cc: Kiran Kumar Kokkilagadda ;
> yanzhirun_...@163.com; dev@dpdk.org; Huichao Cai
> Subject: [EXTERNAL] Re: [PATCH v5 1/1] graph: improve node layout
>
> Is it good to go? 15/11/2024 02: 55, Huichao Cai: > The members "dispatch"
> and "xstat_off" of the structure "rte_node" > can be min cache aligned to make
> room for future expansion and to > make sure have better performance. Add
> corresponding
> Is it good to go?
>
>
> 15/11/2024 02:55, Huichao Cai:
> > The members "dispatch" and "xstat_off" of the structure "rte_node"
> > can be min cache aligned to make room for future expansion and to make
> > sure have better performance. Add corresponding comments.
> >
> > Signed-off-by: Huichao Cai ]
Acked-by: Jerin Jacob
> > ---
> > doc/guides/rel_notes/release_24_11.rst | 2 ++
> > lib/graph/rte_graph_worker_common.h| 10 +++---
> > 2 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/doc/guides/rel_notes/release_24_11.rst
> > b/doc/guides/rel_notes/release_24_11.rst
> > index 5063badf39..32800e8cb0 100644
> > --- a/doc/guides/rel_notes/release_24_11.rst
> > +++ b/doc/guides/rel_notes/release_24_11.rst
> > @@ -491,6 +491,8 @@ ABI Changes
> >added new structure ``rte_node_xstats`` to ``rte_node_register`` and
> >added ``xstat_off`` to ``rte_node``.
> >
> > +* graph: The members ``dispatch`` and ``xstat_off`` of the structure
> > +``rte_node`` have been
> > + marked as RTE_CACHE_LINE_MIN_SIZE bytes aligned.
> >
> > Known Issues
> >
> > diff --git a/lib/graph/rte_graph_worker_common.h
> > b/lib/graph/rte_graph_worker_common.h
> > index a518af2b2a..d3ec88519d 100644
> > --- a/lib/graph/rte_graph_worker_common.h
> > +++ b/lib/graph/rte_graph_worker_common.h
> > @@ -104,16 +104,20 @@ struct __rte_cache_aligned rte_node {
> > /** Original process function when pcap is enabled. */
> > rte_node_process_t original_process;
> >
> > + /** Fast schedule area for mcore dispatch model. */
> > union {
> > - /* Fast schedule area for mcore dispatch model */
> > - struct {
> > + alignas(RTE_CACHE_LINE_MIN_SIZE) struct {
> > unsigned int lcore_id; /**< Node running lcore. */
> > uint64_t total_sched_objs; /**< Number of objects
> scheduled. */
> > uint64_t total_sched_fail; /**< Number of scheduled
> failure. */
> > } dispatch;
> > };
> > +
> > + /** Fast path area cache line 1. */
> > + alignas(RTE_CACHE_LINE_MIN_SIZE)
> > rte_graph_off_t xstat_off; /**< Offset to xstat counters. */
> > - /* Fast path area */
> > +
> > + /** Fast path area cache line 2. */
> > __extension__ struct __rte_cache_aligned { #define RTE_NODE_CTX_SZ
> > 16
> > union {
> >
>
>
>
>
Re: [PATCH] dts: remove redundant test suite
Reviewed-by: Luca Vizzarro
Re: [PATCH v2] eal/linux: fix fbarray name with multiple secondary processes
On Fri, 15 Nov 2024 15:50:08 +0800 Congjie Zhou wrote: > diff --git a/lib/eal/linux/eal_memalloc.c b/lib/eal/linux/eal_memalloc.c > index e354efc..367d401 100644 > --- a/lib/eal/linux/eal_memalloc.c > +++ b/lib/eal/linux/eal_memalloc.c > @@ -16,6 +16,7 @@ > #include > #include > #include > +#include > #ifdef F_ADD_SEALS /* if file sealing is supported, so is memfd */ > #include > #define MEMFD_SUPPORTED > @@ -1447,8 +1448,8 @@ secondary_msl_create_walk(const struct rte_memseg_list > *msl, > local_msl = &local_memsegs[msl_idx]; > > /* create distinct fbarrays for each secondary */ > - snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i", > - primary_msl->memseg_arr.name, getpid()); > + snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i_%"PRIx64, > + primary_msl->memseg_arr.name, getpid(), rte_get_tsc_cycles()); > > ret = rte_fbarray_init(&local_msl->memseg_arr, name, > primary_msl->memseg_arr.len, Need to include to get the prototype rte_get_tsc_cycles()
[PATCH v3 04/10] app/test: avoid duplicate initialization
The event_dev_config initialization had duplicate assignments
to the same element. Change to use structure initialization
so that compiler will catch this type of bug.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: f8f9d233ea0e ("test/eventdev: add unit tests")
Cc: jerin.ja...@caviumnetworks.com
Signed-off-by: Stephen Hemminger
Acked-by: Bruce Richardson
---
app/test/test_event_crypto_adapter.c | 24 ++--
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/app/test/test_event_crypto_adapter.c
b/app/test/test_event_crypto_adapter.c
index 9d38a66bfa..ab24e30a97 100644
--- a/app/test/test_event_crypto_adapter.c
+++ b/app/test/test_event_crypto_adapter.c
@@ -1154,21 +1154,17 @@ configure_cryptodev(void)
static inline void
evdev_set_conf_values(struct rte_event_dev_config *dev_conf,
- struct rte_event_dev_info *info)
+ const struct rte_event_dev_info *info)
{
- memset(dev_conf, 0, sizeof(struct rte_event_dev_config));
- dev_conf->dequeue_timeout_ns = info->min_dequeue_timeout_ns;
- dev_conf->nb_event_ports = NB_TEST_PORTS;
- dev_conf->nb_event_queues = NB_TEST_QUEUES;
- dev_conf->nb_event_queue_flows = info->max_event_queue_flows;
- dev_conf->nb_event_port_dequeue_depth =
- info->max_event_port_dequeue_depth;
- dev_conf->nb_event_port_enqueue_depth =
- info->max_event_port_enqueue_depth;
- dev_conf->nb_event_port_enqueue_depth =
- info->max_event_port_enqueue_depth;
- dev_conf->nb_events_limit =
- info->max_num_events;
+ *dev_conf = (struct rte_event_dev_config) {
+ .dequeue_timeout_ns = info->min_dequeue_timeout_ns,
+ .nb_event_ports = NB_TEST_PORTS,
+ .nb_event_queues = NB_TEST_QUEUES,
+ .nb_event_queue_flows = info->max_event_queue_flows,
+ .nb_event_port_dequeue_depth =
info->max_event_port_dequeue_depth,
+ .nb_event_port_enqueue_depth =
info->max_event_port_enqueue_depth,
+ .nb_events_limit = info->max_num_events,
+ };
}
static int
--
2.45.2
Re: [PATCH v3 08/10] app/test-pmd: remove redundant condition
On Fri, Nov 15, 2024 at 12:08 PM Stephen Hemminger
wrote:
>
> The loop over policy actions will always exit when it sees
> the flow end action, so the next check is redundant.
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: f29fa2c59b85 ("app/testpmd: support policy actions per color")
> Cc: haif...@nvidia.com
> Signed-off-by: Stephen Hemminger
> Acked-by: Bruce Richardson
Acked-by: Ajit Khaparde
> ---
> app/test-pmd/config.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 88770b4dfc..32c4e86c84 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2288,7 +2288,7 @@ port_meter_policy_add(portid_t port_id, uint32_t
> policy_id,
> for (act_n = 0, start = act;
> act->type != RTE_FLOW_ACTION_TYPE_END; act++)
> act_n++;
> - if (act_n && act->type == RTE_FLOW_ACTION_TYPE_END)
> + if (act_n > 0)
> policy.actions[i] = start;
> else
> policy.actions[i] = NULL;
> @@ -7316,4 +7316,3 @@ show_mcast_macs(portid_t port_id)
> printf(" %s\n", buf);
> }
> }
> -
> --
> 2.45.2
>
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH v2] eal/linux: fix fbarray name with multiple secondary processes
On Fri, 15 Nov 2024 15:50:08 +0800 Congjie Zhou wrote: > add the tsc into the name. > > Suggested-by: Stephen Hemminger > Signed-off-by: Congjie Zhou > --- > > When multiple secondary processes run in different containers, names > identified by PIDs are not unique due to the pid namespace. > So Add tsc to redefine a unique name. > > v1: use monotonic time to redefine the name > v2: use tsc to redefine the name > > lib/eal/linux/eal_memalloc.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/lib/eal/linux/eal_memalloc.c b/lib/eal/linux/eal_memalloc.c > index e354efc..367d401 100644 > --- a/lib/eal/linux/eal_memalloc.c > +++ b/lib/eal/linux/eal_memalloc.c > @@ -16,6 +16,7 @@ > #include > #include > #include > +#include > #ifdef F_ADD_SEALS /* if file sealing is supported, so is memfd */ > #include > #define MEMFD_SUPPORTED > @@ -1447,8 +1448,8 @@ secondary_msl_create_walk(const struct rte_memseg_list > *msl, > local_msl = &local_memsegs[msl_idx]; > > /* create distinct fbarrays for each secondary */ > - snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i", > - primary_msl->memseg_arr.name, getpid()); > + snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i_%"PRIx64, > + primary_msl->memseg_arr.name, getpid(), rte_get_tsc_cycles()); > Worst case name is now: 'memseg-1048576k-128-128-1234567890-' = 51 characters Acked-by: Stephen Hemminger
Re: [PATCH v2 10/10] app/test-dma-perf: fix parsing of DMA address
On Fri, 15 Nov 2024 09:13:12 +
Bruce Richardson wrote:
> On Thu, Nov 14, 2024 at 11:25:08AM -0800, Stephen Hemminger wrote:
> > There was useless loop when looking at the DMA address.
> > It looks like it was meant to skip whitespace before
> > calling strtok.
> >
> > Good time to replace strtok with strtok_r as well.
> >
> > Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
> >
> > Fixes: 623dc9364dc6 ("app/dma-perf: introduce DMA performance test")
> > Cc: cheng1.ji...@intel.com
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Stephen Hemminger
>
> One comment inline below. With that fixed:
>
> Acked-by: Bruce Richardson
>
> > ---
> > app/test-dma-perf/main.c | 12 ++--
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/app/test-dma-perf/main.c b/app/test-dma-perf/main.c
> > index 18219918cc..dccb0a3541 100644
> > --- a/app/test-dma-perf/main.c
> > +++ b/app/test-dma-perf/main.c
> > @@ -217,27 +217,27 @@ parse_lcore_dma(struct test_configure *test_case,
> > const char *value)
> > struct lcore_dma_map_t *lcore_dma_map;
> > char *input, *addrs;
> > char *ptrs[2];
> > - char *start, *end, *substr;
> > + char *start, *end, *substr, *saveptr;
> > uint16_t lcore_id;
> > int ret = 0;
> >
> > if (test_case == NULL || value == NULL)
> > return -1;
> >
> > - input = strndup(value, strlen(value) + 1);
> > + input = strdup(value);
> > if (input == NULL)
> > return -1;
> > - addrs = input;
> >
> > - while (*addrs == '\0')
> > - addrs++;
> > + addrs = input;
> > + while (isspace(*addrs))
> > + ++addrs;
> > if (*addrs == '\0') {
> > fprintf(stderr, "No input DMA addresses\n");
> > ret = -1;
> > goto out;
> > }
> >
> > - substr = strtok(addrs, ",");
> > + substr = strtok_r(addrs, ",", &saveptr);
>
> Don't need to use strtok here at all. Just use strchr, and then no need for
> a new temporary variable.
The issue is that the test is passing each comma seperated section to
rte_strsplit
and that needs the first part to be null terminated.
But using strtok_r in only one spot is wrong, will fix.
Re: [PATCH v2 07/10] test/eal: fix core check in c flag test
On Fri, 15 Nov 2024 09:07:42 +
Bruce Richardson wrote:
> On Thu, Nov 14, 2024 at 11:25:05AM -0800, Stephen Hemminger wrote:
> > The expression for checking which lcore is enabled for 0-7
> > was wrong (missing case for 6).
> >
> > Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
> >
> > Fixes: b0209034f2bb ("test/eal: check number of cores before running
> > subtests")
> > Cc: msant...@redhat.com
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Stephen Hemminger
>
> Just wondering would it not be better/safer to put in an actual loop check
> here?
> However, I'm also ok with keeping the fix as-is, so:
>
> Acked-by: Bruce Richardson
My goal was to do minimum changes for now, to avoid introducing new bugs.
[PATCH v3] eal/linux: fix fbarray name with multiple secondary processes
add the tsc into the name. Suggested-by: Stephen Hemminger Signed-off-by: Congjie Zhou --- When multiple secondary processes run in different containers, names identified by PIDs are not unique due to the pid namespace. So Add tsc to redefine a unique name. v1: use monotonic time to redefine the name v2: use tsc to redefine the name v3: include to support all architectures lib/eal/linux/eal_memalloc.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/eal/linux/eal_memalloc.c b/lib/eal/linux/eal_memalloc.c index e354efc..ae1e024 100644 --- a/lib/eal/linux/eal_memalloc.c +++ b/lib/eal/linux/eal_memalloc.c @@ -16,6 +16,7 @@ #include #include #include +#include #ifdef F_ADD_SEALS /* if file sealing is supported, so is memfd */ #include #define MEMFD_SUPPORTED @@ -31,6 +32,7 @@ #include #include #include +#include #include "eal_filesystem.h" #include "eal_internal_cfg.h" @@ -1447,8 +1449,8 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl, local_msl = &local_memsegs[msl_idx]; /* create distinct fbarrays for each secondary */ - snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i", - primary_msl->memseg_arr.name, getpid()); + snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i_%"PRIx64, + primary_msl->memseg_arr.name, getpid(), rte_get_tsc_cycles()); ret = rte_fbarray_init(&local_msl->memseg_arr, name, primary_msl->memseg_arr.len, -- 2.34.1
