On 9/27/23 15:24, [email protected] wrote:
> From: Jakob Meng <[email protected]>
>
> 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.
>
> 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 <[email protected]>
> ---
Hi, Jacob. Thanks for the patch!
Not a full review, but see a few comments below.
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 55700250d..2c5f0a097 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);
> - 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");
Please, align the lines to the parenthesis.
<snip>
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> index 1a54add87..fe7b807f9 100644
> --- a/lib/netdev-dummy.c
> +++ b/lib/netdev-dummy.c
> @@ -795,14 +795,23 @@ netdev_dummy_get_config(const struct netdev *dev,
> struct smap *args)
>
> dummy_packet_conn_get_config(&netdev->conn, args);
>
> + /* pcap, rxq_pcap and tx_pcap cannot be recovered because filenames have
> + * been discarded after opening file descriptors */
> +
> + smap_add_format(args, "ol_ip_csum_set_good", "%s",
> + netdev->ol_ip_csum_set_good ? "true" : "false");
> +
> + smap_add_format(args, "ol_ip_csum", "%s",
> + netdev->ol_ip_csum ? "true" : "false");
If we skip reporting default values, we may avoid changing vast majority
of the tests. These config options are not meaningful in most of them
anyways.
> +
> /* 'dummy-pmd' specific config. */
> if (!netdev_is_pmd(dev)) {
> goto exit;
> }
> - smap_add_format(args, "requested_rx_queues", "%d",
> netdev->requested_n_rxq);
> - smap_add_format(args, "configured_rx_queues", "%d", dev->n_rxq);
> - smap_add_format(args, "requested_tx_queues", "%d",
> netdev->requested_n_txq);
> - smap_add_format(args, "configured_tx_queues", "%d", dev->n_txq);
> +
> + smap_add_format(args, "n_rxq", "%d", netdev->requested_n_rxq);
> + smap_add_format(args, "n_txq", "%d", netdev->requested_n_txq);
> + smap_add_format(args, "numa_id", "%d", netdev->requested_numa_id);
>
> exit:
> ovs_mutex_unlock(&netdev->mutex);
> diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h
> index 0ecf0f748..53e68d122 100644
> --- a/lib/netdev-linux-private.h
> +++ b/lib/netdev-linux-private.h
> @@ -55,6 +55,8 @@ void netdev_linux_run(const struct netdev_class *);
> int get_stats_via_netlink(const struct netdev *netdev_,
> struct netdev_stats *stats);
>
> +int netdev_linux_get_status(const struct netdev *netdev_, struct smap *smap);
Skip the variable names. Types here are self-explanatory.
<snip>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index cfcde34ff..eb2db043f 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3386,6 +3386,29 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
> </p>
> </column>
>
> +
> + <column name="options" key="ol_ip_csum"
> + type='{"type": "boolean"}'>
> + <p>
> + Enable IP checksum offload for netdev-dummy device.
> + </p>
> + <p>
> + Note that this option is specific to netdev-dummy.
> + Defaults to false.
> + </p>
> + </column>
> +
> + <column name="options" key="ol_ip_csum_set_good"
> + type='{"type": "boolean"}'>
> + <p>
> + Flag RX packets for netdev-dummy devices to have good checksums.
> + </p>
> + <p>
> + Note that this option is specific to netdev-dummy.
> + Defaults to false.
> + </p>
> + </column>
> +
Please, don't document options for dummy ports/datapath, they are confusing
for the users.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
