| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 16 Aug 2023 10:27:45 -0700
From: Brett Creeley <bcreeley@....com>
To: Wenjun Wu <wenjun1.wu@...el.com>, intel-wired-lan@...ts.osuosl.org,
netdev@...r.kernel.org
Cc: xuejun.zhang@...el.com, madhu.chittim@...el.com, qi.z.zhang@...el.com,
anthony.l.nguyen@...el.com
Subject: Re: [PATCH iwl-next v2 4/5] iavf: Add devlink port function rate API
support
On 8/7/2023 6:57 PM, Wenjun Wu wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> From: Jun Zhang <xuejun.zhang@...el.com>
>
> To allow user to configure queue based parameters, devlink port function
> rate api functions are added for setting node tx_max and tx_share
> parameters.
>
> iavf rate tree with root node and queue nodes is created and registered
> with devlink rate when iavf adapter is configured.
>
> Signed-off-by: Jun Zhang <xuejun.zhang@...el.com>
> ---
> .../net/ethernet/intel/iavf/iavf_devlink.c | 270 +++++++++++++++++-
> .../net/ethernet/intel/iavf/iavf_devlink.h | 21 ++
> drivers/net/ethernet/intel/iavf/iavf_main.c | 7 +-
> 3 files changed, 295 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_devlink.c b/drivers/net/ethernet/intel/iavf/iavf_devlink.c
> index 991d041e5922..a2bd5295c216 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_devlink.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_devlink.c
> @@ -4,7 +4,273 @@
> #include "iavf.h"
> #include "iavf_devlink.h"
>
> -static const struct devlink_ops iavf_devlink_ops = {};
> +/**
> + * iavf_devlink_rate_init_rate_tree - export rate tree to devlink rate
> + * @adapter: iavf adapter struct instance
> + *
> + * This function builds Rate Tree based on iavf adapter configuration
> + * and exports it's contents to devlink rate.
> + */
> +void iavf_devlink_rate_init_rate_tree(struct iavf_adapter *adapter)
> +{
> + struct iavf_devlink *dl_priv = devlink_priv(adapter->devlink);
> + struct iavf_dev_rate_node *iavf_r_node;
> + struct iavf_dev_rate_node *iavf_q_node;
> + struct devlink_rate *dl_root_node;
> + struct devlink_rate *dl_tmp_node;
> + int q_num, size, i;
> +
> + if (!adapter->devlink_port.registered)
> + return;
> +
> + iavf_r_node = &dl_priv->root_node;
> + memset(iavf_r_node, 0, sizeof(*iavf_r_node));
> + iavf_r_node->tx_max = adapter->link_speed;
> + strscpy(iavf_r_node->name, "iavf_root", IAVF_RATE_NODE_NAME);
> +
> + devl_lock(adapter->devlink);
> + dl_root_node = devl_rate_node_create(adapter->devlink, iavf_r_node,
> + iavf_r_node->name, NULL);
> + if (!dl_root_node || IS_ERR(dl_root_node))
> + goto err_node;
> +
> + iavf_r_node->rate_node = dl_root_node;
> +
> + /* Allocate queue nodes, and chain them under root */
> + q_num = adapter->num_active_queues;
> + if (q_num > 0) {
> + size = q_num * sizeof(struct iavf_dev_rate_node);
> + dl_priv->queue_nodes = kzalloc(size, GFP_KERNEL);
> + if (!dl_priv->queue_nodes)
> + goto err_node;
> +
> + memset(dl_priv->queue_nodes, 0, size);
Why not just use kcalloc() here? Also, it seems like there's no need to
zero the memory here.
> +
> + for (i = 0; i < q_num; ++i) {
> + iavf_q_node = &dl_priv->queue_nodes[i];
> + snprintf(iavf_q_node->name, IAVF_RATE_NODE_NAME,
> + "txq_%d", i);
> + dl_tmp_node = devl_rate_node_create(adapter->devlink,
> + iavf_q_node,
> + iavf_q_node->name,
> + dl_root_node);
> + if (!dl_tmp_node || IS_ERR(dl_tmp_node)) {
> + kfree(dl_priv->queue_nodes); > + goto err_node;
> + }
> +
> + iavf_q_node->rate_node = dl_tmp_node;
> + iavf_q_node->tx_max = IAVF_TX_DEFAULT;
> + iavf_q_node->tx_share = 0;
> + }
> + }
> +
> + dl_priv->update_in_progress = false;
> + dl_priv->iavf_dev_rate_initialized = true;
> + devl_unlock(adapter->devlink);
> + return;
> +err_node:
> + devl_rate_nodes_destroy(adapter->devlink);
> + dl_priv->iavf_dev_rate_initialized = false;
> + devl_unlock(adapter->devlink);
> +}
> +
> +/**
> + * iavf_devlink_rate_deinit_rate_tree - Unregister rate tree with devlink rate
> + * @adapter: iavf adapter struct instance
> + *
> + * This function unregisters the current iavf rate tree registered with devlink
> + * rate and frees resources.
> + */
> +void iavf_devlink_rate_deinit_rate_tree(struct iavf_adapter *adapter)
> +{
> + struct iavf_devlink *dl_priv = devlink_priv(adapter->devlink);
> +
> + if (!dl_priv->iavf_dev_rate_initialized)
> + return;
> +
> + devl_lock(adapter->devlink);
> + devl_rate_leaf_destroy(&adapter->devlink_port);
> + devl_rate_nodes_destroy(adapter->devlink);
> + kfree(dl_priv->queue_nodes);
> + devl_unlock(adapter->devlink);
> +}
> +
> +/**
> + * iavf_check_update_config - check if updating queue parameters needed
> + * @adapter: iavf adapter struct instance
> + * @node: iavf rate node struct instance
> + *
> + * This function sets queue bw & quanta size configuration if all
> + * queue parameters are set
> + */
> +static int iavf_check_update_config(struct iavf_adapter *adapter,
> + struct iavf_dev_rate_node *node)
> +{
> + /* Update queue bw if any one of the queues have been fully updated by
> + * user, the other queues either use the default value or the last
> + * fully updated value
> + */
> + if (node->tx_update_flag ==
> + (IAVF_FLAG_TX_MAX_UPDATED | IAVF_FLAG_TX_SHARE_UPDATED)) {
> + node->tx_max = node->tx_max_temp;
> + node->tx_share = node->tx_share_temp;
> + } else {
> + return 0;
> + }
I think it would more readable to do the following:
if (node->tx_update_flag !=
(IAVE_FLAG_TX_MAX_UPDATED | IAVF_FLAG_TX_SHARE_UPDATED))
return 0;
/* rest of function */
> +
> + /* Reconfig queue bw only when iavf driver on running state */
> + if (adapter->state != __IAVF_RUNNING)
> + return -EBUSY;
> +
> + return 0;
> +}
> +
> +/**
> + * iavf_update_queue_tx_share - sets tx min parameter
> + * @adapter: iavf adapter struct instance
> + * @node: iavf rate node struct instance
> + * @bw: bandwidth in bytes per second
> + * @extack: extended netdev ack structure
> + *
> + * This function sets min BW limit.
> + */
> +static int iavf_update_queue_tx_share(struct iavf_adapter *adapter,
> + struct iavf_dev_rate_node *node,
> + u64 bw, struct netlink_ext_ack *extack)
> +{
> + struct iavf_devlink *dl_priv = devlink_priv(adapter->devlink);
> + u64 tx_share_sum = 0;
> +
> + /* Keep in kbps */
> + node->tx_share_temp = div_u64(bw, IAVF_RATE_DIV_FACTOR);
> +
> + if (ADV_LINK_SUPPORT(adapter)) {
> + int i;
> +
> + for (i = 0; i < adapter->num_active_queues; ++i) {
> + if (node != &dl_priv->queue_nodes[i])
> + tx_share_sum +=
> + dl_priv->queue_nodes[i].tx_share;
> + else
> + tx_share_sum += node->tx_share_temp;
> + }
> +
> + if (tx_share_sum / 1000 > adapter->link_speed_mbps)
> + return -EINVAL;
> + }
> +
> + node->tx_update_flag |= IAVF_FLAG_TX_SHARE_UPDATED;
> + return iavf_check_update_config(adapter, node);
> +}
> +
> +/**
> + * iavf_update_queue_tx_max - sets tx max parameter
> + * @adapter: iavf adapter struct instance
> + * @node: iavf rate node struct instance
> + * @bw: bandwidth in bytes per second
> + * @extack: extended netdev ack structure
> + *
> + * This function sets max BW limit.
> + */
> +static int iavf_update_queue_tx_max(struct iavf_adapter *adapter,
> + struct iavf_dev_rate_node *node,
> + u64 bw, struct netlink_ext_ack *extack)
> +{
> + /* Keep in kbps */
> + node->tx_max_temp = div_u64(bw, IAVF_RATE_DIV_FACTOR);
> + if (ADV_LINK_SUPPORT(adapter)) {
> + if (node->tx_max_temp / 1000 > adapter->link_speed_mbps)
> + return -EINVAL;
> + }
> +
> + node->tx_update_flag |= IAVF_FLAG_TX_MAX_UPDATED;
> +
> + return iavf_check_update_config(adapter, node);
> +}
> +
> +/**
> + * iavf_devlink_rate_node_tx_max_set - devlink_rate API for setting tx max
> + * @rate_node: devlink rate struct instance
> + *
> + * This function implements rate_node_tx_max_set function of devlink_ops
> + */
> +static int iavf_devlink_rate_node_tx_max_set(struct devlink_rate *rate_node,
> + void *priv, u64 tx_max,
> + struct netlink_ext_ack *extack)
> +{
> + struct iavf_dev_rate_node *node = priv;
> + struct iavf_devlink *dl_priv;
> + struct iavf_adapter *adapter;
> +
> + if (!node)
> + return 0;
> +
> + dl_priv = devlink_priv(rate_node->devlink);
> + adapter = dl_priv->devlink_ref;
> +
> + /* Check if last update is in progress */
> + if (dl_priv->update_in_progress)
> + return -EBUSY;
> +
> + if (node == &dl_priv->root_node)
> + return 0;
> +
> + return iavf_update_queue_tx_max(adapter, node, tx_max, extack);
> +}
> +
> +/**
> + * iavf_devlink_rate_node_tx_share_set - devlink_rate API for setting tx share
> + * @rate_node: devlink rate struct instance
> + *
> + * This function implements rate_node_tx_share_set function of devlink_ops
> + */
> +static int iavf_devlink_rate_node_tx_share_set(struct devlink_rate *rate_node,
> + void *priv, u64 tx_share,
> + struct netlink_ext_ack *extack)
> +{
> + struct iavf_dev_rate_node *node = priv;
> + struct iavf_devlink *dl_priv;
> + struct iavf_adapter *adapter;
> +
> + if (!node)
> + return 0;
> +
> + dl_priv = devlink_priv(rate_node->devlink);
> + adapter = dl_priv->devlink_ref;
> +
> + /* Check if last update is in progress */
> + if (dl_priv->update_in_progress)
> + return -EBUSY;
> +
> + if (node == &dl_priv->root_node)
> + return 0;
> +
> + return iavf_update_queue_tx_share(adapter, node, tx_share, extack);
> +}
> +
> +static int iavf_devlink_rate_node_del(struct devlink_rate *rate_node,
> + void *priv,
> + struct netlink_ext_ack *extack)
> +{
> + return -EINVAL;
> +}
> +
> +static int iavf_devlink_set_parent(struct devlink_rate *devlink_rate,
> + struct devlink_rate *parent,
> + void *priv, void *parent_priv,
> + struct netlink_ext_ack *extack)
> +{
> + return -EINVAL;
> +}
> +
> +static const struct devlink_ops iavf_devlink_ops = {
> + .rate_node_tx_share_set = iavf_devlink_rate_node_tx_share_set,
> + .rate_node_tx_max_set = iavf_devlink_rate_node_tx_max_set,
> + .rate_node_del = iavf_devlink_rate_node_del,
> + .rate_leaf_parent_set = iavf_devlink_set_parent,
> + .rate_node_parent_set = iavf_devlink_set_parent,
> +};
>
> /**
> * iavf_devlink_register - Register allocated devlink instance for iavf adapter
> @@ -30,7 +296,7 @@ int iavf_devlink_register(struct iavf_adapter *adapter)
> adapter->devlink = devlink;
> ref = devlink_priv(devlink);
> ref->devlink_ref = adapter;
> -
> + ref->iavf_dev_rate_initialized = false;
> devlink_register(devlink);
>
> return 0;
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_devlink.h b/drivers/net/ethernet/intel/iavf/iavf_devlink.h
> index 5c122278611a..897ff5fc87af 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_devlink.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf_devlink.h
> @@ -4,14 +4,35 @@
> #ifndef _IAVF_DEVLINK_H_
> #define _IAVF_DEVLINK_H_
>
> +#define IAVF_RATE_NODE_NAME 12
> +struct iavf_dev_rate_node {
> + char name[IAVF_RATE_NODE_NAME];
> + struct devlink_rate *rate_node;
> + u8 tx_update_flag;
> +#define IAVF_FLAG_TX_SHARE_UPDATED BIT(0)
> +#define IAVF_FLAG_TX_MAX_UPDATED BIT(1)
> + u64 tx_max;
> + u64 tx_share;
> + u64 tx_max_temp;
> + u64 tx_share_temp;
> +#define IAVF_RATE_DIV_FACTOR 125
> +#define IAVF_TX_DEFAULT 100000
> +};
> +
> /* iavf devlink structure pointing to iavf adapter */
> struct iavf_devlink {
> struct iavf_adapter *devlink_ref; /* ref to iavf adapter */
> + struct iavf_dev_rate_node root_node;
> + struct iavf_dev_rate_node *queue_nodes;
> + bool iavf_dev_rate_initialized;
> + bool update_in_progress;
It seems like this is never true until patch 5/5, so IMO it makes sense
to add it there. Also, why is this bool needed? Is there not another
flag or lock that can be used instead of adding this new flag/bit?
> };
>
> int iavf_devlink_register(struct iavf_adapter *adapter);
> void iavf_devlink_unregister(struct iavf_adapter *adapter);
> int iavf_devlink_port_register(struct iavf_adapter *adapter);
> void iavf_devlink_port_unregister(struct iavf_adapter *adapter);
> +void iavf_devlink_rate_init_rate_tree(struct iavf_adapter *adapter);
> +void iavf_devlink_rate_deinit_rate_tree(struct iavf_adapter *adapter);
>
> #endif /* _IAVF_DEVLINK_H_ */
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index db010e68d5d2..7348b65f9f19 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -2037,6 +2037,7 @@ static void iavf_finish_config(struct work_struct *work)
> iavf_free_rss(adapter);
> iavf_free_misc_irq(adapter);
> iavf_reset_interrupt_capability(adapter);
> + iavf_devlink_rate_deinit_rate_tree(adapter);
> iavf_devlink_port_unregister(adapter);
> iavf_change_state(adapter,
> __IAVF_INIT_CONFIG_ADAPTER);
> @@ -2709,8 +2710,10 @@ static void iavf_init_config_adapter(struct iavf_adapter *adapter)
> if (err)
> goto err_sw_init;
>
> - if (!adapter->netdev_registered)
> + if (!adapter->netdev_registered) {
> iavf_devlink_port_register(adapter);
> + iavf_devlink_rate_init_rate_tree(adapter);
> + }
>
> netif_carrier_off(netdev);
> adapter->link_up = false;
> @@ -2753,6 +2756,7 @@ static void iavf_init_config_adapter(struct iavf_adapter *adapter)
> err_mem:
> iavf_free_rss(adapter);
> iavf_free_misc_irq(adapter);
> + iavf_devlink_rate_deinit_rate_tree(adapter);
> iavf_devlink_port_unregister(adapter);
> err_sw_init:
> iavf_reset_interrupt_capability(adapter);
> @@ -5150,6 +5154,7 @@ static void iavf_remove(struct pci_dev *pdev)
> err);
> }
>
> + iavf_devlink_rate_deinit_rate_tree(adapter);
> iavf_devlink_port_unregister(adapter);
> iavf_devlink_unregister(adapter);
>
> --
> 2.34.1
>
>
Powered by blists - more mailing lists