On 10/4/23 14:21, jm...@redhat.com wrote:
> From: Jakob Meng <c...@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.
>
> 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/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 | 28 ++++++------
> tests/system-dpdk.at | 64 +++++++++++++++++----------
> vswitchd/vswitch.xml | 23 ++++++++++
> 11 files changed, 150 insertions(+), 76 deletions(-)
>
> 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..fb838286b 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 | sed
> 's/\(numa_id=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl
Hi, Jakub. Could you explain what is going on here?
I'm a bit confused. We cear out the number of tx_queues in some
tests, because it is determined dynamically in the datapath. But
why clearing the numa id instead? The numa id is not changing if
not explicitly set in the port configuration, IIRC.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
