Re: vmxnet3 no longer functional on DPDK 21.11
On 11/29/2021 8:45 PM, Lewis Donzis wrote: Hello. We just upgraded from 21.08 to 21.11 and it's rather astounding the number of incompatible changes in three months. Not a big deal, just kind of a surprise, that's all. Anyway, the problem is that the vmxnet3 driver is no longer functional on FreeBSD. In drivers/net/vmxnet3/vmxnet3_ethdev.c, vmxnet3_dev_start() gets an error calling rte_intr_enable(). So it logs "interrupt enable failed" and returns an error. In lib/eal/freebsd/eal_interrupts.c, rte_intr_enable() is returning an error because rte_intr_dev_fd_get(intr_handle) is returning -1. I don't see how that could ever return anything other than -1 since it appears that there is no code that ever calls rte_intr_dev_fd_set() with a value other than -1 on FreeBSD. Also weird to me is that even if it didn't get an error, the switch statement that follows looks like it will return an error in every case. Nonetheless, it worked in 21.08, and I can't quite see why the difference, so I must be missing something. For the moment, I just commented the "return -EIO" in vmxnet3_ethdev.c, and it's now working again, but that's obviously not the correct solution. Can someone who's knowledgable about this mechanism perhaps explain a little bit about what's going on? I'll be happy to help troubleshoot. It seems like it must be something simple, but I just don't see it yet. Thanks, lew +Yong
Re: [PATCH] net/vmxnet3: add spinlocks to register command access
On 11/30/2021 7:31 AM, Yong Wang wrote: -Original Message- From:"sahithi.sin...@oracle.com" Date: Monday, November 8, 2021 at 12:23 AM To: Yong Wang Cc:"dev@dpdk.org" , Sahithi Singam Subject: [PATCH] net/vmxnet3: add spinlocks to register command access From: Sahithi Singam At present, there are no spinlocks around register command access. This resulted in a race condition when two threads running on two different cores invoked link_update function at the same time to get link status. Due to this race condition, one of the threads reported false link status value. Signed-off-by: Sahithi Singam --- Thanks Sahithi for the patch. As we discussed offline, in DPDK, the expectation is that control level synchronization should be handled by the application. In my knowledge, currently no PMD guarantee such synchronization at driver callback level. It makes more sense to have the application manages the synchronization as most likely it needs to work with multiple PMDs and it's better to keep this behavior consistent across all PMDs (i.e, it does not make a lot of sense to support this behavior only in one particular PMD). ack, updating patch status as rejected.
[PATCH] dma/idxd: add allow/block list support
Add support for allow or block list for devices bound
to the kernel driver.
When used the allow or block list applies as an additional
condition to the name prefix.
Signed-off-by: Radu Nicolau
---
doc/guides/dmadevs/idxd.rst | 10 ++
drivers/dma/idxd/idxd_bus.c | 30 ++
2 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/doc/guides/dmadevs/idxd.rst b/doc/guides/dmadevs/idxd.rst
index d4a210b854..6201d4db1d 100644
--- a/doc/guides/dmadevs/idxd.rst
+++ b/doc/guides/dmadevs/idxd.rst
@@ -117,6 +117,16 @@ the value used as the DPDK ``--file-prefix`` parameter may
be used as a workqueu
name prefix, instead of ``dpdk_``, allowing each DPDK application instance to
only
use a subset of configured queues.
+Additionally, the -a (allowlist) or -b (blocklist) commandline parameters are
+also available to further restrict the device list that will be used.
+If the -a option is used then any device that passes the ``dpdk_`` or
+``--file-prefix`` prefix condition must also be present in the allow list.
+Similarly, when the block list is used any device that passes the prefix
+condition must not be in the block list.
+For example, to only use ``wq0.3``, assuming the name prefix condition is met::
+
+ $ dpdk-test -a wq0.3
+
Once probed successfully, irrespective of kernel driver, the device will
appear as a ``dmadev``,
that is a "DMA device type" inside DPDK, and can be accessed using APIs from
the
``rte_dmadev`` library.
diff --git a/drivers/dma/idxd/idxd_bus.c b/drivers/dma/idxd/idxd_bus.c
index 08639e9dce..13cb967f6d 100644
--- a/drivers/dma/idxd/idxd_bus.c
+++ b/drivers/dma/idxd/idxd_bus.c
@@ -9,6 +9,7 @@
#include
#include
+#include
#include
#include
#include
@@ -244,8 +245,18 @@ idxd_probe_dsa(struct rte_dsa_device *dev)
return 0;
}
+static int search_devargs(const char *name)
+{
+ struct rte_devargs *devargs;
+ RTE_EAL_DEVARGS_FOREACH(dsa_bus.bus.name, devargs) {
+ if (strcmp(devargs->name, name) == 0)
+ return 1;
+ }
+ return 0;
+}
+
static int
-is_for_this_process_use(const char *name)
+is_for_this_process_use(struct rte_dsa_device *dev, const char *name)
{
char *runtime_dir = strdup(rte_eal_get_runtime_dir());
char *prefix = basename(runtime_dir);
@@ -257,6 +268,13 @@ is_for_this_process_use(const char *name)
if (strncmp(name, prefix, prefixlen) == 0 && name[prefixlen] == '_')
retval = 1;
+ if (retval && dsa_bus.bus.conf.scan_mode != RTE_BUS_SCAN_UNDEFINED) {
+ if (dsa_bus.bus.conf.scan_mode == RTE_BUS_SCAN_ALLOWLIST)
+ retval = search_devargs(dev->device.name);
+ else
+ retval = !search_devargs(dev->device.name);
+ }
+
free(runtime_dir);
return retval;
}
@@ -273,7 +291,8 @@ dsa_probe(void)
read_wq_string(dev, "name", name, sizeof(name))
< 0)
continue;
- if (strncmp(type, "user", 4) == 0 &&
is_for_this_process_use(name)) {
+ if (strncmp(type, "user", 4) == 0 &&
+ is_for_this_process_use(dev, name)) {
dev->device.driver = &dsa_bus.driver;
idxd_probe_dsa(dev);
continue;
@@ -370,8 +389,11 @@ dsa_addr_parse(const char *name, void *addr)
return -1;
}
- wq->device_id = device_id;
- wq->wq_id = wq_id;
+ if (wq != NULL) {
+ wq->device_id = device_id;
+ wq->wq_id = wq_id;
+ }
+
return 0;
}
--
2.25.1
Re: [PATCH] dma/idxd: add allow/block list support
On Tue, Nov 30, 2021 at 09:54:39AM +, Radu Nicolau wrote: > Add support for allow or block list for devices bound > to the kernel driver. > When used the allow or block list applies as an additional > condition to the name prefix. > > Signed-off-by: Radu Nicolau Reviewed-by: Bruce Richardson Acked-by: Bruce Richardson Minor comments inline below. > --- > doc/guides/dmadevs/idxd.rst | 10 ++ > drivers/dma/idxd/idxd_bus.c | 30 ++ > 2 files changed, 36 insertions(+), 4 deletions(-) > > diff --git a/doc/guides/dmadevs/idxd.rst b/doc/guides/dmadevs/idxd.rst > index d4a210b854..6201d4db1d 100644 > --- a/doc/guides/dmadevs/idxd.rst > +++ b/doc/guides/dmadevs/idxd.rst > @@ -117,6 +117,16 @@ the value used as the DPDK ``--file-prefix`` parameter > may be used as a workqueu > name prefix, instead of ``dpdk_``, allowing each DPDK application instance > to only > use a subset of configured queues. > > +Additionally, the -a (allowlist) or -b (blocklist) commandline parameters are > +also available to further restrict the device list that will be used. > +If the -a option is used then any device that passes the ``dpdk_`` or Should probably have a comma after "used", and then you can split the line at that point, having the rest of the sentence on one line. I believe longer lines are allowed for docs so long as we split them at punctuation points. > +``--file-prefix`` prefix condition must also be present in the allow list. > +Similarly, when the block list is used any device that passes the prefix > +condition must not be in the block list. Similarly for this sentence, can put comma and break after "used". > +For example, to only use ``wq0.3``, assuming the name prefix condition is > met:: > + > + $ dpdk-test -a wq0.3 > + > Once probed successfully, irrespective of kernel driver, the device will > appear as a ``dmadev``, > that is a "DMA device type" inside DPDK, and can be accessed using APIs from > the > ``rte_dmadev`` library.
Re: vmxnet3 no longer functional on DPDK 21.11
On Mon, Nov 29, 2021 at 02:45:15PM -0600, Lewis Donzis wrote: >Hello. >We just upgraded from 21.08 to 21.11 and it's rather astounding the >number of incompatible changes in three months. Not a big deal, just >kind of a surprise, that's all. >Anyway, the problem is that the vmxnet3 driver is no longer functional >on FreeBSD. >In drivers/net/vmxnet3/vmxnet3_ethdev.c, vmxnet3_dev_start() gets an >error calling rte_intr_enable(). So it logs "interrupt enable failed" >and returns an error. >In lib/eal/freebsd/eal_interrupts.c, rte_intr_enable() is returning an >error because rte_intr_dev_fd_get(intr_handle) is returning -1. >I don't see how that could ever return anything other than -1 since it >appears that there is no code that ever calls rte_intr_dev_fd_set() >with a value other than -1 on FreeBSD. Also weird to me is that even >if it didn't get an error, the switch statement that follows looks like >it will return an error in every case. >Nonetheless, it worked in 21.08, and I can't quite see why the >difference, so I must be missing something. >For the moment, I just commented the "return -EIO" in vmxnet3_ethdev.c, >and it's now working again, but that's obviously not the correct >solution. >Can someone who's knowledgable about this mechanism perhaps explain a >little bit about what's going on? I'll be happy to help troubleshoot. >It seems like it must be something simple, but I just don't see it yet. Hi if you have the chance, it would be useful if you could use "git bisect" to identify the commit in 21.11 that broke this driver. Looking through the logs for 21.11 I can't identify any particular likely-looking commit, so bisect is likely a good way to start looking into this. Regards, /Bruce
[Bug 892] librte.sched scheduler can undershoot target rate
https://bugs.dpdk.org/show_bug.cgi?id=892 Bug ID: 892 Summary: librte.sched scheduler can undershoot target rate Product: DPDK Version: 21.11 Hardware: All OS: All Status: UNCONFIRMED Severity: normal Priority: Normal Component: core Assignee: dev@dpdk.org Reporter: mike.ev...@microsoft.com Target Milestone: --- As per the comments, we found that the scheduler can significantly undershoot the target rate, particularly if tc_period is set to a low value, the packet size is large and the bitrate is fairly low. As examples, testing with tc_period = 10ms, packet size of 1538 bytes and both pipe and TC bitrates set to 1875000 bytes/s (1.5Mbps) we saw the target rate undershot by ~20%. The problem appears to be in grinder_credits_update, where TC credits are refreshed. Because the update simply sets the tc_credits to tc_credits_per_period, any residual credits are lost. In our test scenario, tc_period is 10ms, and so tc_credits_per_period is 1875. This means that in each tc_period the scheduler only sends one 1538 byte packet and the residual 337 bytes of credit are lost on the next refresh. This means we send 100 packets per second, whereas we would expect either 121 or 122 packets per second. It's not clear if the behaviour is intentional and the choice of 10ms for tc_credit is a poor one. It seems sensible not to let TC credit grow unbounded when it is not being used, but isn't that the purpose of the tb_size parameter? If this is intentional then it means shaping at the TC is very different to shaping at the pipe or subport levels. -- You are receiving this mail because: You are the assignee for the bug.
Bonding driver and LACP systemID
Two of the major characteristic of Ethernet bundles (LACP) are availability
and flexibility: bundles resist to a single port failure, and ports can be
added and removed from bundles without affecting the overall bundle
availability.
Most devices that support Ethernet bundles set the "System ID" of the bundle
(the bundle identifier in LACP) to a MAC address that is different from the
MAC addresses of the ports part of the bundle, just because ports can be
added and removed, so re-using an existing MAC address can generate
duplicates if the specific port is removed from the bundle.
DPDK bonding driver has the possibility to set bonding MAC address
(rte_eth_bond_mac_address_set), but this is not used as the SystemID: the
SystemID is always set to the "aggregator" port mac address, that is the mac
of the first port added to the bundle.
When that port is removed from the bundle, this automatically reconfigures
itself with a different aggregator port, and so it changes its unique
identifier in the LACP packets. Therefore the counterpart shuts the "old"
bundle down as nobody is sending anymore that id. It will set up a new one
with the new identifier, but in any case there is a connectivity
interruption. So I bring the whole bundle down by just removing a single
port. This is contrary to the main characteristic of bundles.
I could not find a way to keep the bonding up while removing the aggregator
port. Is it possible?
What I succeeded doing is make sure that if a bonding mac address is
specified, this one is used as SystemID in LACP packets (thus allowing a
smooth removal of the aggregator port). If none is specified then the
current behaviour (aggregator port) is maintained.
But I did this changing the driver code; DPDK 21.11 changes to achieve this:
--- rte_eth_bond_8023ad.c
+++ rte_eth_bond_8023ad.c.new
@@ -866,7 +866,7 @@ bond_mode_8023ad_periodic_cb(void *arg)
struct bond_dev_private *internals =
bond_dev->data->dev_private;
struct port *port;
struct rte_eth_link link_info;
- struct rte_ether_addr slave_addr;
+ struct rte_ether_addr bond_addr;
struct rte_mbuf *lacp_pkt = NULL;
uint16_t slave_id;
uint16_t i;
@@ -893,7 +893,7 @@ bond_mode_8023ad_periodic_cb(void *arg)
key = 0;
}
- rte_eth_macaddr_get(slave_id, &slave_addr);
+rte_eth_macaddr_get(bond_dev->data->port_id,
&bond_addr);
port = &bond_mode_8023ad_ports[slave_id];
key = rte_cpu_to_be_16(key);
@@ -905,8 +905,8 @@ bond_mode_8023ad_periodic_cb(void *arg)
SM_FLAG_SET(port, NTT);
}
- if
(!rte_is_same_ether_addr(&port->actor.system, &slave_addr)) {
-rte_ether_addr_copy(&slave_addr,
&port->actor.system);
+if
(!rte_is_same_ether_addr(&port->actor.system, &bond_addr)) {
+ rte_ether_addr_copy(&bond_addr,
&port->actor.system);
if (port->aggregator_port_id ==
slave_id)
SM_FLAG_SET(port,
NTT);
}
@@ -1186,7 +1186,7 @@ bond_mode_8023ad_mac_address_update(stru
if (rte_is_same_ether_addr(&slave_addr,
&slave->actor.system))
continue;
- rte_ether_addr_copy(&slave_addr,
&slave->actor.system);
+rte_ether_addr_copy(bond_dev->data->mac_addrs,
&slave->actor.system);
/* Do nothing if this port is not an
aggregator. In other case
* Set NTT flag on every port that use this
aggregator. */
if (slave->aggregator_port_id != slave_id)
So back to the question: is there an easier way to remove the aggregator
port from a bundle without affecting the overall bundle availability? Or is
a patch required?
Thanks
[PATCH] net/bnxt: fix xstats get names implementation
When the xstats_names parameter to rte_eth_xstats_get_names()
is non-NULL and the size parameter is less than the required
number of entries, the driver must return the required size
without modifying (and over-running) the caller's xstats_names
array.
Update bnxt_dev_xstats_get_names_op() in accordance with this
requirement.
Fixes: bfb9c2260be2 ("net/bnxt: support xstats get/reset")
Cc: sta...@dpdk.org
Signed-off-by: Lance Richardson
---
drivers/net/bnxt/bnxt_stats.c | 93 +--
1 file changed, 46 insertions(+), 47 deletions(-)
diff --git a/drivers/net/bnxt/bnxt_stats.c b/drivers/net/bnxt/bnxt_stats.c
index 991eafc644..197fd7c02b 100644
--- a/drivers/net/bnxt/bnxt_stats.c
+++ b/drivers/net/bnxt/bnxt_stats.c
@@ -846,7 +846,7 @@ int bnxt_flow_stats_cnt(struct bnxt *bp)
int bnxt_dev_xstats_get_names_op(struct rte_eth_dev *eth_dev,
struct rte_eth_xstat_name *xstats_names,
- __rte_unused unsigned int limit)
+ unsigned int size)
{
struct bnxt *bp = (struct bnxt *)eth_dev->data->dev_private;
const unsigned int stat_cnt = RTE_DIM(bnxt_rx_stats_strings) +
@@ -862,63 +862,62 @@ int bnxt_dev_xstats_get_names_op(struct rte_eth_dev
*eth_dev,
if (rc)
return rc;
- if (xstats_names != NULL) {
- count = 0;
+ if (xstats_names == NULL || size < stat_cnt)
+ return stat_cnt;
- for (i = 0; i < RTE_DIM(bnxt_rx_stats_strings); i++) {
- strlcpy(xstats_names[count].name,
- bnxt_rx_stats_strings[i].name,
- sizeof(xstats_names[count].name));
- count++;
- }
+ for (i = 0; i < RTE_DIM(bnxt_rx_stats_strings); i++) {
+ strlcpy(xstats_names[count].name,
+ bnxt_rx_stats_strings[i].name,
+ sizeof(xstats_names[count].name));
+ count++;
+ }
- for (i = 0; i < RTE_DIM(bnxt_tx_stats_strings); i++) {
- strlcpy(xstats_names[count].name,
- bnxt_tx_stats_strings[i].name,
- sizeof(xstats_names[count].name));
- count++;
- }
+ for (i = 0; i < RTE_DIM(bnxt_tx_stats_strings); i++) {
+ strlcpy(xstats_names[count].name,
+ bnxt_tx_stats_strings[i].name,
+ sizeof(xstats_names[count].name));
+ count++;
+ }
- for (i = 0; i < RTE_DIM(bnxt_func_stats_strings); i++) {
- strlcpy(xstats_names[count].name,
- bnxt_func_stats_strings[i].name,
- sizeof(xstats_names[count].name));
- count++;
- }
+ for (i = 0; i < RTE_DIM(bnxt_func_stats_strings); i++) {
+ strlcpy(xstats_names[count].name,
+ bnxt_func_stats_strings[i].name,
+ sizeof(xstats_names[count].name));
+ count++;
+ }
- for (i = 0; i < RTE_DIM(bnxt_rx_ext_stats_strings); i++) {
- strlcpy(xstats_names[count].name,
- bnxt_rx_ext_stats_strings[i].name,
- sizeof(xstats_names[count].name));
+ for (i = 0; i < RTE_DIM(bnxt_rx_ext_stats_strings); i++) {
+ strlcpy(xstats_names[count].name,
+ bnxt_rx_ext_stats_strings[i].name,
+ sizeof(xstats_names[count].name));
- count++;
- }
+ count++;
+ }
- for (i = 0; i < RTE_DIM(bnxt_tx_ext_stats_strings); i++) {
- strlcpy(xstats_names[count].name,
- bnxt_tx_ext_stats_strings[i].name,
- sizeof(xstats_names[count].name));
+ for (i = 0; i < RTE_DIM(bnxt_tx_ext_stats_strings); i++) {
+ strlcpy(xstats_names[count].name,
+ bnxt_tx_ext_stats_strings[i].name,
+ sizeof(xstats_names[count].name));
- count++;
- }
+ count++;
+ }
- if (bp->fw_cap & BNXT_FW_CAP_ADV_FLOW_COUNTERS &&
- bp->fw_cap & BNXT_FW_CAP_ADV_FLOW_MGMT &&
- BNXT_FLOW_XSTATS_EN(bp)) {
- for (i = 0; i < bp->max_l2_ctx; i++) {
- char buf[RTE_ETH_XSTATS_NAME_SIZE];
+ if (bp->fw_cap & BNXT_FW_CAP_ADV_FLOW_COUNTERS &&
+ bp->fw_cap & BNXT_FW_CAP_ADV_FLOW_MGMT &&
+ BNXT_FLOW_XSTATS_EN(bp)) {
+ for (i = 0; i < bp->max_l2_ctx; i++) {
+ char buf[RTE_ETH_XSTATS_NAME_SIZE];
- sprintf(buf, "flow_%d_bytes", i);
-
[PATCH] pktgen: Fix setting user pattern from cli
When setting the user pattern from the pktgen cli, it would always be incorrectly set to the string 'pattern' regardless of input. This can be demonstrated simply by setting a pattern set 0 pattern user set 0 user pattern asdfasdf Signed-off-by: Josh Knight --- app/cli-functions.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/cli-functions.c b/app/cli-functions.c index 4293444..41d354b 100644 --- a/app/cli-functions.c +++ b/app/cli-functions.c @@ -652,7 +652,7 @@ set_cmd(int argc, char **argv) break; case 25: foreach_port(portlist, -pattern_set_user_pattern(info, argv[3])); +pattern_set_user_pattern(info, argv[4])); break; case 30: p = strchr(argv[4], '/'); -- 2.30.1 (Apple Git-130)
FOSDEM 2022 Networking Devroom CFP
The FOSDEM Networking team is excited to announce that the call for proposals is now open for the Networking devroom at [FOSDEM 2022]. FOSDEM is a free event for engineers to meet, share ideas and collaborate. Please note that, due to Covid-19, FOSDEM will be held virtually again next year on the 5th and 6th of February, 2022. All presentations will be pre-recorded with live chat during each presentation and live Q&A after each presentation. [FOSDEM 2022] https://fosdem.org/2022/ Important Dates === Sunday 12th December Submission deadline Friday 24th December Announcement of selected talks Friday 28th January Recorded Presentation and slide upload deadline 5 & 6th February Conference dates (online) About the devroom = The FOSDEM Networking devroom hosts a diversity of talks covering Open Source networking projects and technologies. The scope of the Networking devroom is broad and targets those software developers engaged in network design, monitoring, management and software development with Open Source tools. We typically host a variety of talks on Networking topics including: - Network monitoring (inventory, discovery & telemetry), - Network management (enterprise, carrier-grade, cloud, IoT) - Networking protocols (the full alphabet soup) - Security (IPS, IDS, protocols) - Authentication - Development tools (libraries, SDKs, plugins) - Network stacks Suggested Topics: = If it moves network packets, monitors, or interacts with, or supervises entities that moves network packets, it is likely a topic the devroom is interested in. A very /non-exhaustive/ list of the projects and topics we would love to see on the schedule are: Monitoring Nagios, Icinga2, Naemon, NTop, SNAS, Graphana, Prometheus, collectd Frameworks DPDK, eBPF, XDP, FD.io, PANDA SwitchingSnabb, OpenvSwitch, Tungsten Fabric, Lagopus, FastClick Stacks Linux, *BSD, F-Stack Controllers Istio, Envoy, Network Service Mesh, OpenDayLight, Calico, Contiv Edge Akraino, Openness Routing FRRouting, BIRD, Go-BGP Management ONAP, Ganglia, NetSNMP, PNDA.io Security Suricata, Snort Tooling VSPerf, TRex, Moongen, Scapy, Warp17 Talks should be aimed at a technical audience, but should not assume that attendees are already familiar with your project or how it solves a given problem. Talk proposals may cover very specific solutions to a problem, or may be a higher level project overview for lesser known projects. Code of Conduct === [FOSDEM Code of Conduct]: We'd like to remind all speakers and attendees that all of the presentations and discussions in our devroom are held under the guidelines set in the CoC and we expect attendees, speakers, and volunteers to follow the CoC at all times. If you submit a proposal and it is accepted, you will be required to confirm that you accept the FOSDEM CoC. If you have any questions about the CoC or wish to have one of the devroom organizers review your presentation slides or any other content for CoC compliance, please email us and we will do our best to assist you. [FOSDEM Code of Conduct] https://fosdem.org/2022/practical/conduct/ Proposals: == Proposals should be made through the [FOSDEM Pentabarf submission tool]. You do not need to create a new Pentabarf account if you already have one from a past year. Please select the "Network" as the *track* and ensure you include the following information when submitting a proposal: Section FieldNotes - Person Name(s) Your first, last and public names. Person Abstract A short bio. Person PhotoPlease provide a photo. EventEvent Title *This is the title of your talk* - please be descriptive to encourage attendance. EventAbstract Short abstract of one or two paragraphs. EventDuration Please indicate the length of your talk; 15 min, 30 min or 45 min If your talk is accepted, the deadline to upload your slides and a pre-recorded version of your talk is Friday 28th January. FOSDEM will be held on the weekend of February 5th & 6th, 2022. Please also join the devroom’s mailing list, which is the official communication channel for the devroom: [network-devr...@lists.fosdem.org] (subscription page) [FOSDEM Pentabarf submission tool] https://penta.fosdem.org/submission/FOSDEM22 [network-devr...@lists.fosdem.org] https://lists.fosdem.org/listinfo/network-devroom Team: = Ray Kinsella Stephan Schmidt Thomas Monjalon Emma Foley Alexander Biehl Charles Eckel
Re: [PATCH] version: 22.03-rc0
29/11/2021 14:16, David Marchand: > Start a new release cycle with empty release notes. > Bump version and ABI minor. > Enable ABI checks using latest libabigail. > > Signed-off-by: David Marchand [...] > - LIBABIGAIL_VERSION: libabigail-1.8 > + LIBABIGAIL_VERSION: libabigail-2.0 What is the reason for this update? Can we still use the old version? Maybe add a small comment in the commit log. Acked-by: Thomas Monjalon Thanks
[PATCH v1 1/1] net/qede: fix redundant condition in debug code
Expression "a && 1" is equivalent to just "a", so fix the accidental
inclusion of a literal in code.
Cc: sta...@dpdk.org
Fixes: ec55c118792b ("net/qede: add infrastructure for debug data collection")
Cc: rm...@marvell.com
Signed-off-by: Anatoly Burakov
---
Notes:
This isn't a bug, this is just a syntactic anomaly, likely a remnant of some
kind of debugging code.
This issue was found with Control Flag [1], which i ran on DPDK codebase
just
out of curiosity. This was the only issue worth addressing that the tool
produced output for.
[1] https://github.com/IntelLabs/control-flag
drivers/net/qede/qede_debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/qede/qede_debug.c b/drivers/net/qede/qede_debug.c
index 2297d245c4..ba807ea680 100644
--- a/drivers/net/qede/qede_debug.c
+++ b/drivers/net/qede/qede_debug.c
@@ -3522,7 +3522,7 @@ static enum dbg_status qed_grc_dump(struct ecore_hwfn
*p_hwfn,
/* Dump MCP HW Dump */
if (qed_grc_is_included(p_hwfn, DBG_GRC_PARAM_DUMP_MCP_HW_DUMP) &&
- !qed_grc_get_param(p_hwfn, DBG_GRC_PARAM_NO_MCP) && 1)
+ !qed_grc_get_param(p_hwfn, DBG_GRC_PARAM_NO_MCP))
offset += qed_grc_dump_mcp_hw_dump(p_hwfn,
p_ptt,
dump_buf + offset, dump);
--
2.25.1
Re: [PATCH] version: 22.03-rc0
On Tue, Nov 30, 2021 at 4:35 PM Thomas Monjalon wrote: > > 29/11/2021 14:16, David Marchand: > > Start a new release cycle with empty release notes. > > Bump version and ABI minor. > > Enable ABI checks using latest libabigail. > > > > Signed-off-by: David Marchand > [...] > > - LIBABIGAIL_VERSION: libabigail-1.8 > > + LIBABIGAIL_VERSION: libabigail-2.0 > > What is the reason for this update? Can we still use the old version? Nothing prevents from using the old version, I just used this chance to bump the version. I talked with Dodji, 2.0 is the version used in Fedora for ABI checks. This version comes with enhancements and at least a fix for a bug we got when writing exception rules in dpdk: https://sourceware.org/bugzilla/show_bug.cgi?id=28060 -- David Marchand
Re: [PATCH v8 10/11] app/test: replace .sh scripts with .py scripts
On Wed, Nov 24, 2021 at 01:15:33AM +0300, Dmitry Kozlyuk wrote:
> 2021-10-25 19:46 (UTC-0700), Jie Zhou:
> > - Add python script to check if system supports hugepages
> > - Remove corresponding .sh scripts
> > - Replace calling of .sh with corresponding .py in meson.build
> >
> > Signed-off-by: Jie Zhou
> > ---
> > app/test/has-hugepage.sh | 11 ---
> > app/test/has_hugepage.py | 25 +
> > app/test/meson.build | 2 +-
> > 3 files changed, 26 insertions(+), 12 deletions(-)
> > delete mode 100755 app/test/has-hugepage.sh
> > create mode 100644 app/test/has_hugepage.py
> >
> > diff --git a/app/test/has-hugepage.sh b/app/test/has-hugepage.sh
> > deleted file mode 100755
> > index d600fad319..00
> > --- a/app/test/has-hugepage.sh
> > +++ /dev/null
> > @@ -1,11 +0,0 @@
> > -#! /bin/sh
> > -# SPDX-License-Identifier: BSD-3-Clause
> > -# Copyright 2020 Mellanox Technologies, Ltd
> > -
> > -if [ "$(uname)" = "Linux" ] ; then
> > - cat /proc/sys/vm/nr_hugepages || echo 0
> > -elif [ "$(uname)" = "FreeBSD" ] ; then
> > - echo 1 # assume FreeBSD always has hugepages
> > -else
> > - echo 0
> > -fi
> > diff --git a/app/test/has_hugepage.py b/app/test/has_hugepage.py
> > new file mode 100644
> > index 00..70107c61fd
> > --- /dev/null
> > +++ b/app/test/has_hugepage.py
> > @@ -0,0 +1,25 @@
> > +# SPDX-License-Identifier: BSD-3-Clause
> > +# Copyright (c) 2021 Microsoft Corporation
> > +"""This script checks if the system supports huge pages"""
> > +
> > +import platform
> > +import ctypes
> > +
> > +osName = platform.system()
>
> Please follow Python's preferred naming style, "os_name".
>
> > +if osName == "Linux":
> > +with open("/proc/sys/vm/nr_hugepages") as file_o:
> > +content = file_o.read()
> > +print(content)
>
> Compared to shell version, lost is the logic to print 0 in case of a failure.
>
> > +elif osName == "FreeBSD":
> > +# Assume FreeBSD always has hugepages enabled
> > +print("1")
> > +elif osName == "Windows":
> > +# On Windows, determine if large page is supported based on the
> > +# value returned by GetLargePageMinimum. If the return value is zero,
> > +# the processor does not support large pages.
>
> I honestly don't see what this comment adds to the code below :)
>
> > +if ctypes.windll.kernel32.GetLargePageMinimum() > 0:
> > +print("1")
> > +else:
> > +print("0")
> > +else:
> > +print("0")
> [...]
Thanks Dmitry. Addressed all in V9.
Re: [PATCH v8 06/11] app/test: temporarily "skip" one cmdline test case
On Wed, Nov 24, 2021 at 01:02:13AM +0300, Dmitry Kozlyuk wrote: > 2021-10-25 19:45 (UTC-0700), Jie Zhou: > > cmdline tests pass except one failure at the test_cmdline_socket_fns > > test case with error: failed to open /dev/null for reading! > > Can't it be something like this? > > #ifndef RTE_EXEC_ENV_WINDOWS > #define NULL_INPUT "/dev/null" > #else > #define NULL_INPUT "NUL" > #endif Yes, it works. Thanks!
Re: [PATCH v8 04/11] app/test: exclude ENOTSUP as failure
On Wed, Nov 24, 2021 at 01:02:10AM +0300, Dmitry Kozlyuk wrote:
> 2021-10-25 19:45 (UTC-0700), Jie Zhou:
> > Check rte_errno to exclude ENOTSUP as failures in test_memory.c
> >
> > Signed-off-by: Jie Zhou
> > ---
> > app/test/test_memory.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test/test_memory.c b/app/test/test_memory.c
> > index dbf6871e71..379b0f99ca 100644
> > --- a/app/test/test_memory.c
> > +++ b/app/test/test_memory.c
> > @@ -10,6 +10,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "test.h"
> >
> > @@ -63,7 +64,7 @@ check_seg_fds(const struct rte_memseg_list *msl, const
> > struct rte_memseg *ms,
> > /* we're able to get memseg fd - try getting its offset */
> > ret = rte_memseg_get_fd_offset_thread_unsafe(ms, &offset);
> > if (ret < 0) {
> > - if (errno == ENOTSUP)
> > + if (rte_errno == ENOTSUP)
> > return 1;
> > return -1;
> > }
>
> The commit title and message contradict what is inside.
> ENOTSUP was excluded previously, but errno was checked instead of rte_errno.
> It should be stated what was wrong and how it is fixed (with a Fixes tag).
Will add the Fixes info and revise the title and message.
Re: [PATCH v8 03/11] app/test: replace POSIX specific code
On Wed, Nov 24, 2021 at 01:02:06AM +0300, Dmitry Kozlyuk wrote:
> 2021-10-25 19:45 (UTC-0700), Jie Zhou:
> > - Include rte_os_shim.h
> > - Replace sleep and usleep with rte_delay_us_sleep
> > - #ifndef RTE_EXEC_ENV_WINDOWS for POSIX code only
> >
> > Signed-off-by: Jie Zhou
> > ---
>
> This patch can be combined with the previous one:
> they serve the same purpose---to remove Unix-specific code.
>
> Please try to summarize in the commit message
> which parts of the tests suites are excluded, e.g. multi-process.
> It is more useful then stating what was changed in the code.
>
Will combine and revise the message.
> [...]
> > diff --git a/app/test/test_cmdline.c b/app/test/test_cmdline.c
> > index 115bee966d..9a76bd299f 100644
> > --- a/app/test/test_cmdline.c
> > +++ b/app/test/test_cmdline.c
> > @@ -31,6 +31,7 @@ test_cmdline(void)
> > return -1;
> > if (test_parse_num_invalid_param() < 0)
> > return -1;
> > +#ifndef RTE_EXEC_ENV_WINDOWS
> > printf("Testing parsing IP addresses...\n");
> > if (test_parse_ipaddr_valid() < 0)
> > return -1;
> > @@ -38,6 +39,7 @@ test_cmdline(void)
> > return -1;
> > if (test_parse_ipaddr_invalid_param() < 0)
> > return -1;
> > +#endif
> > printf("Testing parsing strings...\n");
> > if (test_parse_string_valid() < 0)
> > return -1;
>
> What's wrong with parsing IP addresses on Windows?
>
test_cmdline_ipaddr.c uses linux netinet/in.h specific u6_addr. Skip these 3
cases for now and prefer a separate patch to make it work on Windows. Or maybe
there is already DPDK support on this which I am not aware of? Thanks.
> [...]
> > diff --git a/app/test/test_mp_secondary.c b/app/test/test_mp_secondary.c
> > index 5b6f05dbb1..da035348bd 100644
> > --- a/app/test/test_mp_secondary.c
> > +++ b/app/test/test_mp_secondary.c
> > @@ -14,7 +14,9 @@
> > #include
> > #include
> > #include
> > +#ifndef RTE_EXEC_ENV_WINDOWS
> > #include
> > +#endif
> > #include
> > #include
> > #include
>
> is absent on Windows for sure, but you don't exclude it.
> Does this file even need modification?
> It's not going to be compiled for Windows.
>
This is replaced with the test stub in the patch#11 (of V8) to make it compile
on Windows. Sorry for the confusion. Will make sure remove this unnecessary
part from V9.
> [...]
> > diff --git a/app/test/test_ring_stress.c b/app/test/test_ring_stress.c
> > index 1af45e0fc8..ce3535c6b2 100644
> > --- a/app/test/test_ring_stress.c
> > +++ b/app/test/test_ring_stress.c
> > @@ -43,9 +43,10 @@ test_ring_stress(void)
> > n += test_ring_rts_stress.nb_case;
> > k += run_test(&test_ring_rts_stress);
> >
> > +#ifndef RTE_EXEC_ENV_WINDOWS
> > n += test_ring_hts_stress.nb_case;
> > k += run_test(&test_ring_hts_stress);
> > -
> > +#endif
>
> Can you please elaborate what is the issue with this case?
> It is also one of the details you usually want to put
> into the commit message.
Cannot remember what caused this case being skipped in the first place, but it
can work now. So removed the ifndef. Thanks.
Re: [PATCH v8 03/11] app/test: replace POSIX specific code
2021-11-30 17:05 (UTC-0800), Jie Zhou:
> On Wed, Nov 24, 2021 at 01:02:06AM +0300, Dmitry Kozlyuk wrote:
[...]
> > [...]
> > > diff --git a/app/test/test_cmdline.c b/app/test/test_cmdline.c
> > > index 115bee966d..9a76bd299f 100644
> > > --- a/app/test/test_cmdline.c
> > > +++ b/app/test/test_cmdline.c
> > > @@ -31,6 +31,7 @@ test_cmdline(void)
> > > return -1;
> > > if (test_parse_num_invalid_param() < 0)
> > > return -1;
> > > +#ifndef RTE_EXEC_ENV_WINDOWS
> > > printf("Testing parsing IP addresses...\n");
> > > if (test_parse_ipaddr_valid() < 0)
> > > return -1;
> > > @@ -38,6 +39,7 @@ test_cmdline(void)
> > > return -1;
> > > if (test_parse_ipaddr_invalid_param() < 0)
> > > return -1;
> > > +#endif
> > > printf("Testing parsing strings...\n");
> > > if (test_parse_string_valid() < 0)
> > > return -1;
> >
> > What's wrong with parsing IP addresses on Windows?
> >
> test_cmdline_ipaddr.c uses linux netinet/in.h specific u6_addr. Skip these
> 3 cases for now and prefer a separate patch to make it work on Windows. Or
> maybe there is already DPDK support on this which I am not aware of? Thanks.
Understood, only please explain this in the commit log.
