diff mbox

[dpdk-dev,v5] ethdev: check Rx/Tx offloads

Message ID 1524753466-17021-1-git-send-email-wei.dai@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation fail apply issues

Commit Message

Wei Dai April 26, 2018, 2:37 p.m. UTC 
  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.

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 56 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

Comments

Ferruh Yigit April 26, 2018, 3:50 p.m. UTC | #1 
On 4/26/2018 3:37 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.
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>
> ---
>  lib/librte_ether/rte_ethdev.c | 56 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index f0f53d4..5485f47 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;
> +	}
> +
>  	/*
>  	 * Setup new number of RX/TX queues and reconfigure device.
>  	 */
> @@ -1547,6 +1569,23 @@ 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

This comment doesn't match below check, below doesn't check pure per-port offload

> +	 */
> +	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;
> +	}

Lets add one more check and consider this for rc1, later we can update checks
for suggested API in rc2.

Check to add (this is for existing API)
If a pure per-port update enabled configure, it should be enabled in
queue_setup() as well.


> +
>  	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
>  					      socket_id, &local_conf, mp);
>  	if (!ret) {
> @@ -1681,6 +1720,23 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>  					  &local_conf.offloads);
>  	}
>  
> +	/*
> +	 * Only per-queue offload can be enabled from applcation.
> +	 * If any pure per-port offload is sent to this function, return -EINVAL
> +	 */
> +	if ((local_conf.offloads & dev_info.tx_queue_offload_capa) !=
> +	     local_conf.offloads) {
> +		RTE_PMD_DEBUG_TRACE("Ethdev port_id=%d tx_queue_id=%d "
> +				    "Requested offload 0x%" PRIx64 "doesn't "
> +				    "match per-queue capability 0x%" PRIx64
> +				    " in rte_eth_tx_queue_setup( )\n",
> +				    port_id,
> +				    tx_queue_id,
> +				    local_conf.offloads,
> +				    dev_info.tx_queue_offload_capa);
> +		return -EINVAL;
> +	}
> +
>  	return eth_err(port_id, (*dev->dev_ops->tx_queue_setup)(dev,
>  		       tx_queue_id, nb_tx_desc, socket_id, &local_conf));
>  }
>
Thomas Monjalon April 26, 2018, 3:56 p.m. UTC | #2 
26/04/2018 17:50, Ferruh Yigit:
> Lets add one more check and consider this for rc1, later we can update checks
> for suggested API in rc2.

I don't think checks are required for RC1.
Can we take time to properly settle it in RC2?
Ferruh Yigit April 26, 2018, 3:59 p.m. UTC | #3 
On 4/26/2018 4:56 PM, Thomas Monjalon wrote:
> 26/04/2018 17:50, Ferruh Yigit:
>> Lets add one more check and consider this for rc1, later we can update checks
>> for suggested API in rc2.
> 
> I don't think checks are required for RC1.
> Can we take time to properly settle it in RC2?

That is OK, I am always having a hesitation about the target of this patch, if
it is trying to implement existing one or suggested one, that is the reason.

Let me update my comment for suggested API change.
Ferruh Yigit April 26, 2018, 4:11 p.m. UTC | #4 
On 4/26/2018 4:50 PM, Ferruh Yigit wrote:
> On 4/26/2018 3:37 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.
>>
>> Signed-off-by: Wei Dai <wei.dai@intel.com>
>> ---
>>  lib/librte_ether/rte_ethdev.c | 56 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>
>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>> index f0f53d4..5485f47 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;
>> +	}
>> +
>>  	/*
>>  	 * Setup new number of RX/TX queues and reconfigure device.
>>  	 */
>> @@ -1547,6 +1569,23 @@ 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
> 
> This comment doesn't match below check, below doesn't check pure per-port offload
> 
>> +	 */
>> +	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;
>> +	}
> 
> Lets add one more check and consider this for rc1, later we can update checks
> for suggested API in rc2.
> 
> Check to add (this is for existing API)
> If a pure per-port update enabled configure, it should be enabled in
> queue_setup() as well.

According comment from Thomas, lets target directly rc2, new API. For that I
think we still need an update to this patch.

In previous version of this patch there was a comment to strip port_offloads
from requested offloads before passing vales to PMD.
Just to highlight not strip port_capability, but port_offloads set by configure()

The logic is if an offload set in configure(), both port level offload or queue
level offload, no need to duplicate it for PMD.

> 
> 
>> +
>>  	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
>>  					      socket_id, &local_conf, mp);
>>  	if (!ret) {
>> @@ -1681,6 +1720,23 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>>  					  &local_conf.offloads);
>>  	}
>>  
>> +	/*
>> +	 * Only per-queue offload can be enabled from applcation.
>> +	 * If any pure per-port offload is sent to this function, return -EINVAL
>> +	 */
>> +	if ((local_conf.offloads & dev_info.tx_queue_offload_capa) !=
>> +	     local_conf.offloads) {
>> +		RTE_PMD_DEBUG_TRACE("Ethdev port_id=%d tx_queue_id=%d "
>> +				    "Requested offload 0x%" PRIx64 "doesn't "
>> +				    "match per-queue capability 0x%" PRIx64
>> +				    " in rte_eth_tx_queue_setup( )\n",
>> +				    port_id,
>> +				    tx_queue_id,
>> +				    local_conf.offloads,
>> +				    dev_info.tx_queue_offload_capa);
>> +		return -EINVAL;
>> +	}
>> +
>>  	return eth_err(port_id, (*dev->dev_ops->tx_queue_setup)(dev,
>>  		       tx_queue_id, nb_tx_desc, socket_id, &local_conf));
>>  }
>>
>
diff mbox

Patch

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index f0f53d4..5485f47 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;
+	}
+
 	/*
 	 * Setup new number of RX/TX queues and reconfigure device.
 	 */
@@ -1547,6 +1569,23 @@  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;
+	}
+
 	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
 					      socket_id, &local_conf, mp);
 	if (!ret) {
@@ -1681,6 +1720,23 @@  rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
 					  &local_conf.offloads);
 	}
 
+	/*
+	 * Only per-queue offload can be enabled from applcation.
+	 * If any pure per-port offload is sent to this function, return -EINVAL
+	 */
+	if ((local_conf.offloads & dev_info.tx_queue_offload_capa) !=
+	     local_conf.offloads) {
+		RTE_PMD_DEBUG_TRACE("Ethdev port_id=%d tx_queue_id=%d "
+				    "Requested offload 0x%" PRIx64 "doesn't "
+				    "match per-queue capability 0x%" PRIx64
+				    " in rte_eth_tx_queue_setup( )\n",
+				    port_id,
+				    tx_queue_id,
+				    local_conf.offloads,
+				    dev_info.tx_queue_offload_capa);
+		return -EINVAL;
+	}
+
 	return eth_err(port_id, (*dev->dev_ops->tx_queue_setup)(dev,
 		       tx_queue_id, nb_tx_desc, socket_id, &local_conf));
 }