diff mbox

[dpdk-dev,V3,1/2] net/failsafe: convert to new Tx offloads API

Message ID 1515595223-36144-1-git-send-email-motih@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Moti Haimovsky Jan. 10, 2018, 2:40 p.m. UTC 
  Ethdev Tx offloads API has changed since:
commit cba7f53b717d ("ethdev: introduce Tx queue offloads API")
This commit adds support for the new Tx offloads API.

Signed-off-by: Moti Haimovsky <motih@mellanox.com>
---
V3:
Modification according to inputs from Stephen Hemminger
* Removed unnecessary initialization of tx_queue_offload_capa

V2:
* Fixed coding style warnings.
---
 drivers/net/failsafe/failsafe_ops.c | 44 +++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

Comments

Gaëtan Rivet Jan. 15, 2018, 3:57 p.m. UTC | #1 
Hi Moti,

Thanks for the patches.
I have some nitpicks, but overall it is good. Sorry for doing the review
so late, the changes should not take long, it's mostly syntax.

For the commit title, I'd prefer "use" instead of "convert to"

   [PATCH V3 1/2] net/failsafe: use new Tx offloads API

As the old API is still supported and used.

On Wed, Jan 10, 2018 at 04:40:22PM +0200, Moti Haimovsky wrote:

..

> @@ -84,9 +85,18 @@
>  fs_dev_configure(struct rte_eth_dev *dev)
>  {
>  	struct sub_device *sdev;
> +	uint64_t supp_tx_offloads = PRIV(dev)->infos.tx_offload_capa;
> +	uint64_t tx_offloads = dev->data->dev_conf.txmode.offloads;

I'm not fond of compound variable definition and initialization.
Initialization should be done after the definition list.

>  	uint8_t i;
>  	int ret;
>  
> +	if ((tx_offloads & supp_tx_offloads) != tx_offloads) {
> +		rte_errno = ENOTSUP;
> +		ERROR("Some Tx offloads are not supported, "
> +		      "requested 0x%lx supported 0x%lx\n",

Please use PRIx64 instead of %lx for displaying uint64_t.

> +		      tx_offloads, supp_tx_offloads);
> +		return -rte_errno;
> +	}
>  	FOREACH_SUBDEV(sdev, i, dev) {
>  		int rmv_interrupt = 0;
>  		int lsc_interrupt = 0;
> @@ -311,6 +321,22 @@
>  	return ret;
>  }
>  
> +static bool
> +fs_txq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads)

_are seems unnecessary here:

        if (fs_txq_offloads_valid() == false) {
                ...
        }

reads well enough and is shorter.

> +{
> +	uint64_t port_offloads = dev->data->dev_conf.txmode.offloads;
> +	uint64_t queue_supp_offloads = PRIV(dev)->infos.tx_queue_offload_capa;
> +	uint64_t port_supp_offloads = PRIV(dev)->infos.tx_offload_capa;
> +
> +	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> +	     offloads)
> +		return false;
> +	/* Verify we have no conflict with port offloads */
> +	if ((port_offloads ^ offloads) & port_supp_offloads)
> +		return false;
> +	return true;
> +}
> +
>  static void
>  fs_tx_queue_release(void *queue)
>  {
> @@ -347,6 +373,22 @@
>  		fs_tx_queue_release(txq);
>  		dev->data->tx_queues[tx_queue_id] = NULL;
>  	}
> +	/*
> +	 * Don't verify queue offloads for applications which
> +	 * use the old API.
> +	 */
> +	if (tx_conf != NULL &&
> +	   !!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE) &&

No need for !!

> +	   !fs_txq_are_offloads_valid(dev, tx_conf->offloads)) {

Please check against explicit "false" instead of using '!'.
diff mbox

Patch

diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index e16a590..fc1b85a 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -31,6 +31,7 @@ 
  *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <stdbool.h>
 #include <stdint.h>
 
 #include <rte_debug.h>
@@ -84,9 +85,18 @@ 
 fs_dev_configure(struct rte_eth_dev *dev)
 {
 	struct sub_device *sdev;
+	uint64_t supp_tx_offloads = PRIV(dev)->infos.tx_offload_capa;
+	uint64_t tx_offloads = dev->data->dev_conf.txmode.offloads;
 	uint8_t i;
 	int ret;
 
+	if ((tx_offloads & supp_tx_offloads) != tx_offloads) {
+		rte_errno = ENOTSUP;
+		ERROR("Some Tx offloads are not supported, "
+		      "requested 0x%lx supported 0x%lx\n",
+		      tx_offloads, supp_tx_offloads);
+		return -rte_errno;
+	}
 	FOREACH_SUBDEV(sdev, i, dev) {
 		int rmv_interrupt = 0;
 		int lsc_interrupt = 0;
@@ -311,6 +321,22 @@ 
 	return ret;
 }
 
+static bool
+fs_txq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads)
+{
+	uint64_t port_offloads = dev->data->dev_conf.txmode.offloads;
+	uint64_t queue_supp_offloads = PRIV(dev)->infos.tx_queue_offload_capa;
+	uint64_t port_supp_offloads = PRIV(dev)->infos.tx_offload_capa;
+
+	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
+	     offloads)
+		return false;
+	/* Verify we have no conflict with port offloads */
+	if ((port_offloads ^ offloads) & port_supp_offloads)
+		return false;
+	return true;
+}
+
 static void
 fs_tx_queue_release(void *queue)
 {
@@ -347,6 +373,22 @@ 
 		fs_tx_queue_release(txq);
 		dev->data->tx_queues[tx_queue_id] = NULL;
 	}
+	/*
+	 * Don't verify queue offloads for applications which
+	 * use the old API.
+	 */
+	if (tx_conf != NULL &&
+	   !!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE) &&
+	   !fs_txq_are_offloads_valid(dev, tx_conf->offloads)) {
+		rte_errno = ENOTSUP;
+		ERROR("%p: Tx queue offloads 0x%lx don't match port "
+		      "offloads 0x%lx or supported offloads 0x%lx",
+		      (void *)dev, tx_conf->offloads,
+		      dev->data->dev_conf.txmode.offloads,
+		      PRIV(dev)->infos.tx_offload_capa |
+		      PRIV(dev)->infos.tx_queue_offload_capa);
+		return -rte_errno;
+	}
 	txq = rte_zmalloc("ethdev TX queue",
 			  sizeof(*txq) +
 			  sizeof(rte_atomic64_t) * PRIV(dev)->subs_tail,
@@ -559,6 +601,8 @@ 
 		PRIV(dev)->infos.rx_offload_capa = rx_offload_capa;
 		PRIV(dev)->infos.tx_offload_capa &=
 					default_infos.tx_offload_capa;
+		PRIV(dev)->infos.tx_queue_offload_capa &=
+					default_infos.tx_queue_offload_capa;
 		PRIV(dev)->infos.flow_type_rss_offloads &=
 					default_infos.flow_type_rss_offloads;
 	}