On 13/10/2023 10:07, jm...@redhat.com wrote:
From: Jakob Meng <c...@jakobmeng.de>
For better usability, the function pairs get_config() and
set_config() for netdevs should be symmetric: Options which are
accepted by set_config() should be returned by get_config() and the
latter should output valid options for set_config() only.
This patch moves key-value pairs which are no valid options from
get_config() to the get_status() callback. For example, get_config()
in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
previously. For requested rx queues the proper option name is n_rxq,
so requested_rx_queues has been renamed respectively. Tx queues
cannot be changed by the user, hence requested_tx_queues has been
dropped. Both configured_{rx,tx}_queues will be returned as
n_{r,t}xq in the get_status() callback.
The documentation in vswitchd/vswitch.xml for status columns as well
as tests have been updated accordingly.
Reported-at: https://bugzilla.redhat.com/1949855
Signed-off-by: Jakob Meng <c...@jakobmeng.de>
---
Documentation/topics/dpdk/phy.rst | 4 +-
lib/netdev-dpdk.c | 104 +++++++++++++++++++++---------
tests/system-dpdk.at | 64 +++++++++++-------
vswitchd/vswitch.xml | 14 +++-
4 files changed, 127 insertions(+), 59 deletions(-)
diff --git a/Documentation/topics/dpdk/phy.rst
b/Documentation/topics/dpdk/phy.rst
index f66b106c4..41cc3588a 100644
--- a/Documentation/topics/dpdk/phy.rst
+++ b/Documentation/topics/dpdk/phy.rst
@@ -198,7 +198,7 @@ Example::
a dedicated queue, it will be explicit::
$ ovs-vsctl get interface dpdk-p0 status
- {..., rx_steering=unsupported}
+ {..., rx-steering=unsupported}
More details can often be found in ``ovs-vswitchd.log``::
@@ -499,7 +499,7 @@ its options::
$ ovs-appctl dpctl/show
[...]
- port 3: dpdk-rep0 (dpdk: configured_rx_queues=1, ...,
dpdk-vf-mac=00:11:22:33:44:55, ...)
+ port 3: dpdk-rep0 (dpdk: ..., dpdk-vf-mac=00:11:22:33:44:55, ...)
$ ovs-vsctl show
[...]
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 55700250d..56588f92a 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1905,31 +1905,32 @@ netdev_dpdk_get_config(const struct netdev *netdev,
struct smap *args)
ovs_mutex_lock(&dev->mutex);
- smap_add_format(args, "requested_rx_queues", "%d", dev->user_n_rxq);
- smap_add_format(args, "configured_rx_queues", "%d", netdev->n_rxq);
- smap_add_format(args, "requested_tx_queues", "%d", dev->requested_n_txq);
- smap_add_format(args, "configured_tx_queues", "%d", netdev->n_txq);
- smap_add_format(args, "mtu", "%d", dev->mtu);
+ smap_add_format(args, "dpdk-devargs", "%s", dev->devargs);
+ smap_add_format(args, "n_rxq", "%d", dev->user_n_rxq);
+ smap_add_format(args, "rx-flow-ctrl", "%s",
+ dev->fc_conf.mode == RTE_ETH_FC_TX_PAUSE ||
+ dev->fc_conf.mode == RTE_ETH_FC_FULL ? "true" : "false");
+ smap_add_format(args, "tx-flow-ctrl", "%s",
+ dev->fc_conf.mode == RTE_ETH_FC_RX_PAUSE ||
+ dev->fc_conf.mode == RTE_ETH_FC_FULL ? "true" : "false");
+ smap_add_format(args, "flow-ctrl-autoneg", "%s",
+ dev->fc_conf.autoneg ? "true" : "false");
- if (dev->type == DPDK_DEV_ETH) {
- smap_add_format(args, "n_rxq_desc", "%d", dev->rxq_size);
- smap_add_format(args, "n_txq_desc", "%d", dev->txq_size);
- if (dev->hw_ol_features & NETDEV_RX_CHECKSUM_OFFLOAD) {
- smap_add(args, "rx_csum_offload", "true");
- } else {
- smap_add(args, "rx_csum_offload", "false");
- }
- if (dev->rx_steer_flags == DPDK_RX_STEER_LACP) {
- smap_add(args, "rx-steering", "rss+lacp");
- }
- smap_add(args, "lsc_interrupt_mode",
- dev->lsc_interrupt_mode ? "true" : "false");
+ smap_add_format(args, "n_rxq_desc", "%d", dev->rxq_size);
+ smap_add_format(args, "n_txq_desc", "%d", dev->txq_size);
- if (dpdk_port_is_representor(dev)) {
- smap_add_format(args, "dpdk-vf-mac", ETH_ADDR_FMT,
- ETH_ADDR_ARGS(dev->requested_hwaddr));
- }
+ if (dev->rx_steer_flags == DPDK_RX_STEER_LACP) {
+ smap_add(args, "rx-steering", "rss+lacp");
+ }
+
+ smap_add(args, "dpdk-lsc-interrupt",
+ dev->lsc_interrupt_mode ? "true" : "false");
nit: indent
+
+ if (dpdk_port_is_representor(dev)) {
+ smap_add_format(args, "dpdk-vf-mac", ETH_ADDR_FMT,
+ ETH_ADDR_ARGS(dev->requested_hwaddr));
}
+
ovs_mutex_unlock(&dev->mutex);
return 0;
@@ -2324,6 +2325,29 @@ out:
return err;
}
+static int
+netdev_dpdk_vhost_client_get_config(const struct netdev *netdev,
+ struct smap *args)
+{
+ struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+
+ ovs_mutex_lock(&dev->mutex);
+
+ if (dev->vhost_id) {
+ smap_add(args, "vhost-server-path", dev->vhost_id);
+ }
+
+ int tx_retries_max;
I'm not sure it breaks any hard rule, but it seems more consistent to
put this ^ at the start of the function.
+ atomic_read_relaxed(&dev->vhost_tx_retries_max, &tx_retries_max);
+ if (tx_retries_max != VHOST_ENQ_RETRY_DEF) {
+ smap_add_format(args, "tx-retries-max", "%d", tx_retries_max);
+ }
+
+ ovs_mutex_unlock(&dev->mutex);
+
+ return 0;
+}
+
static int
netdev_dpdk_vhost_client_set_config(struct netdev *netdev,
const struct smap *args,
@@ -4091,6 +4115,9 @@ netdev_dpdk_vhost_user_get_status(const struct netdev
*netdev,
smap_add_format(args, "userspace-tso", "disabled");
}
+ smap_add_format(args, "n_rxq", "%d", netdev->n_rxq);
+ smap_add_format(args, "n_txq", "%d", netdev->n_txq);
+
ovs_mutex_unlock(&dev->mutex);
return 0;
}
@@ -4161,6 +4188,13 @@ netdev_dpdk_get_status(const struct netdev *netdev,
struct smap *args)
smap_add_format(args, "max_vfs", "%u", dev_info.max_vfs);
smap_add_format(args, "max_vmdq_pools", "%u", dev_info.max_vmdq_pools);
+ smap_add_format(args, "n_rxq", "%d", netdev->n_rxq);
+ smap_add_format(args, "n_txq", "%d", netdev->n_txq);
+
+ smap_add(args, "rx_csum_offload",
+ dev->hw_ol_features & NETDEV_RX_CHECKSUM_OFFLOAD
+ ? "true" : "false");
+
/* Querying the DPDK library for iftype may be done in future, pending
* support; cf. RFC 3635 Section 3.2.4. */
enum { IF_TYPE_ETHERNETCSMACD = 6 };
@@ -4186,16 +4220,21 @@ netdev_dpdk_get_status(const struct netdev *netdev,
struct smap *args)
ETH_ADDR_ARGS(dev->hwaddr));
}
- if (rx_steer_flags) {
- if (!rx_steer_flows_num) {
- smap_add(args, "rx_steering", "unsupported");
+ if (rx_steer_flags && !rx_steer_flows_num) {
+ smap_add(args, "rx-steering", "unsupported");
+ } else if (rx_steer_flags == DPDK_RX_STEER_LACP) {
+ smap_add(args, "rx-steering", "rss+lacp");
+ } else {
+ ovs_assert(!rx_steer_flags);
+ smap_add(args, "rx-steering", "rss");
+ }
+
+ if (rx_steer_flags && rx_steer_flows_num) {
+ smap_add_format(args, "rx_steering_queue", "%d", n_rxq - 1);
+ if (n_rxq > 2) {
+ smap_add_format(args, "rss_queues", "0-%d", n_rxq - 2);
} else {
- smap_add_format(args, "rx_steering_queue", "%d", n_rxq - 1);
- if (n_rxq > 2) {
- smap_add_format(args, "rss_queues", "0-%d", n_rxq - 2);
- } else {
- smap_add(args, "rss_queues", "0");
- }
+ smap_add(args, "rss_queues", "0");
}
}
Maybe can combine above code blocks to simplify ?
if (rx_steer_flags) {
if (!rx_steer_flows_num) {
smap_add(args, "rx-steering", "unsupported");
} else {
if (rx_steering_flags == DPDK_RX_STEER_LACP) {
smap_add(args, "rx-steering", "rss+lacp");
}
smap_add_format(args, "rx_steering_queue", "%d", n_rxq - 1);
if (n_rxq > 2) {
smap_add_format(args, "rss_queues", "0-%d", n_rxq - 2);
} else {
smap_add(args, "rss_queues", "0");
}
}
} else {
smap_add(args, "rx-steering", "rss");
}
@@ -6415,7 +6454,6 @@ parse_vhost_config(const struct smap *ovs_other_config)
.is_pmd = true, \
.alloc = netdev_dpdk_alloc, \
.dealloc = netdev_dpdk_dealloc, \
- .get_config = netdev_dpdk_get_config, \
.get_numa_id = netdev_dpdk_get_numa_id, \
.set_etheraddr = netdev_dpdk_set_etheraddr, \
.get_etheraddr = netdev_dpdk_get_etheraddr, \
@@ -6459,6 +6497,7 @@ static const struct netdev_class dpdk_class = {
.type = "dpdk",
NETDEV_DPDK_CLASS_BASE,
.construct = netdev_dpdk_construct,
+ .get_config = netdev_dpdk_get_config,
.set_config = netdev_dpdk_set_config,
.send = netdev_dpdk_eth_send,
};
@@ -6485,6 +6524,7 @@ static const struct netdev_class dpdk_vhost_client_class
= {
.init = netdev_dpdk_vhost_class_init,
.construct = netdev_dpdk_vhost_client_construct,
.destruct = netdev_dpdk_vhost_destruct,
+ .get_config = netdev_dpdk_vhost_client_get_config,
.set_config = netdev_dpdk_vhost_client_set_config,
.send = netdev_dpdk_vhost_send,
.get_carrier = netdev_dpdk_vhost_get_carrier,
diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
index 0f58e8574..fd42aed0b 100644
--- a/tests/system-dpdk.at
+++ b/tests/system-dpdk.at
@@ -588,8 +588,9 @@ AT_CHECK([ovs-vsctl show], [], [stdout])
sleep 2
dnl Check default MTU value in the datapath
-AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
-AT_CHECK([grep -E 'mtu=1500' stdout], [], [stdout])
+AT_CHECK([ovs-vsctl get Interface phy0 mtu], [0], [dnl
+1500
+])
dnl Increase MTU value and check in the datapath
AT_CHECK([ovs-vsctl set Interface phy0 mtu_request=9000])
@@ -600,8 +601,9 @@ AT_FAIL_IF([grep "Interface phy0 does not support MTU
configuration" ovs-vswitch
dnl Fail if error is encountered during MTU setup
AT_FAIL_IF([grep "Interface phy0 MTU (9000) setup error" ovs-vswitchd.log],
[], [stdout])
-AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
-AT_CHECK([grep -E 'mtu=9000' stdout], [], [stdout])
+AT_CHECK([ovs-vsctl get Interface phy0 mtu], [0], [dnl
+9000
+])
dnl Clean up
@@ -636,14 +638,16 @@ dnl Fail if error is encountered during MTU setup
AT_FAIL_IF([grep "Interface phy0 MTU (9000) setup error" ovs-vswitchd.log],
[], [stdout])
dnl Check MTU value in the datapath
-AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
-AT_CHECK([grep -E 'mtu=9000' stdout], [], [stdout])
+AT_CHECK([ovs-vsctl get Interface phy0 mtu], [0], [dnl
+9000
+])
dnl Decrease MTU value and check in the datapath
AT_CHECK([ovs-vsctl set Interface phy0 mtu_request=2000])
-AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
-AT_CHECK([grep -E 'mtu=2000' stdout], [], [stdout])
+AT_CHECK([ovs-vsctl get Interface phy0 mtu], [0], [dnl
+2000
+])
dnl Clean up
@@ -686,16 +690,19 @@ tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat
NUMA_NODE)" --no-pci\
--single-file-segments -- -a >$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log
2>&1 &
OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])
+OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0 link_state | grep
-w up])
dnl Check default MTU value in the datapath
-AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
-AT_CHECK([grep -E 'mtu=1500' stdout], [], [stdout])
+AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
+1500
+])
dnl Increase MTU value and check in the datapath
AT_CHECK([ovs-vsctl set Interface dpdkvhostuserclient0 mtu_request=9000])
-AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
-AT_CHECK([grep -E 'mtu=9000' stdout], [], [stdout])
+AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
+9000
+])
dnl Clean up the testpmd now
pkill -f -x -9 'tail -f /dev/null'
@@ -743,16 +750,19 @@ tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat
NUMA_NODE)" --no-pci\
--single-file-segments -- -a >$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log
2>&1 &
OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])
+OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0 link_state | grep
-w up])
dnl Check MTU value in the datapath
-AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
-AT_CHECK([grep -E 'mtu=9000' stdout], [], [stdout])
+AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
+9000
+])
dnl Decrease MTU value and check in the datapath
AT_CHECK([ovs-vsctl set Interface dpdkvhostuserclient0 mtu_request=2000])
-AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
-AT_CHECK([grep -E 'mtu=2000' stdout], [], [stdout])
+AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
+2000
+])
dnl Clean up the testpmd now
pkill -f -x -9 'tail -f /dev/null'
@@ -789,8 +799,9 @@ dnl Fail if error is encountered during MTU setup
AT_FAIL_IF([grep "Interface phy0 MTU (9702) setup error" ovs-vswitchd.log],
[], [stdout])
dnl Check MTU value in the datapath
-AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
-AT_CHECK([grep -E 'mtu=9702' stdout], [], [stdout])
+AT_CHECK([ovs-vsctl get Interface phy0 mtu], [0], [dnl
+9702
+])
dnl Set MTU value above upper bound and check for error
AT_CHECK([ovs-vsctl set Interface phy0 mtu_request=9711])
@@ -830,8 +841,9 @@ dnl Fail if error is encountered during MTU setup
AT_FAIL_IF([grep "Interface phy0 MTU (68) setup error" ovs-vswitchd.log], [],
[stdout])
dnl Check MTU value in the datapath
-AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
-AT_CHECK([grep -E 'mtu=68' stdout], [], [stdout])
+AT_CHECK([ovs-vsctl get Interface phy0 mtu], [0], [dnl
+68
+])
dnl Set MTU value below lower bound and check for error
AT_CHECK([ovs-vsctl set Interface phy0 mtu_request=67])
@@ -877,10 +889,12 @@ tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat
NUMA_NODE)" --no-pci\
--single-file-segments -- -a >$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log
2>&1 &
OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])
+OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0 link_state | grep
-w up])
dnl Check MTU value in the datapath
-AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
-AT_CHECK([grep -E 'mtu=9702' stdout], [], [stdout])
+AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
+9702
+])
dnl Set MTU value above upper bound and check for error
AT_CHECK([ovs-vsctl set Interface dpdkvhostuserclient0 mtu_request=9711])
@@ -934,10 +948,12 @@ tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat
NUMA_NODE)" --no-pci\
--single-file-segments -- -a >$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log
2>&1 &
OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])
+OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0 link_state | grep
-w up])
dnl Check MTU value in the datapath
-AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
-AT_CHECK([grep -E 'mtu=68' stdout], [], [stdout])
+AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
+68
+])
dnl Set MTU value below lower bound and check for error
AT_CHECK([ovs-vsctl set Interface dpdkvhostuserclient0 mtu_request=67])
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 1e2a1267d..dadaf5b05 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -3789,6 +3789,18 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch
options:peer=p1 \
Maximum number of VMDq pools.
</column>
+ <column name="status" key="n_rxq">
+ Number of rx queues.
+ </column>
+
+ <column name="status" key="n_txq">
+ Number of tx queues.
+ </column>
+
+ <column name="status" key="rx_csum_offload">
+ Whether hardware has support for RX Checksum Offload or not.
Better to be explicit that this is current status, rather than implying
by saying the hardware supports it. I know the code is a check on
whether hardare supports it, but that is also what is used when deciding
whether to enable. So suggest to reword to something like, "Whether Rx
Checksum offload is enabled or not"
+ </column>
+
<column name="status" key="if_type">
Interface type ID according to IANA ifTYPE MIB definitions.
</column>
@@ -3807,7 +3819,7 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch
options:peer=p1 \
VF representors.
</column>
- <column name="status" key="rx_steering">
+ <column name="status" key="rx-steering">
Hardware Rx queue steering policy in use.
</column>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev