On 27/10/2023 14:38, Ilya Maximets wrote:
On 10/26/23 11:29, Jakob Meng wrote:
On 25.10.23 19:10, Ilya Maximets wrote:
On 10/24/23 11:21, Kevin Traynor wrote:
Using correct email for Simon this time
On 24/10/2023 10:19, Kevin Traynor wrote:
On 23/10/2023 10:11, Jakob Meng wrote:
On 20.10.23 12:02, 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 series moves key-value pairs which are no valid options
from get_config() to the get_status() callback. The documentation in
vswitchd/vswitch.xml for status columns as well as tests have been
updated accordingly.
Compared to v4, the patch has been split into a series. Change requests
from Kevin Traynor have been incorporated, e.g. n_{r,t}xq will be
reported in dpkvhostclient status and tx-steering in the dpdk status
will be "unsupported" if the hw does not support steering traffic to
additional rxq.
The netdev dpdk classes no longer share a common get_config callback,
instead both the dpdk_class and the dpdk_vhost_client_class defines
their own callbacks.
Looks like you've lost the callback for the the vhost-user server ports.
(I noticed that in the code, but I didn't do a full review, so replying here.)
With "vhost-user server ports" you meant dpdk_vhost_class?
The get_config callback for dpdk_vhost_class has been dropped because it does
not have a set_config callback.
Ah, true. Please, add a note about that to the commit message.
For dpdk_vhost_client_class both config options
vhost-server-path and tx-retries-max are returned which were missed in
the previous patch version.
Jakob Meng (3):
netdev-dpdk: Sync and clean {get,set}_config() callbacks.
netdev-dummy: Sync and clean {get,set}_config() callbacks.
netdev-afxdp: Sync and clean {get,set}_config() callbacks.
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 | 104 ++++++++++++++++++--------
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, 193 insertions(+), 88 deletions(-)
Hi Jakob,
The output looks good to me. Just a few minor code related comments on the code.
@previous reviewers/committers, if anyone else is intending to review before
Jakob respins for possibly the final version, please shout now!
As it is user visible change, it's probably worth to put a note in the NEWS so
users can quickly spot if they notice a change.
Best to mention the commands/output that changed ('ovs-appctl dpctl/show' and
'ovs-vsctl get Interface <interface_name> status') and say briefly that you've
aligned set_confg/get_config and updated status etc.
Suggest to not to bother mentioning specific netdevs and just do in one update,
maybe in first patch?
thanks,
Kevin.
Good idea. How about appending this to NEWS?
Post-v3.2.0
--------------------
- OVSDB:
* ...
- ovs-appctl:
* 'ovs-appctl dpctl/show' has been changed to print valid config options
It is an appctl section, no need to repeat.
only: {configured,requested}_{rx,tx}_queues and 'rx_csum_offload' have
been dropped. Now, 'n_rxq' will be used for requested rx queues. Tx
queues cannot be changed by the user, hence 'requested_tx_queues' has
been dropped. 'lsc_interrupt_mode' has been renamed to option name
'dpdk-lsc-interrupt'.
Use 'ovs-vsctl get interface *** status' to query for values which
have
previously been returned by 'ovs-appctl dpctl/show'. For example, use
'ovs-vsctl get interface *** status:n_txq' to get what was previously
returned by 'configured_tx_queues'.
* 'ovs-appctl dpctl/show' will now print all available config options
like
'dpdk-devargs', '{rx,tx}-flow-ctrl-{autoneg}', 'vhost-server-path' and
'tx-retries-max' if they are set.
This doesn't seem to be entirely true. The flow control stuff
is always reported even if not set by the user. Should we maybe
avoid reporting default values for these somehow?
It is not handled consistently. If we decide so, we would also want to change reporting
of n_{r,t}xq_desc and all the others. Does anyone has strong opinions here? Maybe your
"maybe" was a strong opinion? 😄
The flow control is just the least used config and it is an on/off switch,
unlike the queue sizes, so it's easy for user to guess a default value.
So, it might make sense to be a little inconsistent here.
Not a strong opinion from my side.
Wdyt?
I was thinking something shorter without mentioning individual configs
would be sufficient, but I can see the benefit of listing the individual
changed configs that could be found with a grep too. Though it would be
easier to search for config names without the {}.
I'd suggest a shorter description focusing on the nature of the change
instead of particular options changed. These are not configuration
interfaces, but informational.
Maybe something along these lines:
- ovs-appctl:
* Output of 'dpctl/show' command no longer shows interface configuration
status, only values of the actual configuration options, a.k.a.
'requested' configuration. The interface configuration status,
a.k.a. 'configured' values, can be found in the 'status' column of
the Interface table, i.e. with 'ovs-vsctl get interface <..> status'.
Reported names adjusted accordingly.
What do you think?
Simple and concise 👍 I will use that. However, we could add an example,
it could make it easier to grasp the meaning.
I guess, the reference to 'configured' and 'requested' should be enough
of an example. I think, if we would add an example, it should be very
short, i.e. no longer than one extra line. This entry is already too long.
Should I put the NEWS change in one of the patches or in a separate patch 4/4?
I'd say since the netdev-dpdk changes are the most noticeable, add the
NEWS change into netdev-dpdk patch, but move the patch itself to the
end of the set, so the NEWS entry is correct.
Having a NEWS entry as a separate patch is not a good practice as if
impairs ability to git blame the NEWS file.
The plan above looks good to me too,
Kevin.
Another option would be to add something short in NEWS and to mention
individual configs in *.rst to say what changed in this version. e.g.
https://github.com/openvswitch/ovs/blob/master/Documentation/topics/dpdk/phy.rst?plain=1#L38
Other opinions ? How much details do we want in NEWS ?
We're not changing how the OVS is configured, only a format of the status
reporting. So, maybe it's not necessary to explain every single detail.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
