Thanks for your feedback, Kevin! Added some notes inline.
On 20.10.23 12:03, 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 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/topics/dpdk/phy.rst | 4 +-
>> lib/netdev-dpdk.c | 104 +++++++++++++++++++++---------
>> tests/system-dpdk.at | 64 +++++++++++-------
>> vswitchd/vswitch.xml | 14 +++-
>> 4 files changed, 127 insertions(+), 59 deletions(-)
>>
>> 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-dpdk.c b/lib/netdev-dpdk.c
>> index 55700250d..56588f92a 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -1905,31 +1905,32 @@ 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",
>> - dev->lsc_interrupt_mode ? "true" : "false");
>> + smap_add_format(args, "n_rxq_desc", "%d", dev->rxq_size);
>> + smap_add_format(args, "n_txq_desc", "%d", dev->txq_size);
>> - if (dpdk_port_is_representor(dev)) {
>> - smap_add_format(args, "dpdk-vf-mac", ETH_ADDR_FMT,
>> - ETH_ADDR_ARGS(dev->requested_hwaddr));
>> - }
>> + if (dev->rx_steer_flags == DPDK_RX_STEER_LACP) {
>> + smap_add(args, "rx-steering", "rss+lacp");
>> + }
>> +
>> + smap_add(args, "dpdk-lsc-interrupt",
>> + dev->lsc_interrupt_mode ? "true" : "false");
>
> nit: indent
🙈 Will fix..
>
>> +
>> + if (dpdk_port_is_representor(dev)) {
>> + smap_add_format(args, "dpdk-vf-mac", ETH_ADDR_FMT,
>> + ETH_ADDR_ARGS(dev->requested_hwaddr));
>> }
>> +
>> ovs_mutex_unlock(&dev->mutex);
>> return 0;
>> @@ -2324,6 +2325,29 @@ out:
>> return err;
>> }
>> +static int
>> +netdev_dpdk_vhost_client_get_config(const struct netdev *netdev,
>> + struct smap *args)
>> +{
>> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>> +
>> + ovs_mutex_lock(&dev->mutex);
>> +
>> + if (dev->vhost_id) {
>> + smap_add(args, "vhost-server-path", dev->vhost_id);
>> + }
>> +
>
>> + int tx_retries_max;
>
> I'm not sure it breaks any hard rule, but it seems more consistent to put
> this ^ at the start of the function.
It is not handled consistently in ovs (definitiely allowed in the coding style
doc though). The rest of this file puts variable definitions at the beginning,
so I will change it as suggested.👍
>
>> + atomic_read_relaxed(&dev->vhost_tx_retries_max, &tx_retries_max);
>> + if (tx_retries_max != VHOST_ENQ_RETRY_DEF) {
>> + smap_add_format(args, "tx-retries-max", "%d", tx_retries_max);
>> + }
>> +
>> + ovs_mutex_unlock(&dev->mutex);
>> +
>> + return 0;
>> +}
>> +
>> static int
>> netdev_dpdk_vhost_client_set_config(struct netdev *netdev,
>> const struct smap *args,
>> @@ -4091,6 +4115,9 @@ netdev_dpdk_vhost_user_get_status(const struct netdev
>> *netdev,
>> smap_add_format(args, "userspace-tso", "disabled");
>> }
>> + smap_add_format(args, "n_rxq", "%d", netdev->n_rxq);
>> + smap_add_format(args, "n_txq", "%d", netdev->n_txq);
>> +
>> ovs_mutex_unlock(&dev->mutex);
>> return 0;
>> }
>> @@ -4161,6 +4188,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 +4220,21 @@ 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");
>> + if (rx_steer_flags && !rx_steer_flows_num) {
>> + smap_add(args, "rx-steering", "unsupported");
>> + } else if (rx_steer_flags == DPDK_RX_STEER_LACP) {
>> + smap_add(args, "rx-steering", "rss+lacp");
>> + } else {
>> + ovs_assert(!rx_steer_flags);
>> + smap_add(args, "rx-steering", "rss");
>> + }
>> +
>> + 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");
>> }
>> }
>
> Maybe can combine above code blocks to simplify ?
>
> if (rx_steer_flags) {
> if (!rx_steer_flows_num) {
> smap_add(args, "rx-steering", "unsupported");
> } else {
> if (rx_steering_flags == DPDK_RX_STEER_LACP) {
> smap_add(args, "rx-steering", "rss+lacp");
> }
> 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");
> }
> }
> } else {
> smap_add(args, "rx-steering", "rss");
> }
Nooo, please not 😱 I split this big code block into smaller chunks because it
is hardly readable with all those different if's 😬
>
>> @@ -6415,7 +6454,6 @@ parse_vhost_config(const struct smap
>> *ovs_other_config)
>> .is_pmd = true, \
>> .alloc = netdev_dpdk_alloc, \
>> .dealloc = netdev_dpdk_dealloc, \
>> - .get_config = netdev_dpdk_get_config, \
>> .get_numa_id = netdev_dpdk_get_numa_id, \
>> .set_etheraddr = netdev_dpdk_set_etheraddr, \
>> .get_etheraddr = netdev_dpdk_get_etheraddr, \
>> @@ -6459,6 +6497,7 @@ static const struct netdev_class dpdk_class = {
>> .type = "dpdk",
>> NETDEV_DPDK_CLASS_BASE,
>> .construct = netdev_dpdk_construct,
>> + .get_config = netdev_dpdk_get_config,
>> .set_config = netdev_dpdk_set_config,
>> .send = netdev_dpdk_eth_send,
>> };
>> @@ -6485,6 +6524,7 @@ static const struct netdev_class
>> dpdk_vhost_client_class = {
>> .init = netdev_dpdk_vhost_class_init,
>> .construct = netdev_dpdk_vhost_client_construct,
>> .destruct = netdev_dpdk_vhost_destruct,
>> + .get_config = netdev_dpdk_vhost_client_get_config,
>> .set_config = netdev_dpdk_vhost_client_set_config,
>> .send = netdev_dpdk_vhost_send,
>> .get_carrier = netdev_dpdk_vhost_get_carrier,
>> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
>> index 0f58e8574..fd42aed0b 100644
>> --- a/tests/system-dpdk.at
>> +++ b/tests/system-dpdk.at
>> @@ -588,8 +588,9 @@ AT_CHECK([ovs-vsctl show], [], [stdout])
>> sleep 2
>> dnl Check default MTU value in the datapath
>> -AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
>> -AT_CHECK([grep -E 'mtu=1500' stdout], [], [stdout])
>> +AT_CHECK([ovs-vsctl get Interface phy0 mtu], [0], [dnl
>> +1500
>> +])
>> dnl Increase MTU value and check in the datapath
>> AT_CHECK([ovs-vsctl set Interface phy0 mtu_request=9000])
>> @@ -600,8 +601,9 @@ AT_FAIL_IF([grep "Interface phy0 does not support MTU
>> configuration" ovs-vswitch
>> dnl Fail if error is encountered during MTU setup
>> AT_FAIL_IF([grep "Interface phy0 MTU (9000) setup error"
>> ovs-vswitchd.log], [], [stdout])
>> -AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
>> -AT_CHECK([grep -E 'mtu=9000' stdout], [], [stdout])
>> +AT_CHECK([ovs-vsctl get Interface phy0 mtu], [0], [dnl
>> +9000
>> +])
>> dnl Clean up
>> @@ -636,14 +638,16 @@ dnl Fail if error is encountered during MTU setup
>> AT_FAIL_IF([grep "Interface phy0 MTU (9000) setup error"
>> ovs-vswitchd.log], [], [stdout])
>> dnl Check MTU value in the datapath
>> -AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
>> -AT_CHECK([grep -E 'mtu=9000' stdout], [], [stdout])
>> +AT_CHECK([ovs-vsctl get Interface phy0 mtu], [0], [dnl
>> +9000
>> +])
>> dnl Decrease MTU value and check in the datapath
>> AT_CHECK([ovs-vsctl set Interface phy0 mtu_request=2000])
>> -AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
>> -AT_CHECK([grep -E 'mtu=2000' stdout], [], [stdout])
>> +AT_CHECK([ovs-vsctl get Interface phy0 mtu], [0], [dnl
>> +2000
>> +])
>> dnl Clean up
>> @@ -686,16 +690,19 @@ tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat
>> NUMA_NODE)" --no-pci\
>> --single-file-segments -- -a
>> >$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &
>> OVS_WAIT_UNTIL([grep "virtio is now ready for processing"
>> ovs-vswitchd.log])
>> +OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0 link_state |
>> grep -w up])
>> dnl Check default MTU value in the datapath
>> -AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
>> -AT_CHECK([grep -E 'mtu=1500' stdout], [], [stdout])
>> +AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
>> +1500
>> +])
>> dnl Increase MTU value and check in the datapath
>> AT_CHECK([ovs-vsctl set Interface dpdkvhostuserclient0 mtu_request=9000])
>> -AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
>> -AT_CHECK([grep -E 'mtu=9000' stdout], [], [stdout])
>> +AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
>> +9000
>> +])
>> dnl Clean up the testpmd now
>> pkill -f -x -9 'tail -f /dev/null'
>> @@ -743,16 +750,19 @@ tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat
>> NUMA_NODE)" --no-pci\
>> --single-file-segments -- -a
>> >$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &
>> OVS_WAIT_UNTIL([grep "virtio is now ready for processing"
>> ovs-vswitchd.log])
>> +OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0 link_state |
>> grep -w up])
>> dnl Check MTU value in the datapath
>> -AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
>> -AT_CHECK([grep -E 'mtu=9000' stdout], [], [stdout])
>> +AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
>> +9000
>> +])
>> dnl Decrease MTU value and check in the datapath
>> AT_CHECK([ovs-vsctl set Interface dpdkvhostuserclient0 mtu_request=2000])
>> -AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
>> -AT_CHECK([grep -E 'mtu=2000' stdout], [], [stdout])
>> +AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
>> +2000
>> +])
>> dnl Clean up the testpmd now
>> pkill -f -x -9 'tail -f /dev/null'
>> @@ -789,8 +799,9 @@ dnl Fail if error is encountered during MTU setup
>> AT_FAIL_IF([grep "Interface phy0 MTU (9702) setup error"
>> ovs-vswitchd.log], [], [stdout])
>> dnl Check MTU value in the datapath
>> -AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
>> -AT_CHECK([grep -E 'mtu=9702' stdout], [], [stdout])
>> +AT_CHECK([ovs-vsctl get Interface phy0 mtu], [0], [dnl
>> +9702
>> +])
>> dnl Set MTU value above upper bound and check for error
>> AT_CHECK([ovs-vsctl set Interface phy0 mtu_request=9711])
>> @@ -830,8 +841,9 @@ dnl Fail if error is encountered during MTU setup
>> AT_FAIL_IF([grep "Interface phy0 MTU (68) setup error" ovs-vswitchd.log],
>> [], [stdout])
>> dnl Check MTU value in the datapath
>> -AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
>> -AT_CHECK([grep -E 'mtu=68' stdout], [], [stdout])
>> +AT_CHECK([ovs-vsctl get Interface phy0 mtu], [0], [dnl
>> +68
>> +])
>> dnl Set MTU value below lower bound and check for error
>> AT_CHECK([ovs-vsctl set Interface phy0 mtu_request=67])
>> @@ -877,10 +889,12 @@ tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat
>> NUMA_NODE)" --no-pci\
>> --single-file-segments -- -a
>> >$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &
>> OVS_WAIT_UNTIL([grep "virtio is now ready for processing"
>> ovs-vswitchd.log])
>> +OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0 link_state |
>> grep -w up])
>> dnl Check MTU value in the datapath
>> -AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
>> -AT_CHECK([grep -E 'mtu=9702' stdout], [], [stdout])
>> +AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
>> +9702
>> +])
>> dnl Set MTU value above upper bound and check for error
>> AT_CHECK([ovs-vsctl set Interface dpdkvhostuserclient0 mtu_request=9711])
>> @@ -934,10 +948,12 @@ tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat
>> NUMA_NODE)" --no-pci\
>> --single-file-segments -- -a
>> >$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &
>> OVS_WAIT_UNTIL([grep "virtio is now ready for processing"
>> ovs-vswitchd.log])
>> +OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0 link_state |
>> grep -w up])
>> dnl Check MTU value in the datapath
>> -AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
>> -AT_CHECK([grep -E 'mtu=68' stdout], [], [stdout])
>> +AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
>> +68
>> +])
>> dnl Set MTU value below lower bound and check for error
>> AT_CHECK([ovs-vsctl set Interface dpdkvhostuserclient0 mtu_request=67])
>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>> index 1e2a1267d..dadaf5b05 100644
>> --- a/vswitchd/vswitch.xml
>> +++ b/vswitchd/vswitch.xml
>> @@ -3789,6 +3789,18 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
>> type=patch options:peer=p1 \
>> Maximum number of VMDq pools.
>> </column>
>> + <column name="status" key="n_rxq">
>> + Number of rx queues.
>> + </column>
>> +
>> + <column name="status" key="n_txq">
>> + Number of tx queues.
>> + </column>
>> +
>> + <column name="status" key="rx_csum_offload">
>> + Whether hardware has support for RX Checksum Offload or not.
>
> Better to be explicit that this is current status, rather than implying by
> saying the hardware supports it. I know the code is a check on whether
> hardare supports it, but that is also what is used when deciding whether to
> enable. So suggest to reword to something like, "Whether Rx Checksum offload
> is enabled or not"
Ack👍
>
>> + </column>
>> +
>> <column name="status" key="if_type">
>> Interface type ID according to IANA ifTYPE MIB definitions.
>> </column>
>> @@ -3807,7 +3819,7 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
>> type=patch options:peer=p1 \
>> VF representors.
>> </column>
>> - <column name="status" key="rx_steering">
>> + <column name="status" key="rx-steering">
>> Hardware Rx queue steering policy in use.
>> </column>
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
