On 23/10/2023 09:37, Jakob Meng wrote:
Thanks for your feedback, Kevin! Added some notes inline.On 20.10.23 12:03, Kevin Traynor wrote: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🙈 Will fix..+ + 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.It is not handled consistently in ovs (definitiely allowed in the coding style doc though). The rest of this file puts variable definitions at the beginning, so I will change it as suggested.👍+ 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"); }Nooo, please not 😱 I split this big code block into smaller chunks because it is hardly readable with all those different if's 😬
Well, yes I agree it's one more conditional level of complexity, but otoh it's not conditional on combinations and it removes duplicate checks on rx_steer_flags and rx_steer_flows_numa from below.
Anyway, both code does the job and I guess it's just down to personal preferences, so I won't insist you to type something against your will :-)
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(args, "rss_queues", "0");
}
}
@@ -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"Ack👍+ </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
