Re: [dpdk-dev] [PATCH v4] ethdev: check Rx/Tx offloads

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 4/25/2018 12:50 PM, Wei Dai wrote:
> This patch check if a requested offloading is supported
> in the device capability.
> Any offloading is disabled by default if it is not set
> in rte_eth_dev_configure( ) and rte_eth_[rt]x_queue_setup().
> A per port offloading can only be enabled in
> rte_eth_dev_configure(). If a per port offloading is
> sent to rte_eth_[rt]x_queue_setup( ), return error.
> Only per queue offloading can be sent to
> rte_eth_[rt]x_queue_setup( ). A per queue offloading is
> enabled only if it is enabled in rte_eth_dev_configure( ) OR
> if it is enabled in rte_eth_[rt]x_queue_setup( ).
> If a per queue offloading is enabled in rte_eth_dev_configure(),
> it can't be disabled in rte_eth_[rt]x_queue_setup( ).
> If a per queue offloading is disabled in rte_eth_dev_configure( ),
> it can be enabled or disabled( ) in rte_eth_[rt]x_queue_setup( ).
> 
> This patch can make such checking in a common way in rte_ethdev
> layer to avoid same checking in underlying PMD.

Hi Wei,

For clarification, there is existing API for rc1, and there is a suggested
update in API for rc2. I guess this patch is for suggested update in rc2?

> Signed-off-by: Wei Dai <wei.dai@intel.com>
> 
> ---
> v4: fix a wrong description in git log message.
> 
> v3: rework according to dicision of offloading API in community
> 
> v2: add offloads checking in rte_eth_dev_configure( ).
>     check if a requested offloading is supported.
> ---
>  lib/librte_ether/rte_ethdev.c | 76 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index f0f53d4..70a7904 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1196,6 +1196,28 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>  							ETHER_MAX_LEN;
>  	}
>  
> +	/* Any requested offload must be within its device capability */
> +	if ((local_conf.rxmode.offloads & dev_info.rx_offload_capa) !=
> +	     local_conf.rxmode.offloads) {
> +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d requested Rx offloads "
> +				    "0x%" PRIx64 " doesn't match Rx offloads "
> +				    "capability 0x%" PRIx64 "\n",
> +				    port_id,
> +				    local_conf.rxmode.offloads,
> +				    dev_info.rx_offload_capa);
> +		return -EINVAL;
> +	}
> +	if ((local_conf.txmode.offloads & dev_info.tx_offload_capa) !=
> +	     local_conf.txmode.offloads) {
> +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d requested Tx offloads "
> +				    "0x%" PRIx64 " doesn't match Tx offloads "
> +				    "capability 0x%" PRIx64 "\n",
> +				    port_id,
> +				    local_conf.txmode.offloads,
> +				    dev_info.tx_offload_capa);
> +		return -EINVAL;
> +	}
+1 having these checks here.

> +
>  	/*
>  	 * Setup new number of RX/TX queues and reconfigure device.
>  	 */
> @@ -1547,6 +1569,33 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>  						    &local_conf.offloads);
>  	}
>  
> +	/*
> +	 * Only per-queue offload can be enabled from application.
> +	 * If any pure per-port offload is sent to this function, return -EINVAL
> +	 */
> +	if ((local_conf.offloads & dev_info.rx_queue_offload_capa) !=
> +	     local_conf.offloads) {
> +		RTE_PMD_DEBUG_TRACE("Ethdev port_id=%d rx_queue_id=%d "
> +				    "Requested offload 0x%" PRIx64 "doesn't "
> +				    "match per-queue capability 0x%" PRIx64
> +				    " in rte_eth_rx_queue_setup( )\n",
> +				    port_id,
> +				    rx_queue_id,
> +				    local_conf.offloads,
> +				    dev_info.rx_queue_offload_capa);
> +		return -EINVAL;
> +	}

There is a change here. If requested offload is already enabled in port level,
instead of returning error, ignore it.
So this removes the restriction for apps that "only an offload from queue
capabilities can be send for queue_setup() functions". This is not requirement
for application as it has been before, but this is allowed for app now.

If app tried to enable a port offload in queue level that is not already
enabled, it should still return error.

> +
> +	/*
> +	 * If a per-queue offload is enabled in rte_eth_dev_configure( ),
> +	 * it is also enabled on all queues and can't be disabled here.
> +	 * If it is diabled in rte_eth_dev_configure( ), it can be enabled
> +	 * or disabled here.
> +	 * If a per-port offload is enabled in rte_eth_dev_configure( ),
> +	 * it is also enabled for all queues here.
> +	 */
> +	local_conf.offloads |= dev->data->dev_conf.rxmode.offloads;

I didn't get this one, why add rxmode.offloads into queue offloads?

Based on above change Thomas has an suggestion [1]:

"
In the case of offload already enabled at port level
and repeated in queue setup,
ethdev can avoid passing it to the PMD queue setup function.
"

So almost reverse of what you are doing, strip rxmode.offloads from
local_conf.offloads for PMDs. What do you think?


[1]
https://dpdk.org/ml/archives/dev/2018-April/098956.html