On 11.10.23 18:48, Kevin Traynor wrote:
> On 11/10/2023 11:11, 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.
>> 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 <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 | 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>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
