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.
>
>>>>>> 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? 😄
>
>>>>
>>>> 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.
Should I put the NEWS change in one of the patches or in a separate patch 4/4?
>
>>> 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
