From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet Subject: Re: [PATCH V3 1/2] net/failsafe: convert to new Tx offloads API Date: Mon, 15 Jan 2018 16:57:36 +0100 Message-ID: <20180115155736.jk6uzuu5bqj4mcff@bidouze.vm.6wind.com> References: <1515095458-186363-2-git-send-email-motih@mellanox.com> <1515595223-36144-1-git-send-email-motih@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: ferruh.yigit@intel.com, stephen@networkplumber.org, dev@dpdk.org To: Moti Haimovsky Return-path: Received: from mail-wm0-f49.google.com (mail-wm0-f49.google.com [74.125.82.49]) by dpdk.org (Postfix) with ESMTP id 64DFEA56E for ; Mon, 15 Jan 2018 16:57:50 +0100 (CET) Received: by mail-wm0-f49.google.com with SMTP id i186so2766162wmi.4 for ; Mon, 15 Jan 2018 07:57:50 -0800 (PST) Content-Disposition: inline In-Reply-To: <1515595223-36144-1-git-send-email-motih@mellanox.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 '!'. -- Gaëtan Rivet 6WIND