[dpdk-dev] [PATCH v2] ethdev: add API to query proxy port to manage transfer flows

[Andrew Rybchenko]

On 10/6/21 12:10 AM, Ivan Malov wrote:
> Not all DPDK ports in a given e-switch domain may have the

Search in the Internet does not provide e-switch as a shorter
form of "embedded switch". So, I agree with Thomas, it is
better to avoid it in the documentation. Especially taking
into account that there is a company with a similar name.

I.e. e-switch -> embedded switch

> privilege to manage "transfer" flows. Add an API to find a
> port with sufficient privileges by any port in the domain.
> 
> Signed-off-by: Ivan Malov <ivan.malov at oktetlabs.ru>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>

I have to review it better before, but it is better to do it
later than never.

> Acked-by: Ori Kam <orika at nvidia.com>
> ---
> Patch series [1] has reworked support for "transfer" flows.
> This allows to elaborate on the idea which first appeared
> in RFC [2]. Hence the patch in question.
> 
> net/sfc driver is going to support the new API. The
> corresponding patch is already in progress and will
> be provided in the course of this release cycle.
> 
> [1] https://patches.dpdk.org/project/dpdk/list/?series=19326
> [2] https://patches.dpdk.org/project/dpdk/list/?series=18737

[snip]

> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> index 0787baed8c..5de30fad9c 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -67,6 +67,9 @@ New Features
>    Added macros ETH_RSS_IPV4_CHKSUM and ETH_RSS_L4_CHKSUM, now IPv4 and
>    TCP/UDP/SCTP header checksum field can be used as input set for RSS.
>  
> +* **Added an API to get a proxy port to manage "transfer" (e-switch) flows**

Let's avoid (e-switch) here. I guess initial definition of the
transfer intentionally avoid the term.

> +  A new API, ``rte_flow_pick_transfer_proxy()``, was added.
> +
>  * **Updated Broadcom bnxt PMD.**
>  
>    * Added flow offload support for Thor.
> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> index 647bbf91ce..15e978f7f7 100644
> --- a/lib/ethdev/rte_flow.c
> +++ b/lib/ethdev/rte_flow.c
> @@ -1270,3 +1270,25 @@ rte_flow_tunnel_item_release(uint16_t port_id,
>  				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>  				  NULL, rte_strerror(ENOTSUP));
>  }
> +
> +int
> +rte_flow_pick_transfer_proxy(uint16_t port_id, uint16_t *proxy_port_id,
> +			     struct rte_flow_error *error)
> +{
> +	struct rte_eth_dev *dev = &rte_eth_devices[port_id];

Let's avoid initialization here since 'port_id' is not checked
yet to be correct. Let's assign dev below just before usage.

> +	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> +
> +	if (unlikely(ops == NULL))
> +		return -rte_errno;
> +
> +	if (likely(ops->pick_transfer_proxy != NULL)) {

Yes, I see that it is the stile in the file to expect
non-NULL callback, it is a bit unfair to say so when
just driver implements it. So, I suggest to remove it.
Also it is not an error or exception. It is documented
behaviour.

> +		return flow_err(port_id,
> +				ops->pick_transfer_proxy(dev, proxy_port_id,
> +							 error),
> +				error);
> +	}
> +
> +	*proxy_port_id = port_id;

I think code will have less lines and easier to read if
we revert above if condition, do the assignment and
return 0 from its body.

> +
> +	return 0;
> +}
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> index f195aa7224..5405e2565a 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h

[snip]

> @@ -4427,6 +4430,34 @@ rte_flow_tunnel_item_release(uint16_t port_id,
>  			     struct rte_flow_item *items,
>  			     uint32_t num_of_items,
>  			     struct rte_flow_error *error);
> +
> +/**

Don't we need experimental header here?

> + * Get a proxy port to manage "transfer" (e-switch) flows.

I suggest to have no (e-switch) here as well. "transfer" is
sufficient.

> + *
> + * Managing "transfer" flows requires that the user communicate them
> + * through a port which has the privilege to control the e-switch.

e-switch -> embedded switch

> + * For some vendors, all ports in a given e-switch domain have

e-switch -> embedded switch, or "switching domain"

> + * this privilege. For other vendors, it's only one port.
> + *
> + * This API indicates such a privileged port (a "proxy")
> + * for a given port in the same e-switch domain.

e-switch -> embedded switch, or "switching domain"

> + *
> + * @note
> + *   If the PMD serving @p port_id doesn't have the corresponding method
> + *   implemented, the API will return @p port_id via @p proxy_port_id.
> + *
> + * @param port_id
> + *   Indicates the port to get a "proxy" for
> + * @param[out] proxy_port_id
> + *   Indicates the "proxy" port

I should notice it earlier, but 'error' parameter description
is missing here.

> + *
> + * @return
> + *   0 on success, a negative error code otherwise
> + */
> +__rte_experimental
> +int
> +rte_flow_pick_transfer_proxy(uint16_t port_id, uint16_t *proxy_port_id,
> +			     struct rte_flow_error *error);
>  #ifdef __cplusplus
>  }
>  #endif