| Message ID | 20231013090759.709191-1-jmeng@redhat.com |
|---|---|
| Headers | show |
| Series | netdev: Sync and clean {get, set}_config() callbacks. | expand |
On 13/10/2023 10:07, jmeng@redhat.com wrote: > From: Jakob Meng <code@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. 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. > -- > 2.39.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 20.10.23 12:02, Kevin Traynor wrote: > On 13/10/2023 10:07, jmeng@redhat.com wrote: >> From: Jakob Meng <code@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. 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 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. Wdyt?
On 23/10/2023 10:11, Jakob Meng wrote: > On 20.10.23 12:02, Kevin Traynor wrote: >> On 13/10/2023 10:07, jmeng@redhat.com wrote: >>> From: Jakob Meng <code@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. 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 > 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. > > > 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 {}. 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 ?
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, jmeng@redhat.com wrote: >>>> From: Jakob Meng <code@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. 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 >> 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. >> >> >> 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 {}. > > 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 ?
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, jmeng@redhat.com wrote: >>>>> From: Jakob Meng <code@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.) >>>>> 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? >>> >>> >>> 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? >> >> 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. Best regards, Ilya Maximets.
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, jmeng@redhat.com wrote: >>>>>> From: Jakob Meng <code@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. >
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, jmeng@redhat.com wrote: >>>>>>> From: Jakob Meng <code@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. > >> >>>> 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. >> >
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, jmeng@redhat.com wrote: >>>>>>>> From: Jakob Meng <code@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. >>> >> >
On 27.10.23 17:25, Kevin Traynor wrote: > 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: >>>> ... >>>> 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. Thank you both, Ilya and Kevin! I have incorporated your suggestions into v7: https://patchwork.ozlabs.org/project/openvswitch/list/?series=379901 (cover letter includes list of what was changed, but it's not listed in patchwork)
On 10/30/23 10:54, Jakob Meng wrote: > On 27.10.23 17:25, Kevin Traynor wrote: >> 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: >>>>> ... >>>>> 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. > > Thank you both, Ilya and Kevin! I have incorporated your suggestions into v7: > > https://patchwork.ozlabs.org/project/openvswitch/list/?series=379901 > > (cover letter includes list of what was changed, but it's not listed in patchwork) It actually is in the patchwork, you just need to 'expand' the 'Series' after navigating to one of the patches: https://patchwork.ozlabs.org/project/openvswitch/cover/20231030095000.720854-1-jmeng@redhat.com/ Best regards, Ilya Maximets.
On 30.10.23 14:07, Ilya Maximets wrote: > On 10/30/23 10:54, Jakob Meng wrote: >> On 27.10.23 17:25, Kevin Traynor wrote: >>> 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: >>>>>> ... >>>>>> 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. >> Thank you both, Ilya and Kevin! I have incorporated your suggestions into v7: >> >> https://patchwork.ozlabs.org/project/openvswitch/list/?series=379901 >> >> (cover letter includes list of what was changed, but it's not listed in patchwork) > It actually is in the patchwork, you just need to 'expand' the 'Series' after > navigating to one of the patches: > https://patchwork.ozlabs.org/project/openvswitch/cover/20231030095000.720854-1-jmeng@redhat.com/ > > Best regards, Ilya Maximets. Ah, nice! Thank you ☺️
From: Jakob Meng <code@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. 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(-) -- 2.39.2