DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@amd.com>
To: Jiawei Wang <jiaweiw@nvidia.com>,
	viacheslavo@nvidia.com, orika@nvidia.com, thomas@monjalon.net,
	andrew.rybchenko@oktetlabs.ru,
	Aman Singh <aman.deep.singh@intel.com>,
	Yuying Zhang <yuying.zhang@intel.com>
Cc: dev@dpdk.org, rasland@nvidia.com
Subject: Re: [PATCH v5 1/2] ethdev: introduce the Tx map API for aggregated ports
Date: Thu, 16 Feb 2023 17:58:11 +0000	[thread overview]
Message-ID: <f7dfd05d-634b-0c4a-e8a1-68b1fa2b352d@amd.com> (raw)
In-Reply-To: <20230214154836.9681-2-jiaweiw@nvidia.com>

On 2/14/2023 3:48 PM, Jiawei Wang wrote:
> When multiple ports are aggregated into a single DPDK port,
> (example: Linux bonding, DPDK bonding, failsafe, etc.),
> we want to know which port use for Tx via a queue.
> 
> This patch introduces the new ethdev API
> rte_eth_dev_map_aggr_tx_affinity(), it's used to map a Tx queue
> with an aggregated port of the DPDK port (specified with port_id),
> The affinity is the number of the aggregated port.
> Value 0 means no affinity and traffic could be routed to any
> aggregated port, this is the default current behavior.
> 
> The maximum number of affinity is given by rte_eth_dev_count_aggr_ports().
> 
> Add the trace point for ethdev rte_eth_dev_count_aggr_ports()
> and rte_eth_dev_map_aggr_tx_affinity() functions.
> 
> Add the testpmd command line:
> testpmd> port config (port_id) txq (queue_id) affinity (value)
> 
> For example, there're two physical ports connected to
> a single DPDK port (port id 0), and affinity 1 stood for
> the first physical port and affinity 2 stood for the second
> physical port.
> Use the below commands to config tx phy affinity for per Tx Queue:
>         port config 0 txq 0 affinity 1
>         port config 0 txq 1 affinity 1
>         port config 0 txq 2 affinity 2
>         port config 0 txq 3 affinity 2
> 
> These commands config the Tx Queue index 0 and Tx Queue index 1 with
> phy affinity 1, uses Tx Queue 0 or Tx Queue 1 send packets,
> these packets will be sent from the first physical port, and similar
> with the second physical port if sending packets with Tx Queue 2
> or Tx Queue 3.
> 
> Signed-off-by: Jiawei Wang <jiaweiw@nvidia.com>

<...>

> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index dc0a4eb12c..1d5b3a16b2 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -6915,6 +6915,55 @@ rte_eth_buffer_split_get_supported_hdr_ptypes(uint16_t port_id, uint32_t *ptypes
>  	return j;
>  }
>  
> +int rte_eth_dev_count_aggr_ports(uint16_t port_id)
> +{
> +	struct rte_eth_dev *dev;
> +	int ret;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (*dev->dev_ops->count_aggr_ports == NULL)
> +		return -ENOTSUP;

What do you think to return a default value when dev_ops is not defined,
assuming device is not a bounded device.
Not sure which one is better for application, return a default value or
error.


> +	ret = eth_err(port_id, (*dev->dev_ops->count_aggr_ports)(port_id));
> +
> +	rte_eth_trace_count_aggr_ports(port_id, ret);
> +
> +	return ret;
> +}
> +
> +int rte_eth_dev_map_aggr_tx_affinity(uint16_t port_id, uint16_t tx_queue_id,
> +				     uint8_t affinity)
> +{
> +	struct rte_eth_dev *dev;
> +	int ret;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (tx_queue_id >= dev->data->nb_tx_queues) {
> +		RTE_ETHDEV_LOG(ERR, "Invalid Tx queue_id=%u\n", tx_queue_id);
> +		return -EINVAL;
> +	}
> +

Although documentation says this API should be called before configure,
if user misses it I guess above can crash, is there a way to add runtime
check, like checking 'dev->data->dev_configured'?


> +	if (*dev->dev_ops->map_aggr_tx_affinity == NULL)
> +		return -ENOTSUP;
> +
> +	if (dev->data->dev_started) {
> +		RTE_ETHDEV_LOG(ERR,
> +			"Port %u must be stopped to allow configuration\n",
> +			port_id);
> +		return -EBUSY;
> +	}
> +
> +	ret = eth_err(port_id, (*dev->dev_ops->map_aggr_tx_affinity)(port_id,
> +				tx_queue_id, affinity));
> +

Should API check if port_id is a bonding port before it continue with
mapping?

> +	rte_eth_trace_map_aggr_tx_affinity(port_id, tx_queue_id, affinity, ret);
> +
> +	return ret;
> +}
> +
>  RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
>  
>  RTE_INIT(ethdev_init_telemetry)
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index c129ca1eaf..07b8250eb8 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -2589,6 +2589,52 @@ int rte_eth_hairpin_bind(uint16_t tx_port, uint16_t rx_port);
>  __rte_experimental
>  int rte_eth_hairpin_unbind(uint16_t tx_port, uint16_t rx_port);
>  
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Get the number of aggregated ports.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @return
> + *   - (>=0) the number of aggregated port if success.
> + *   - (-ENOTSUP) if not supported.
> + */
> +__rte_experimental
> +int rte_eth_dev_count_aggr_ports(uint16_t port_id);


Can you please give more details in the function description, in the
context of this patch it is clear, but someone sees it first time can be
confused what is "aggregated ports" is.

What is expected value for regular pysical port, that doesn't have any
sub-devices, 0 or 1? Can you please document?


> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + *  Map a Tx queue with an aggregated port of the DPDK port (specified with port_id).
> + *  When multiple ports are aggregated into a single one,
> + *  it allows to choose which port to use for Tx via a queue.
> + *
> + *  The application should use rte_eth_dev_map_aggr_tx_affinity()
> + *  after rte_eth_dev_configure(), rte_eth_tx_queue_setup(), and
> + *  before rte_eth_dev_start().
> + *
> + * @param port_id
> + *   The identifier of the port used in rte_eth_tx_burst().
> + * @param tx_queue_id
> + *   The index of the transmit queue used in rte_eth_tx_burst().
> + *   The value must be in the range [0, nb_tx_queue - 1] previously supplied
> + *   to rte_eth_dev_configure().
> + * @param affinity
> + *   The number of the aggregated port.
> + *   Value 0 means no affinity and traffic could be routed to any aggregated port.
> + *   The first aggregated port is number 1 and so on.
> + *   The maximum number is given by rte_eth_dev_count_aggr_ports().
> + *
> + * @return
> + *   Zero if successful. Non-zero otherwise.
> + */
> +__rte_experimental
> +int rte_eth_dev_map_aggr_tx_affinity(uint16_t port_id, uint16_t tx_queue_id,
> +				     uint8_t affinity);
> +
>  /**
>   * Return the NUMA socket to which an Ethernet device is connected
>   *
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index dbc2bffe64..685aa71e51 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -300,6 +300,8 @@ EXPERIMENTAL {
>  	rte_mtr_meter_profile_get;
>  
>  	# added in 23.03
> +	rte_eth_dev_count_aggr_ports;
> +	rte_eth_dev_map_aggr_tx_affinity;
>  	rte_flow_async_create_by_index;
>  };
>  


  parent reply	other threads:[~2023-02-16 17:58 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20221221102934.13822-1-jiaweiw@nvidia.com/>
2023-02-03  5:07 ` [PATCH v3 0/2] add new PHY affinity in the flow item and Tx queue API Jiawei Wang
2023-02-03  5:07   ` [PATCH v3 1/2] ethdev: introduce the PHY affinity field in " Jiawei Wang
2023-02-03  5:07   ` [PATCH v3 2/2] ethdev: add PHY affinity match item Jiawei Wang
2023-02-03 13:33   ` [PATCH v4 0/2] add new PHY affinity in the flow item and Tx queue API Jiawei Wang
2023-02-03 13:33     ` [PATCH v4 1/2] ethdev: introduce the PHY affinity field in " Jiawei Wang
2023-02-06 15:29       ` Jiawei(Jonny) Wang
2023-02-07  9:40       ` Ori Kam
2023-02-09 19:44       ` Ferruh Yigit
2023-02-10 14:06         ` Jiawei(Jonny) Wang
2023-02-14  9:38         ` Jiawei(Jonny) Wang
2023-02-14 10:01           ` Ferruh Yigit
2023-02-03 13:33     ` [PATCH v4 2/2] ethdev: add PHY affinity match item Jiawei Wang
2023-02-14 15:48   ` [PATCH v5 0/2] Added support for Tx queue mapping with an aggregated port Jiawei Wang
2023-02-14 15:48     ` [PATCH v5 1/2] ethdev: introduce the Tx map API for aggregated ports Jiawei Wang
2023-02-15 11:41       ` Jiawei(Jonny) Wang
2023-02-16 17:42       ` Thomas Monjalon
2023-02-17  6:45         ` Jiawei(Jonny) Wang
2023-02-16 17:58       ` Ferruh Yigit [this message]
2023-02-17  6:44         ` Jiawei(Jonny) Wang
2023-02-17  8:24         ` Andrew Rybchenko
2023-02-17  9:50           ` Jiawei(Jonny) Wang
2023-02-14 15:48     ` [PATCH v5 2/2] ethdev: add Aggregated affinity match item Jiawei Wang
2023-02-16 17:46       ` Thomas Monjalon
2023-02-17  6:45         ` Jiawei(Jonny) Wang
2023-02-17 10:50   ` [PATCH v6 0/2] Add Tx queue mapping of aggregated ports Jiawei Wang
2023-02-17 10:50     ` [PATCH v6 1/2] ethdev: add " Jiawei Wang
2023-02-17 12:53       ` Ferruh Yigit
2023-02-17 12:56       ` Andrew Rybchenko
2023-02-17 12:59         ` Ferruh Yigit
2023-02-17 13:05           ` Jiawei(Jonny) Wang
2023-02-17 13:41         ` Jiawei(Jonny) Wang
2023-02-17 15:03           ` Andrew Rybchenko
2023-02-17 15:32             ` Ferruh Yigit
2023-02-17 10:50     ` [PATCH v6 2/2] ethdev: add flow matching of aggregated port Jiawei Wang
2023-02-17 13:01     ` [PATCH v6 0/2] Add Tx queue mapping of aggregated ports Ferruh Yigit
2023-02-17 13:07       ` Jiawei(Jonny) Wang
2023-02-17 15:47   ` [PATCH v7 " Jiawei Wang
2023-02-17 15:47     ` [PATCH v7 1/2] ethdev: add " Jiawei Wang
2023-02-17 15:47     ` [PATCH v7 2/2] ethdev: add flow matching of aggregated port Jiawei Wang
2023-02-17 16:45     ` [PATCH v7 0/2] Add Tx queue mapping of aggregated ports Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f7dfd05d-634b-0c4a-e8a1-68b1fa2b352d@amd.com \
    --to=ferruh.yigit@amd.com \
    --cc=aman.deep.singh@intel.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=dev@dpdk.org \
    --cc=jiaweiw@nvidia.com \
    --cc=orika@nvidia.com \
    --cc=rasland@nvidia.com \
    --cc=thomas@monjalon.net \
    --cc=viacheslavo@nvidia.com \
    --cc=yuying.zhang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).