Re: [PATCH] app/testpmd: fix memory leak for dscp table
Reviewed-by: Sean Morrissey
Thanks.
On 28/06/2022 14:29, Jasvinder Singh wrote:
This patch fixes memory leak reported by coverity.
Coverity issue: 379220
Fixes: 9f5488e326d3 ("app/testpmd: support different input color method")
Cc: sta...@dpdk.org
Signed-off-by: Jasvinder Singh
---
app/test-pmd/cmdline_mtr.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/app/test-pmd/cmdline_mtr.c b/app/test-pmd/cmdline_mtr.c
index b92e66cedb..833273da0d 100644
--- a/app/test-pmd/cmdline_mtr.c
+++ b/app/test-pmd/cmdline_mtr.c
@@ -131,8 +131,10 @@ parse_input_color_table_entries(char *str, enum rte_color
**dscp_table,
/* Allocate memory for vlan table */
vlan = (enum rte_color *)malloc(MAX_VLAN_TABLE_ENTRIES *
sizeof(enum rte_color));
- if (vlan == NULL)
+ if (vlan == NULL) {
+ free(*dscp_table);
return -1;
+ }
i = 0;
while (1) {
@@ -144,6 +146,7 @@ parse_input_color_table_entries(char *str, enum rte_color
**dscp_table,
vlan[i++] = RTE_COLOR_RED;
else {
free(vlan);
+ free(*dscp_table);
return -1;
}
if (i == MAX_VLAN_TABLE_ENTRIES)
@@ -152,6 +155,7 @@ parse_input_color_table_entries(char *str, enum rte_color
**dscp_table,
token = strtok_r(str, PARSE_DELIMITER, &str);
if (token == NULL) {
free(vlan);
+ free(*dscp_table);
return -1;
}
}
Re: [dpdk-dev] [PATCH v3 4/5] lib/kvargs: remove unneeded header includes
On 15/10/2021 10:00, Olivier Matz wrote: Hi Sean, On Thu, Oct 07, 2021 at 10:25:56AM +, Sean Morrissey wrote: These header includes have been flagged by the iwyu_tool and removed. Signed-off-by: Sean Morrissey --- lib/kvargs/rte_kvargs.c | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/kvargs/rte_kvargs.c b/lib/kvargs/rte_kvargs.c index 38e9d5c1ca..4cce8e953b 100644 --- a/lib/kvargs/rte_kvargs.c +++ b/lib/kvargs/rte_kvargs.c @@ -7,7 +7,6 @@ #include #include -#include #include #include "rte_kvargs.h" -- 2.25.1 Did you check that it still compiles for the Windows platform after this change? +CC Dmitry Hi Olivier, I cross-compiled with MinGW-64 after this change and it still compiled. Thanks, Sean.
Re: [dpdk-dev] [PATCH v1 1/5] devtools: script to remove unused headers includes
On 06/10/2021 14:37, Thomas Monjalon wrote: 04/10/2021 12:10, Sean Morrissey: This script can be used for removing headers flagged for removal by the include-what-you-use (IWYU) tool. The script has the ability to remove headers from specified sub-directories or dpdk as a whole. My fear is that it could flag an include as useless, while it is required for a different system (BSD, Windows). Hi Thomas, Stephen raised a similar concern. Please see Bruce's reply here: https://patches.dpdk.org/project/dpdk/cover/20211004101058.2396458-1-sean.morris...@intel.com/ Thanks, Sean.
Re: [PATCH] rcu: fix build failure with debug dp log level
On 29/08/2022 17:55, Honnappa Nagarahalli wrote:
-Original Message-
From: Anoob Joseph
Sent: Monday, August 29, 2022 11:52 AM
To: Honnappa Nagarahalli
Cc: jer...@marvell.com; dev@dpdk.org; sean.morris...@intel.com
Subject: [PATCH] rcu: fix build failure with debug dp log level
Build fails if RTE_LOG_DP_LEVEL is set to RTE_LOG_DEBUG. Fix the same by
including the required header when RTE_LOG_DP_LEVEL is set to
RTE_LOG_DEBUG.
../lib/rcu/rte_rcu_qsbr.h:678:40: error: expected ‘)’ before ‘PRIu64’
678 |"%s: status: least acked token = %" PRIu64,
|^~
Fixes: 30a1de105a5f ("lib: remove unneeded header includes")
Cc: sean.morris...@intel.com
Agree on the fix.
@sean.morris...@intel.com Does the process that removed this header file
inclusion needs fixing?
If yes, should that fix be included in this patch?
@honnappa.nagaraha...@arm.com Yes, I believe the tool will need an
update, however, I believe it should be a separate patch for that.
Signed-off-by: Anoob Joseph
---
lib/rcu/rte_rcu_qsbr.h | 4
1 file changed, 4 insertions(+)
diff --git a/lib/rcu/rte_rcu_qsbr.h b/lib/rcu/rte_rcu_qsbr.h index
d81bf5e8db..b0f1720ca1 100644
--- a/lib/rcu/rte_rcu_qsbr.h
+++ b/lib/rcu/rte_rcu_qsbr.h
@@ -37,6 +37,10 @@ extern "C" {
#include
#include
+#if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG
+#include
+#endif
+
extern int rte_rcu_log_type;
#if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG
--
2.25.1
Re: [PATCH v3] dmadev: add telemetry support
On 01/04/2022 12:00, Walsh, Conor wrote:
From: Bruce Richardson
Sent: Friday 1 April 2022 11:50
To: Morrissey, Sean
Cc: Chengwen Feng ; Laatz, Kevin
; dev@dpdk.org; Pai G, Sunil
Subject: Re: [PATCH v3] dmadev: add telemetry support
On Fri, Apr 01, 2022 at 10:24:02AM +, Sean Morrissey wrote:
Telemetry commands are now registered through the dmadev library
for the gathering of DSA stats. The corresponding callback
functions for listing dmadevs and providing info and stats for a
specific dmadev are implemented in the dmadev library.
An example usage can be seen below:
Connecting to /var/run/dpdk/rte/dpdk_telemetry.v2
{"version": "DPDK 22.03.0-rc2", "pid": 2956551, "max_output_len": 16384}
Connected to application: "dpdk-dma"
--> /
{"/": ["/", "/dmadev/info", "/dmadev/list", "/dmadev/stats", ...]}
--> /dmadev/list
{"/dmadev/list": [0, 1]}
--> /dmadev/info,0
{"/dmadev/info": {"name": ":00:01.0", "nb_vchans": 1, "numa_node":
0,
"max_vchans": 1, "max_desc": 4096, "min_desc": 32, "max_sges": 0,
"capabilities": {"fill": 1, "sva": 0, "silent": 0, ...}}}
--> /dmadev/stats,0,0
{"/dmadev/stats": {"submitted": 0, "completed": 0, "errors": 0}}
Signed-off-by: Sean Morrissey
Tested-by: Sunil Pai G
Reviewed-by: Bruce Richardson
Hi Sean,
I'd agree with Bruce's comment below about trying to keep the names the same.
Looks good to me though and I've tested it with IOAT and dmafwd.
Thanks,
Reviewed-by: Conor Walsh
One comment inline below, which I'd like feedback from others on.
---
V3:
* update docs with correct examples
* code cleanup and added comments
+
+#define ADD_CAPA(c, s) rte_tel_data_add_dict_int(dma_caps, #c,
!!(dev_capa & RTE_DMA_CAPA_ ## s))
+
+static int
+dmadev_handle_dev_info(const char *cmd __rte_unused,
+ const char *params, struct rte_tel_data *d)
+{
+ struct rte_dma_info dma_info;
+ struct rte_tel_data *dma_caps;
+ dma_caps = rte_tel_data_alloc();
+ if (!dma_caps)
+ return -ENOMEM;
+
+ rte_tel_data_start_dict(dma_caps);
+ ADD_CAPA(fill, OPS_FILL);
+ ADD_CAPA(sva, SVA);
+ ADD_CAPA(silent, SILENT);
+ ADD_CAPA(copy, OPS_COPY);
+ ADD_CAPA(mem2mem, MEM_TO_MEM);
I'm not 100% sure about this approach of having slightly different names
compared to the flags, just to have things in lower-case. Looking to have
some more input here - I'd tend to have the capabilities in upper case to
avoid duplicating parameters, but I'm not massively concerned either way.
Hi all,
If that is the preferred approach then I will send another version. I
got the lower case
names from the capa_names struct in the dma_capability_name() function
and these
naming conventions are also used in the logs i.e. "Device %d don't
support mem2mem transfer".
For this reason, I thought this was the preferred approach to naming the
capabilities, however
I will keep the names consistent with the flags as suggested.
+ ADD_CAPA(mem2dev, MEM_TO_DEV);
+ ADD_CAPA(dev2mem, DEV_TO_MEM);
+ ADD_CAPA(dev2dev, DEV_TO_DEV);
+ ADD_CAPA(copy_sg, OPS_COPY_SG);
+ ADD_CAPA(handles_errors, HANDLES_ERRORS);
+ rte_tel_data_add_dict_container(d, "capabilities", dma_caps, 0);
+
+ return 0;
+}
Re: [PATCH v4] examples/l3fwd: merge l3fwd-acl into l3fwd
Hi Konstantin,
Comment on some of your feedback below. I will make the rest of the
changes and send a new version.
On 08/04/2022 18:26, Ananyev, Konstantin wrote:
Hi Sean,
Few nits, that I didn't spot previously, pls see below.
+
+/* Setup ACL context. 8< */
Looks like some typo within comments.
I believe these characters are needed in the comments to state the start
and end of the automated code snippets for the docs.
+static struct rte_acl_ctx*
+app_acl_init(struct rte_acl_rule *route_base,
+ struct rte_acl_rule *acl_base, unsigned int route_num,
+ unsigned int acl_num, int ipv6, int socketid)
+{
+ char name[PATH_MAX];
+ struct rte_acl_param acl_param;
+ struct rte_acl_config acl_build_param;
+ struct rte_acl_ctx *context;
+ int dim = ipv6 ? RTE_DIM(ipv6_defs) : RTE_DIM(ipv4_defs);
+
+ /* Create ACL contexts */
+ snprintf(name, sizeof(name), "%s%d",
+ ipv6 ? L3FWD_ACL_IPV6_NAME : L3FWD_ACL_IPV4_NAME,
+ socketid);
+
+ acl_param.name = name;
+ acl_param.socket_id = socketid;
+ acl_param.rule_size = RTE_ACL_RULE_SZ(dim);
+ acl_param.max_rule_num = MAX_ACL_RULE_NUM;
+
+ context = rte_acl_create(&acl_param);
+ if (context == NULL)
+ rte_exit(EXIT_FAILURE, "Failed to create ACL context\n");
+
+ if (parm_config.alg != RTE_ACL_CLASSIFY_DEFAULT &&
+ rte_acl_set_ctx_classify(context, parm_config.alg) != 0)
+ rte_exit(EXIT_FAILURE,
+ "Failed to setup classify method for ACL context\n");
+
+ if (rte_acl_add_rules(context, route_base, route_num) < 0)
+ rte_exit(EXIT_FAILURE, "add rules failed\n");
+
+ if (rte_acl_add_rules(context, acl_base, acl_num) < 0)
+ rte_exit(EXIT_FAILURE, "add rules failed\n");
+
+ /* Perform builds */
+ memset(&acl_build_param, 0, sizeof(acl_build_param));
+
+ acl_build_param.num_categories = DEFAULT_MAX_CATEGORIES;
+ acl_build_param.num_fields = dim;
+ memcpy(&acl_build_param.defs, ipv6 ? ipv6_defs : ipv4_defs,
+ ipv6 ? sizeof(ipv6_defs) : sizeof(ipv4_defs));
+
+ if (rte_acl_build(context, &acl_build_param) != 0)
+ rte_exit(EXIT_FAILURE, "Failed to build ACL trie\n");
+
+ rte_acl_dump(context);
+
+ return context;
+}
+/* >8 End of ACL context setup. */
Typo in comments.
Same as above.
Re: [PATCH v1 18/19] timer: remove unneeded header includes
On 21/04/2022 21:08, Stephen Hemminger wrote: On Thu, 21 Apr 2022 19:08:58 + Sean Morrissey wrote: diff --git a/lib/timer/rte_timer.c b/lib/timer/rte_timer.c index c51a393e5c..f52ccc33ed 100644 --- a/lib/timer/rte_timer.c +++ b/lib/timer/rte_timer.c @@ -5,12 +5,9 @@ #include #include #include -#include #include -#include #include -#include #include #include #include This doesn't look right. rte_timer.c relies on rte_get_timer_cycles() which is defined in rte_cycles.h Perhaps iwyu is getting confused, or thinking that is already covered by another include file? IWYU can throw false positives. Please let me fix all build issues with this patchset and get back to you. It could re-introduce the header in question. If not I will investigate where IWYU believes the include is coming from. Typically the headers removed by this tool are usually already included from a daisy chain of includes from another header.
Re: [dpdk-dev] [PATCH] lib/librte_eal: fix unrecongized telemetry eal arg
+Harry Van Haaren -- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Re: [dpdk-dev] [PATCH v2] Revert "eal: fix parsing option --telemetry"
Subject: Re: [PATCH v2] Revert "eal: fix parsing option --telemetry" 24/07/2019 17:20, Sean Morrissey: > This reverts commit debacba0297fbe214b4185a9791e6a9fdf6642ba. > > Reverting this patch as it currently breaks the initialization of > telemetry, more investigation is ongoing to fix the issue for the > printed error message for unrecognized argument. > > Signed-off-by: Sean Morrissey It's very disapointing. Did you or the reviewer tested the previous patch? The reporter of the bug tested this and verified functionality and closed the internal bug. -- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Re: [dpdk-dev] [PATCH v2] Revert "eal: fix parsing option --telemetry"
Subject: Re: [PATCH v2] Revert "eal: fix parsing option --telemetry" 24/07/2019 18:28, Morrissey, Sean: > > Subject: Re: [PATCH v2] Revert "eal: fix parsing option --telemetry" > > 24/07/2019 17:20, Sean Morrissey: > > This reverts commit debacba0297fbe214b4185a9791e6a9fdf6642ba. > > > > Reverting this patch as it currently breaks the initialization of > > telemetry, more investigation is ongoing to fix the issue for the > > printed error message for unrecognized argument. > > > > Signed-off-by: Sean Morrissey > > It's very disapointing. > Did you or the reviewer tested the previous patch? > > The reporter of the bug tested this and verified functionality and closed the > internal bug. So the reverted commit is supposed to work? I won't apply this revert until I fully understand what happens. The patch that I sent up was to remove an "unrecognized option telemetry" warning message, which the patch removed and was tested to ensure the message was removed. Further testing, after the patch was sent up, revealed that the unix domain socket, which is required by telemetry consumers, was no longer being created, rendering the telemetry functionality non-functional. On further investigation, the full fix involves a change in the EAL command line parameter handling, which is probably too risky for RC3. This revert will allow telemetry to function again, but with the erroneous message still in place. We will aim to fix in the next release. -- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
