| Message ID | 20231011101105.21803-1-jmeng@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [ovs-dev,v5] netdev: Sync'ed and cleaned {get, set}_config(). | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 11/10/2023 11:11, jmeng@redhat.com wrote: > From: Jakob Meng <code@jakobmeng.de> > > For better usability, the function pairs get_config() and > set_config() for each netdev 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. > vi yo > 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 <code@jakobmeng.de> > --- > Documentation/intro/install/afxdp.rst | 12 ++--- > Documentation/topics/dpdk/phy.rst | 4 +- > lib/netdev-afxdp.c | 21 ++++++++- > lib/netdev-afxdp.h | 1 + > lib/netdev-dpdk.c | 49 +++++++++++--------- > lib/netdev-dummy.c | 19 ++++++-- > lib/netdev-linux-private.h | 1 + > lib/netdev-linux.c | 4 +- > tests/pmd.at | 26 +++++------ > tests/system-dpdk.at | 64 +++++++++++++++++---------- > vswitchd/vswitch.xml | 25 ++++++++++- > 11 files changed, 150 insertions(+), 76 deletions(-) > Hi Jakob, some initial comments below. It feels like a lot of changes in one patch, split across different netdevs. I wonder could it be broken up into patches for the different netdev types ? or even further like groups for related features, queues/rx-steering/flow control ? Not sure what works best without causing too much intermediate updates with UTs etc. Also, I'd suggest adding a cover letter with diff from previous version i.e. the thing I forgot last week :-) thanks, Kevin. > diff --git a/Documentation/intro/install/afxdp.rst b/Documentation/intro/install/afxdp.rst > index 51c24bf5b..5776614c8 100644 > --- a/Documentation/intro/install/afxdp.rst > +++ b/Documentation/intro/install/afxdp.rst > @@ -219,14 +219,10 @@ Otherwise, enable debugging by:: > ovs-appctl vlog/set netdev_afxdp::dbg > > To check which XDP mode was chosen by ``best-effort``, you can look for > -``xdp-mode-in-use`` in the output of ``ovs-appctl dpctl/show``:: > - > - # ovs-appctl dpctl/show > - netdev@ovs-netdev: > - <...> > - port 2: ens802f0 (afxdp: n_rxq=1, use-need-wakeup=true, > - xdp-mode=best-effort, > - xdp-mode-in-use=native-with-zerocopy) > +``xdp-mode`` in the output of ``ovs-vsctl get interface INT status:xdp-mode``:: > + > + # ovs-vsctl get interface ens802f0 status:xdp-mode > + "native-with-zerocopy" > > References > ---------- > 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} > The fix is correct, but the meaning of unsupported is changed so text above (not shown in diff) is incorrect. Mentioning here but I think it will change in the status reporting and then the docs can be synced with that. > 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-afxdp.c b/lib/netdev-afxdp.c > index 16f26bc30..8519b5a2b 100644 > --- a/lib/netdev-afxdp.c > +++ b/lib/netdev-afxdp.c > @@ -672,8 +672,6 @@ netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args) > ovs_mutex_lock(&dev->mutex); > smap_add_format(args, "n_rxq", "%d", netdev->n_rxq); > smap_add_format(args, "xdp-mode", "%s", xdp_modes[dev->xdp_mode].name); > - smap_add_format(args, "xdp-mode-in-use", "%s", > - xdp_modes[dev->xdp_mode_in_use].name); > smap_add_format(args, "use-need-wakeup", "%s", > dev->use_need_wakeup ? "true" : "false"); > ovs_mutex_unlock(&dev->mutex); > @@ -1367,3 +1365,22 @@ netdev_afxdp_get_stats(const struct netdev *netdev, > > return error; > } > + > +int > +netdev_afxdp_get_status(const struct netdev *netdev, > + struct smap *args) > +{ > + int error = netdev_linux_get_status(netdev, args); blank line here > + if (error) { > + return error; > + } > + > + struct netdev_linux *dev = netdev_linux_cast(netdev); > + > + ovs_mutex_lock(&dev->mutex); > + smap_add_format(args, "xdp-mode", "%s", > + xdp_modes[dev->xdp_mode_in_use].name); > + ovs_mutex_unlock(&dev->mutex); > + > + return error; > +} > diff --git a/lib/netdev-afxdp.h b/lib/netdev-afxdp.h > index e91cd102d..bd3b9dfbe 100644 > --- a/lib/netdev-afxdp.h > +++ b/lib/netdev-afxdp.h > @@ -63,6 +63,7 @@ int netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args, > int netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args); > int netdev_afxdp_get_stats(const struct netdev *netdev_, > struct netdev_stats *stats); > +int netdev_afxdp_get_status(const struct netdev *netdev, struct smap *args); > int netdev_afxdp_get_custom_stats(const struct netdev *netdev, > struct netdev_custom_stats *custom_stats); > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 55700250d..05153d02f 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1905,24 +1905,26 @@ 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); User is losing info here as n_rxq and n_txq are not reported in dpkvhostclient status, suggest to add them to there. > - 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"); Flow control config should be for dpdk devices only below > > 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", > + > + smap_add(args, "dpdk-lsc-interrupt", > dev->lsc_interrupt_mode ? "true" : "false"); > > if (dpdk_port_is_representor(dev)) { > @@ -1930,6 +1932,7 @@ netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args) > ETH_ADDR_ARGS(dev->requested_hwaddr)); > } > } > + nit: unneeded newline added > ovs_mutex_unlock(&dev->mutex); > > return 0; > @@ -4161,6 +4164,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 +4196,15 @@ 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"); > + smap_add(args, "rx-steering", dev->rx_steer_flags == DPDK_RX_STEER_LACP > + ? "rss+lacp" : "unsupported"); I don't think "unsupported" is the right term now. If I don't enable rx-steering, it will report rx-steering=unsupported. It was previously for if rx-steering was requested by the user but the flow rule to steer traffic to the additional rxq was unsupported in the NIC. Also the phy.rst mentions this and it's out of sync now. Could consider reporting "rss" if rx_steer_flags are not set, but that is the default anyway so perhaps not needed. > + > + 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"); > } > } > > diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c > index 1a54add87..fe82317d7 100644 > --- a/lib/netdev-dummy.c > +++ b/lib/netdev-dummy.c > @@ -795,14 +795,25 @@ netdev_dummy_get_config(const struct netdev *dev, struct smap *args) > > dummy_packet_conn_get_config(&netdev->conn, args); > > + /* pcap, rxq_pcap and tx_pcap cannot be recovered because filenames have > + * been discarded after opening file descriptors */ > + > + if (netdev->ol_ip_csum) { > + smap_add_format(args, "ol_ip_csum", "%s", "true"); > + } > + > + if (netdev->ol_ip_csum_set_good) { > + smap_add_format(args, "ol_ip_csum_set_good", "%s", "true"); > + } > + > /* 'dummy-pmd' specific config. */ > if (!netdev_is_pmd(dev)) { > goto exit; > } > - smap_add_format(args, "requested_rx_queues", "%d", netdev->requested_n_rxq); > - smap_add_format(args, "configured_rx_queues", "%d", dev->n_rxq); > - smap_add_format(args, "requested_tx_queues", "%d", netdev->requested_n_txq); > - smap_add_format(args, "configured_tx_queues", "%d", dev->n_txq); > + > + smap_add_format(args, "n_rxq", "%d", netdev->requested_n_rxq); > + smap_add_format(args, "n_txq", "%d", netdev->requested_n_txq); > + smap_add_format(args, "numa_id", "%d", netdev->requested_numa_id); > > exit: > ovs_mutex_unlock(&netdev->mutex); > diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h > index 0ecf0f748..188e8438a 100644 > --- a/lib/netdev-linux-private.h > +++ b/lib/netdev-linux-private.h > @@ -50,6 +50,7 @@ struct netdev_rxq_linux { > }; > > int netdev_linux_construct(struct netdev *); > +int netdev_linux_get_status(const struct netdev *, struct smap *); > void netdev_linux_run(const struct netdev_class *); > > int get_stats_via_netlink(const struct netdev *netdev_, > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index cca340879..70521e3c7 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -3493,7 +3493,7 @@ netdev_linux_get_next_hop(const struct in_addr *host, struct in_addr *next_hop, > return ENXIO; > } > > -static int > +int > netdev_linux_get_status(const struct netdev *netdev_, struct smap *smap) > { > struct netdev_linux *netdev = netdev_linux_cast(netdev_); > @@ -3759,7 +3759,7 @@ const struct netdev_class netdev_internal_class = { > .destruct = netdev_afxdp_destruct, \ > .get_stats = netdev_afxdp_get_stats, \ > .get_custom_stats = netdev_afxdp_get_custom_stats, \ > - .get_status = netdev_linux_get_status, \ > + .get_status = netdev_afxdp_get_status, \ > .set_config = netdev_afxdp_set_config, \ > .get_config = netdev_afxdp_get_config, \ > .reconfigure = netdev_afxdp_reconfigure, \ > diff --git a/tests/pmd.at b/tests/pmd.at > index 7bdaca9e7..df6adde6c 100644 > --- a/tests/pmd.at > +++ b/tests/pmd.at > @@ -93,11 +93,11 @@ pmd thread numa_id <cleared> core_id <cleared>: > overhead: NOT AVAIL > ]) > > -AT_CHECK([ovs-appctl dpif/show | sed 's/\(tx_queues=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl > +AT_CHECK([ovs-appctl dpif/show], [0], [dnl > dummy@ovs-dummy: hit:0 missed:0 > br0: > br0 65534/100: (dummy-internal) > - p0 1/1: (dummy-pmd: configured_rx_queues=1, configured_tx_queues=<cleared>, requested_rx_queues=1, requested_tx_queues=<cleared>) > + p0 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0) > ]) > > OVS_VSWITCHD_STOP > @@ -111,11 +111,11 @@ CHECK_PMD_THREADS_CREATED() > > AT_CHECK([ovs-vsctl set interface p0 options:n_rxq=8]) > > -AT_CHECK([ovs-appctl dpif/show | sed 's/\(tx_queues=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl > +AT_CHECK([ovs-appctl dpif/show], [0], [dnl > dummy@ovs-dummy: hit:0 missed:0 > br0: > br0 65534/100: (dummy-internal) > - p0 1/1: (dummy-pmd: configured_rx_queues=8, configured_tx_queues=<cleared>, requested_rx_queues=8, requested_tx_queues=<cleared>) > + p0 1/1: (dummy-pmd: n_rxq=8, n_txq=1, numa_id=0) > ]) > > AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN], [0], [dnl > @@ -144,11 +144,11 @@ OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0 type=dummy-pmd options:n > CHECK_CPU_DISCOVERED(2) > CHECK_PMD_THREADS_CREATED() > > -AT_CHECK([ovs-appctl dpif/show | sed 's/\(tx_queues=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl > +AT_CHECK([ovs-appctl dpif/show], [0], [dnl > dummy@ovs-dummy: hit:0 missed:0 > br0: > br0 65534/100: (dummy-internal) > - p0 1/1: (dummy-pmd: configured_rx_queues=8, configured_tx_queues=<cleared>, requested_rx_queues=8, requested_tx_queues=<cleared>) > + p0 1/1: (dummy-pmd: n_rxq=8, n_txq=1, numa_id=0) > ]) > > AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN], [0], [dnl > @@ -227,11 +227,11 @@ TMP=$(($(cat ovs-vswitchd.log | wc -l | tr -d [[:blank:]])+1)) > CHECK_CPU_DISCOVERED(4) > CHECK_PMD_THREADS_CREATED() > > -AT_CHECK([ovs-appctl dpif/show | sed 's/\(tx_queues=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl > +AT_CHECK([ovs-appctl dpif/show | sed 's/\(numa_id=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl > dummy@ovs-dummy: hit:0 missed:0 > br0: > br0 65534/100: (dummy-internal) > - p0 1/1: (dummy-pmd: configured_rx_queues=8, configured_tx_queues=<cleared>, requested_rx_queues=8, requested_tx_queues=<cleared>) > + p0 1/1: (dummy-pmd: n_rxq=8, n_txq=1, numa_id=<cleared>) > ]) > > AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN], [0], [dnl > @@ -436,11 +436,11 @@ AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:smc-enable=true]) > > sleep 1 > > -AT_CHECK([ovs-appctl dpif/show | sed 's/\(tx_queues=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl > +AT_CHECK([ovs-appctl dpif/show], [0], [dnl > dummy@ovs-dummy: hit:0 missed:0 > br0: > br0 65534/100: (dummy-internal) > - p0 7/1: (dummy-pmd: configured_rx_queues=4, configured_tx_queues=<cleared>, requested_rx_queues=4, requested_tx_queues=<cleared>) > + p0 7/1: (dummy-pmd: n_rxq=4, n_txq=1, numa_id=0) > ]) > > AT_CHECK([ovs-appctl dpif-netdev/pmd-stats-show | sed SED_NUMA_CORE_PATTERN | sed '/cycles/d' | grep pmd -A 12], [0], [dnl > @@ -604,8 +604,8 @@ icmp,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_src=10 > dnl Check resetting to default number of rx queues after removal from the db. > AT_CHECK([ovs-vsctl remove interface p1 options n_rxq]) > > -AT_CHECK([ovs-appctl dpif/show | grep p1 | sed 's/\(tx_queues=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl > - p1 1/1: (dummy-pmd: configured_rx_queues=1, configured_tx_queues=<cleared>, requested_rx_queues=1, requested_tx_queues=<cleared>) > +AT_CHECK([ovs-appctl dpif/show | grep p1], [0], [dnl > + p1 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0) > ]) > > OVS_VSWITCHD_STOP > @@ -1152,7 +1152,7 @@ dummy@dp0: > lookups: hit:0 missed:0 lost:0 > flows: 0 > port 0: dp0 (dummy-internal) > - port 1: p1 (dummy-pmd: configured_rx_queues=1, configured_tx_queues=1, requested_rx_queues=1, requested_tx_queues=1) > + port 1: p1 (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0) > port 2: p2 (dummy) > ]) > > 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..a8037a96f 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. > + </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> > > @@ -3821,6 +3833,17 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ > supported by hardware. > </column> > </group> > + > + <group title="afxdp"> > + <p> > + AF_XDP netdev specific interface status options. > + </p> > + > + <column name="status" key="xdp-mode"> > + XDP mode which was chosen. > + </column> > + > + </group> > </group> > > <group title="Statistics">
On 11.10.23 18:48, Kevin Traynor wrote: > On 11/10/2023 11:11, jmeng@redhat.com wrote: >> From: Jakob Meng <code@jakobmeng.de> >> >> For better usability, the function pairs get_config() and >> set_config() for each netdev 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. >> vi yo > >> 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 <code@jakobmeng.de> >> --- >> Documentation/intro/install/afxdp.rst | 12 ++--- >> Documentation/topics/dpdk/phy.rst | 4 +- >> lib/netdev-afxdp.c | 21 ++++++++- >> lib/netdev-afxdp.h | 1 + >> lib/netdev-dpdk.c | 49 +++++++++++--------- >> lib/netdev-dummy.c | 19 ++++++-- >> lib/netdev-linux-private.h | 1 + >> lib/netdev-linux.c | 4 +- >> tests/pmd.at | 26 +++++------ >> tests/system-dpdk.at | 64 +++++++++++++++++---------- >> vswitchd/vswitch.xml | 25 ++++++++++- >> 11 files changed, 150 insertions(+), 76 deletions(-) >> > > Hi Jakob, some initial comments below. > Thank you ☺️ > It feels like a lot of changes in one patch, split across different netdevs. I wonder could it be broken up into patches for the different netdev types ? or even further like groups for related features, queues/rx-steering/flow control ? Not sure what works best without causing too much intermediate updates with UTs etc. > > Also, I'd suggest adding a cover letter with diff from previous version i.e. the thing I forgot last week :-) > Ack, will try to split this patch up for the next revision. But first, some questions below ;) > >> diff --git a/Documentation/intro/install/afxdp.rst b/Documentation/intro/install/afxdp.rst >> index 51c24bf5b..5776614c8 100644 >> --- a/Documentation/intro/install/afxdp.rst >> +++ b/Documentation/intro/install/afxdp.rst >> @@ -219,14 +219,10 @@ Otherwise, enable debugging by:: >> ovs-appctl vlog/set netdev_afxdp::dbg >> To check which XDP mode was chosen by ``best-effort``, you can look for >> -``xdp-mode-in-use`` in the output of ``ovs-appctl dpctl/show``:: >> - >> - # ovs-appctl dpctl/show >> - netdev@ovs-netdev: >> - <...> >> - port 2: ens802f0 (afxdp: n_rxq=1, use-need-wakeup=true, >> - xdp-mode=best-effort, >> - xdp-mode-in-use=native-with-zerocopy) >> +``xdp-mode`` in the output of ``ovs-vsctl get interface INT status:xdp-mode``:: >> + >> + # ovs-vsctl get interface ens802f0 status:xdp-mode >> + "native-with-zerocopy" >> References >> ---------- >> 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} >> > > The fix is correct, but the meaning of unsupported is changed so text above (not shown in diff) is incorrect. Mentioning here but I think it will change in the status reporting and then the docs can be synced with that. Good catch! We could expose "rx_steer_flows_num" but this would be less self-explanatory than printing some kind of "unsupported". Any idea how to fix this properly in the docs? > >> 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-afxdp.c b/lib/netdev-afxdp.c >> index 16f26bc30..8519b5a2b 100644 >> --- a/lib/netdev-afxdp.c >> +++ b/lib/netdev-afxdp.c >> @@ -672,8 +672,6 @@ netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args) >> ovs_mutex_lock(&dev->mutex); >> smap_add_format(args, "n_rxq", "%d", netdev->n_rxq); >> smap_add_format(args, "xdp-mode", "%s", xdp_modes[dev->xdp_mode].name); >> - smap_add_format(args, "xdp-mode-in-use", "%s", >> - xdp_modes[dev->xdp_mode_in_use].name); >> smap_add_format(args, "use-need-wakeup", "%s", >> dev->use_need_wakeup ? "true" : "false"); >> ovs_mutex_unlock(&dev->mutex); >> @@ -1367,3 +1365,22 @@ netdev_afxdp_get_stats(const struct netdev *netdev, >> return error; >> } >> + >> +int >> +netdev_afxdp_get_status(const struct netdev *netdev, >> + struct smap *args) >> +{ >> + int error = netdev_linux_get_status(netdev, args); > > blank line here Why should I add a blank line before "if (error)"? In most cases across OVS' codebase, the conditional has no blank line in front. Or what are you referring to? > >> + if (error) { >> + return error; >> + } >> + >> + struct netdev_linux *dev = netdev_linux_cast(netdev); >> + >> + ovs_mutex_lock(&dev->mutex); >> + smap_add_format(args, "xdp-mode", "%s", >> + xdp_modes[dev->xdp_mode_in_use].name); >> + ovs_mutex_unlock(&dev->mutex); >> + >> + return error; >> +} >> diff --git a/lib/netdev-afxdp.h b/lib/netdev-afxdp.h >> index e91cd102d..bd3b9dfbe 100644 >> --- a/lib/netdev-afxdp.h >> +++ b/lib/netdev-afxdp.h >> @@ -63,6 +63,7 @@ int netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args, >> int netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args); >> int netdev_afxdp_get_stats(const struct netdev *netdev_, >> struct netdev_stats *stats); >> +int netdev_afxdp_get_status(const struct netdev *netdev, struct smap *args); >> int netdev_afxdp_get_custom_stats(const struct netdev *netdev, >> struct netdev_custom_stats *custom_stats); >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 55700250d..05153d02f 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1905,24 +1905,26 @@ 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); > > User is losing info here as n_rxq and n_txq are not reported in dpkvhostclient status, suggest to add them to there. > >> - 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"); > > Flow control config should be for dpdk devices only below Well, I completely missed netdev_dpdk_vhost_client_set_config🙈 Will fix. > >> 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", >> + >> + smap_add(args, "dpdk-lsc-interrupt", >> dev->lsc_interrupt_mode ? "true" : "false"); >> if (dpdk_port_is_representor(dev)) { >> @@ -1930,6 +1932,7 @@ netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args) >> ETH_ADDR_ARGS(dev->requested_hwaddr)); >> } >> } >> + > > nit: unneeded newline added > >> ovs_mutex_unlock(&dev->mutex); >> return 0; >> @@ -4161,6 +4164,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 +4196,15 @@ 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"); >> + smap_add(args, "rx-steering", dev->rx_steer_flags == DPDK_RX_STEER_LACP >> + ? "rss+lacp" : "unsupported"); > > I don't think "unsupported" is the right term now. If I don't enable rx-steering, it will report rx-steering=unsupported. > > It was previously for if rx-steering was requested by the user but the flow rule to steer traffic to the additional rxq was unsupported in the NIC. Also the phy.rst mentions this and it's out of sync now. > > Could consider reporting "rss" if rx_steer_flags are not set, but that is the default anyway so perhaps not needed. Thank you for the explanation. A value other than "rss+lacp" in "rx-steering" will cause dpdk_set_rx_steer_config() to log an error and basically ignore that option. Suddenly returning a different value such as "rss" for "rx-steering" would be strange. On the other hand, the try_rx_steer code in netdev_dpdk_reconfigure() will log "rx-steering: default rss" when hw support is missing. Then again the dpdk_rx_steer_flags enum only lists DPDK_RX_STEER_LACP which feels like it contradicts the log message?!? How to clean this up? > >> + >> + if (rx_steer_flags && rx_steer_flows_num) { >> <REMAINING TEXT REMOVED BECAUSE IRRELEVANT TO DISCUSSION>
On 12/10/2023 14:06, Jakob Meng wrote: > On 11.10.23 18:48, Kevin Traynor wrote: >> On 11/10/2023 11:11, jmeng@redhat.com wrote: >>> From: Jakob Meng <code@jakobmeng.de> >>> >>> For better usability, the function pairs get_config() and >>> set_config() for each netdev 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. >>> vi yo >> >>> 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 <code@jakobmeng.de> >>> --- >>> Documentation/intro/install/afxdp.rst | 12 ++--- >>> Documentation/topics/dpdk/phy.rst | 4 +- >>> lib/netdev-afxdp.c | 21 ++++++++- >>> lib/netdev-afxdp.h | 1 + >>> lib/netdev-dpdk.c | 49 +++++++++++--------- >>> lib/netdev-dummy.c | 19 ++++++-- >>> lib/netdev-linux-private.h | 1 + >>> lib/netdev-linux.c | 4 +- >>> tests/pmd.at | 26 +++++------ >>> tests/system-dpdk.at | 64 +++++++++++++++++---------- >>> vswitchd/vswitch.xml | 25 ++++++++++- >>> 11 files changed, 150 insertions(+), 76 deletions(-) >>> >> >> Hi Jakob, some initial comments below. >> > > Thank you ☺️ > >> It feels like a lot of changes in one patch, split across different netdevs. I wonder could it be broken up into patches for the different netdev types ? or even further like groups for related features, queues/rx-steering/flow control ? Not sure what works best without causing too much intermediate updates with UTs etc. >> >> Also, I'd suggest adding a cover letter with diff from previous version i.e. the thing I forgot last week :-) >> > > Ack, will try to split this patch up for the next revision. But first, some questions below ;) > >> >>> diff --git a/Documentation/intro/install/afxdp.rst b/Documentation/intro/install/afxdp.rst >>> index 51c24bf5b..5776614c8 100644 >>> --- a/Documentation/intro/install/afxdp.rst >>> +++ b/Documentation/intro/install/afxdp.rst >>> @@ -219,14 +219,10 @@ Otherwise, enable debugging by:: >>> ovs-appctl vlog/set netdev_afxdp::dbg >>> To check which XDP mode was chosen by ``best-effort``, you can look for >>> -``xdp-mode-in-use`` in the output of ``ovs-appctl dpctl/show``:: >>> - >>> - # ovs-appctl dpctl/show >>> - netdev@ovs-netdev: >>> - <...> >>> - port 2: ens802f0 (afxdp: n_rxq=1, use-need-wakeup=true, >>> - xdp-mode=best-effort, >>> - xdp-mode-in-use=native-with-zerocopy) >>> +``xdp-mode`` in the output of ``ovs-vsctl get interface INT status:xdp-mode``:: >>> + >>> + # ovs-vsctl get interface ens802f0 status:xdp-mode >>> + "native-with-zerocopy" >>> References >>> ---------- >>> 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} >>> >> >> The fix is correct, but the meaning of unsupported is changed so text above (not shown in diff) is incorrect. Mentioning here but I think it will change in the status reporting and then the docs can be synced with that. > > Good catch! We could expose "rx_steer_flows_num" but this would be less self-explanatory than printing some kind of "unsupported". Any idea how to fix this properly in the docs? > I think the important thing is that user knows the default is rss and that is mentioned. It could be explicitly stated that it is the default and only non 'rss' values are shown. Anyway, best to figure out what to show below and then match the docs to it. >> >>> 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-afxdp.c b/lib/netdev-afxdp.c >>> index 16f26bc30..8519b5a2b 100644 >>> --- a/lib/netdev-afxdp.c >>> +++ b/lib/netdev-afxdp.c >>> @@ -672,8 +672,6 @@ netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args) >>> ovs_mutex_lock(&dev->mutex); >>> smap_add_format(args, "n_rxq", "%d", netdev->n_rxq); >>> smap_add_format(args, "xdp-mode", "%s", xdp_modes[dev->xdp_mode].name); >>> - smap_add_format(args, "xdp-mode-in-use", "%s", >>> - xdp_modes[dev->xdp_mode_in_use].name); >>> smap_add_format(args, "use-need-wakeup", "%s", >>> dev->use_need_wakeup ? "true" : "false"); >>> ovs_mutex_unlock(&dev->mutex); >>> @@ -1367,3 +1365,22 @@ netdev_afxdp_get_stats(const struct netdev *netdev, >>> return error; >>> } >>> + >>> +int >>> +netdev_afxdp_get_status(const struct netdev *netdev, >>> + struct smap *args) >>> +{ >>> + int error = netdev_linux_get_status(netdev, args); >> >> blank line here > > Why should I add a blank line before "if (error)"? In most cases across OVS' codebase, the conditional has no blank line in front. Or what are you referring to? > I was referring to the variable definition at the start of the function. There is normally a blank line after them. >> >>> + if (error) { >>> + return error; >>> + } >>> + >>> + struct netdev_linux *dev = netdev_linux_cast(netdev); >>> + >>> + ovs_mutex_lock(&dev->mutex); >>> + smap_add_format(args, "xdp-mode", "%s", >>> + xdp_modes[dev->xdp_mode_in_use].name); >>> + ovs_mutex_unlock(&dev->mutex); >>> + >>> + return error; >>> +} >>> diff --git a/lib/netdev-afxdp.h b/lib/netdev-afxdp.h >>> index e91cd102d..bd3b9dfbe 100644 >>> --- a/lib/netdev-afxdp.h >>> +++ b/lib/netdev-afxdp.h >>> @@ -63,6 +63,7 @@ int netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args, >>> int netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args); >>> int netdev_afxdp_get_stats(const struct netdev *netdev_, >>> struct netdev_stats *stats); >>> +int netdev_afxdp_get_status(const struct netdev *netdev, struct smap *args); >>> int netdev_afxdp_get_custom_stats(const struct netdev *netdev, >>> struct netdev_custom_stats *custom_stats); >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>> index 55700250d..05153d02f 100644 >>> --- a/lib/netdev-dpdk.c >>> +++ b/lib/netdev-dpdk.c >>> @@ -1905,24 +1905,26 @@ 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); >> >> User is losing info here as n_rxq and n_txq are not reported in dpkvhostclient status, suggest to add them to there. >> >>> - 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"); >> >> Flow control config should be for dpdk devices only below > > Well, I completely missed netdev_dpdk_vhost_client_set_config🙈 Will fix. > 'tx-retries-max' is one that is set but not reported from get_config that could be added. There is already reporting of socket path with 'dpdk-devargs' and 'socket' >> >>> 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", >>> + >>> + smap_add(args, "dpdk-lsc-interrupt", >>> dev->lsc_interrupt_mode ? "true" : "false"); >>> if (dpdk_port_is_representor(dev)) { >>> @@ -1930,6 +1932,7 @@ netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args) >>> ETH_ADDR_ARGS(dev->requested_hwaddr)); >>> } >>> } >>> + >> >> nit: unneeded newline added >> >>> ovs_mutex_unlock(&dev->mutex); >>> return 0; >>> @@ -4161,6 +4164,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 +4196,15 @@ 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"); >>> + smap_add(args, "rx-steering", dev->rx_steer_flags == DPDK_RX_STEER_LACP >>> + ? "rss+lacp" : "unsupported"); >> >> I don't think "unsupported" is the right term now. If I don't enable rx-steering, it will report rx-steering=unsupported. >> >> It was previously for if rx-steering was requested by the user but the flow rule to steer traffic to the additional rxq was unsupported in the NIC. Also the phy.rst mentions this and it's out of sync now. >> >> Could consider reporting "rss" if rx_steer_flags are not set, but that is the default anyway so perhaps not needed. > > Thank you for the explanation. A value other than "rss+lacp" in "rx-steering" will cause dpdk_set_rx_steer_config() to log an error and basically ignore that option. Suddenly returning a different value such as "rss" for "rx-steering" would be strange. > > On the other hand, the try_rx_steer code in netdev_dpdk_reconfigure() will log "rx-steering: default rss" when hw support is missing. Then again the dpdk_rx_steer_flags enum only lists DPDK_RX_STEER_LACP which feels like it contradicts the log message?!? > > How to clean this up? > ok, 'rss' is documented as default, so maybe we don't need to display if it is in use by default, selected by user or as fallback. That would make things a bit easier as 'rx-steering:' is free to use to indicate the unsupported case. So maybe status could just have: 'rx-steering:rss+lacp' for user enabled rss+lacp and NIC supports 'rx-steering:unsupported' for user enabled rss+lacp and NIC does not support. If rx-steering is not reported, then it is using the default 'rss'. Robin, what do you think ? >> >>> + >>> + if (rx_steer_flags && rx_steer_flows_num) { >>> <REMAINING TEXT REMOVED BECAUSE IRRELEVANT TO DISCUSSION> >
Kevin Traynor, Oct 12, 2023 at 17:34: > ok, 'rss' is documented as default, so maybe we don't need to display if > it is in use by default, selected by user or as fallback. > > That would make things a bit easier as 'rx-steering:' is free to use to > indicate the unsupported case. > > So maybe status could just have: > 'rx-steering:rss+lacp' for user enabled rss+lacp and NIC supports > 'rx-steering:unsupported' for user enabled rss+lacp and NIC does not > support. > > If rx-steering is not reported, then it is using the default 'rss'. > > Robin, what do you think ? It may be surprising to users not to see any mention in get_config() when explicitly setting rx-steering=rss but I don't see that as a common/standard use case. So I think it should be OK.
On 12.10.23 17:39, Robin Jarry wrote: > Kevin Traynor, Oct 12, 2023 at 17:34: >> ok, 'rss' is documented as default, so maybe we don't need to display if it is in use by default, selected by user or as fallback. >> >> That would make things a bit easier as 'rx-steering:' is free to use to indicate the unsupported case. >> >> So maybe status could just have: >> 'rx-steering:rss+lacp' for user enabled rss+lacp and NIC supports >> 'rx-steering:unsupported' for user enabled rss+lacp and NIC does not support. >> >> If rx-steering is not reported, then it is using the default 'rss'. >> >> Robin, what do you think ? > > It may be surprising to users not to see any mention in get_config() when explicitly setting rx-steering=rss but I don't see that as a common/standard use case. So I think it should be OK. > Thank you Kevin and Robin ☺️ Your change requests have been incorporated in v6: https://patchwork.ozlabs.org/project/openvswitch/list/?series=377463
diff --git a/Documentation/intro/install/afxdp.rst b/Documentation/intro/install/afxdp.rst index 51c24bf5b..5776614c8 100644 --- a/Documentation/intro/install/afxdp.rst +++ b/Documentation/intro/install/afxdp.rst @@ -219,14 +219,10 @@ Otherwise, enable debugging by:: ovs-appctl vlog/set netdev_afxdp::dbg To check which XDP mode was chosen by ``best-effort``, you can look for -``xdp-mode-in-use`` in the output of ``ovs-appctl dpctl/show``:: - - # ovs-appctl dpctl/show - netdev@ovs-netdev: - <...> - port 2: ens802f0 (afxdp: n_rxq=1, use-need-wakeup=true, - xdp-mode=best-effort, - xdp-mode-in-use=native-with-zerocopy) +``xdp-mode`` in the output of ``ovs-vsctl get interface INT status:xdp-mode``:: + + # ovs-vsctl get interface ens802f0 status:xdp-mode + "native-with-zerocopy" References ---------- 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-afxdp.c b/lib/netdev-afxdp.c index 16f26bc30..8519b5a2b 100644 --- a/lib/netdev-afxdp.c +++ b/lib/netdev-afxdp.c @@ -672,8 +672,6 @@ netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args) ovs_mutex_lock(&dev->mutex); smap_add_format(args, "n_rxq", "%d", netdev->n_rxq); smap_add_format(args, "xdp-mode", "%s", xdp_modes[dev->xdp_mode].name); - smap_add_format(args, "xdp-mode-in-use", "%s", - xdp_modes[dev->xdp_mode_in_use].name); smap_add_format(args, "use-need-wakeup", "%s", dev->use_need_wakeup ? "true" : "false"); ovs_mutex_unlock(&dev->mutex); @@ -1367,3 +1365,22 @@ netdev_afxdp_get_stats(const struct netdev *netdev, return error; } + +int +netdev_afxdp_get_status(const struct netdev *netdev, + struct smap *args) +{ + int error = netdev_linux_get_status(netdev, args); + if (error) { + return error; + } + + struct netdev_linux *dev = netdev_linux_cast(netdev); + + ovs_mutex_lock(&dev->mutex); + smap_add_format(args, "xdp-mode", "%s", + xdp_modes[dev->xdp_mode_in_use].name); + ovs_mutex_unlock(&dev->mutex); + + return error; +} diff --git a/lib/netdev-afxdp.h b/lib/netdev-afxdp.h index e91cd102d..bd3b9dfbe 100644 --- a/lib/netdev-afxdp.h +++ b/lib/netdev-afxdp.h @@ -63,6 +63,7 @@ int netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args, int netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args); int netdev_afxdp_get_stats(const struct netdev *netdev_, struct netdev_stats *stats); +int netdev_afxdp_get_status(const struct netdev *netdev, struct smap *args); int netdev_afxdp_get_custom_stats(const struct netdev *netdev, struct netdev_custom_stats *custom_stats); diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 55700250d..05153d02f 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1905,24 +1905,26 @@ 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", + + smap_add(args, "dpdk-lsc-interrupt", dev->lsc_interrupt_mode ? "true" : "false"); if (dpdk_port_is_representor(dev)) { @@ -1930,6 +1932,7 @@ netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args) ETH_ADDR_ARGS(dev->requested_hwaddr)); } } + ovs_mutex_unlock(&dev->mutex); return 0; @@ -4161,6 +4164,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 +4196,15 @@ 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"); + smap_add(args, "rx-steering", dev->rx_steer_flags == DPDK_RX_STEER_LACP + ? "rss+lacp" : "unsupported"); + + 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"); } } diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 1a54add87..fe82317d7 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c @@ -795,14 +795,25 @@ netdev_dummy_get_config(const struct netdev *dev, struct smap *args) dummy_packet_conn_get_config(&netdev->conn, args); + /* pcap, rxq_pcap and tx_pcap cannot be recovered because filenames have + * been discarded after opening file descriptors */ + + if (netdev->ol_ip_csum) { + smap_add_format(args, "ol_ip_csum", "%s", "true"); + } + + if (netdev->ol_ip_csum_set_good) { + smap_add_format(args, "ol_ip_csum_set_good", "%s", "true"); + } + /* 'dummy-pmd' specific config. */ if (!netdev_is_pmd(dev)) { goto exit; } - smap_add_format(args, "requested_rx_queues", "%d", netdev->requested_n_rxq); - smap_add_format(args, "configured_rx_queues", "%d", dev->n_rxq); - smap_add_format(args, "requested_tx_queues", "%d", netdev->requested_n_txq); - smap_add_format(args, "configured_tx_queues", "%d", dev->n_txq); + + smap_add_format(args, "n_rxq", "%d", netdev->requested_n_rxq); + smap_add_format(args, "n_txq", "%d", netdev->requested_n_txq); + smap_add_format(args, "numa_id", "%d", netdev->requested_numa_id); exit: ovs_mutex_unlock(&netdev->mutex); diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h index 0ecf0f748..188e8438a 100644 --- a/lib/netdev-linux-private.h +++ b/lib/netdev-linux-private.h @@ -50,6 +50,7 @@ struct netdev_rxq_linux { }; int netdev_linux_construct(struct netdev *); +int netdev_linux_get_status(const struct netdev *, struct smap *); void netdev_linux_run(const struct netdev_class *); int get_stats_via_netlink(const struct netdev *netdev_, diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index cca340879..70521e3c7 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -3493,7 +3493,7 @@ netdev_linux_get_next_hop(const struct in_addr *host, struct in_addr *next_hop, return ENXIO; } -static int +int netdev_linux_get_status(const struct netdev *netdev_, struct smap *smap) { struct netdev_linux *netdev = netdev_linux_cast(netdev_); @@ -3759,7 +3759,7 @@ const struct netdev_class netdev_internal_class = { .destruct = netdev_afxdp_destruct, \ .get_stats = netdev_afxdp_get_stats, \ .get_custom_stats = netdev_afxdp_get_custom_stats, \ - .get_status = netdev_linux_get_status, \ + .get_status = netdev_afxdp_get_status, \ .set_config = netdev_afxdp_set_config, \ .get_config = netdev_afxdp_get_config, \ .reconfigure = netdev_afxdp_reconfigure, \ diff --git a/tests/pmd.at b/tests/pmd.at index 7bdaca9e7..df6adde6c 100644 --- a/tests/pmd.at +++ b/tests/pmd.at @@ -93,11 +93,11 @@ pmd thread numa_id <cleared> core_id <cleared>: overhead: NOT AVAIL ]) -AT_CHECK([ovs-appctl dpif/show | sed 's/\(tx_queues=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl +AT_CHECK([ovs-appctl dpif/show], [0], [dnl dummy@ovs-dummy: hit:0 missed:0 br0: br0 65534/100: (dummy-internal) - p0 1/1: (dummy-pmd: configured_rx_queues=1, configured_tx_queues=<cleared>, requested_rx_queues=1, requested_tx_queues=<cleared>) + p0 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0) ]) OVS_VSWITCHD_STOP @@ -111,11 +111,11 @@ CHECK_PMD_THREADS_CREATED() AT_CHECK([ovs-vsctl set interface p0 options:n_rxq=8]) -AT_CHECK([ovs-appctl dpif/show | sed 's/\(tx_queues=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl +AT_CHECK([ovs-appctl dpif/show], [0], [dnl dummy@ovs-dummy: hit:0 missed:0 br0: br0 65534/100: (dummy-internal) - p0 1/1: (dummy-pmd: configured_rx_queues=8, configured_tx_queues=<cleared>, requested_rx_queues=8, requested_tx_queues=<cleared>) + p0 1/1: (dummy-pmd: n_rxq=8, n_txq=1, numa_id=0) ]) AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN], [0], [dnl @@ -144,11 +144,11 @@ OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0 type=dummy-pmd options:n CHECK_CPU_DISCOVERED(2) CHECK_PMD_THREADS_CREATED() -AT_CHECK([ovs-appctl dpif/show | sed 's/\(tx_queues=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl +AT_CHECK([ovs-appctl dpif/show], [0], [dnl dummy@ovs-dummy: hit:0 missed:0 br0: br0 65534/100: (dummy-internal) - p0 1/1: (dummy-pmd: configured_rx_queues=8, configured_tx_queues=<cleared>, requested_rx_queues=8, requested_tx_queues=<cleared>) + p0 1/1: (dummy-pmd: n_rxq=8, n_txq=1, numa_id=0) ]) AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN], [0], [dnl @@ -227,11 +227,11 @@ TMP=$(($(cat ovs-vswitchd.log | wc -l | tr -d [[:blank:]])+1)) CHECK_CPU_DISCOVERED(4) CHECK_PMD_THREADS_CREATED() -AT_CHECK([ovs-appctl dpif/show | sed 's/\(tx_queues=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl +AT_CHECK([ovs-appctl dpif/show | sed 's/\(numa_id=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl dummy@ovs-dummy: hit:0 missed:0 br0: br0 65534/100: (dummy-internal) - p0 1/1: (dummy-pmd: configured_rx_queues=8, configured_tx_queues=<cleared>, requested_rx_queues=8, requested_tx_queues=<cleared>) + p0 1/1: (dummy-pmd: n_rxq=8, n_txq=1, numa_id=<cleared>) ]) AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN], [0], [dnl @@ -436,11 +436,11 @@ AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:smc-enable=true]) sleep 1 -AT_CHECK([ovs-appctl dpif/show | sed 's/\(tx_queues=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl +AT_CHECK([ovs-appctl dpif/show], [0], [dnl dummy@ovs-dummy: hit:0 missed:0 br0: br0 65534/100: (dummy-internal) - p0 7/1: (dummy-pmd: configured_rx_queues=4, configured_tx_queues=<cleared>, requested_rx_queues=4, requested_tx_queues=<cleared>) + p0 7/1: (dummy-pmd: n_rxq=4, n_txq=1, numa_id=0) ]) AT_CHECK([ovs-appctl dpif-netdev/pmd-stats-show | sed SED_NUMA_CORE_PATTERN | sed '/cycles/d' | grep pmd -A 12], [0], [dnl @@ -604,8 +604,8 @@ icmp,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_src=10 dnl Check resetting to default number of rx queues after removal from the db. AT_CHECK([ovs-vsctl remove interface p1 options n_rxq]) -AT_CHECK([ovs-appctl dpif/show | grep p1 | sed 's/\(tx_queues=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl - p1 1/1: (dummy-pmd: configured_rx_queues=1, configured_tx_queues=<cleared>, requested_rx_queues=1, requested_tx_queues=<cleared>) +AT_CHECK([ovs-appctl dpif/show | grep p1], [0], [dnl + p1 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0) ]) OVS_VSWITCHD_STOP @@ -1152,7 +1152,7 @@ dummy@dp0: lookups: hit:0 missed:0 lost:0 flows: 0 port 0: dp0 (dummy-internal) - port 1: p1 (dummy-pmd: configured_rx_queues=1, configured_tx_queues=1, requested_rx_queues=1, requested_tx_queues=1) + port 1: p1 (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0) port 2: p2 (dummy) ]) 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..a8037a96f 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. + </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> @@ -3821,6 +3833,17 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ supported by hardware. </column> </group> + + <group title="afxdp"> + <p> + AF_XDP netdev specific interface status options. + </p> + + <column name="status" key="xdp-mode"> + XDP mode which was chosen. + </column> + + </group> </group> <group title="Statistics">