From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f193.google.com (mail-wr0-f193.google.com [209.85.128.193]) by dpdk.org (Postfix) with ESMTP id 0D2271B20D for ; Wed, 3 Jan 2018 18:29:37 +0100 (CET) Received: by mail-wr0-f193.google.com with SMTP id w107so2313726wrb.9 for ; Wed, 03 Jan 2018 09:29:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=GRjE+wzoopghUAQOsJGrEFWVj8I/OzDHTDnnUh+IUPw=; b=iexgNHJn/etQLXUsATg4yYi1JAIdexhKy4oK93qJsn6yVPXn7yS6G0WvCsV5rBQBs3 SoCVQPc6xjc8LCf5nvaioMNHKSyi5SGSubTj+xFP1DnytyTUyCjG+wmyh7ZQq09M/2qx lMngmC0VuR0uSHvjjACMJc+Mi1nKCL1uh6HRFmMryY9fmwQ3t4NiztFWo9kHwaoJ4g93 aod217ynDqiXbpSNP923crp3r+n8u7jKVg+dbqILS8rhvxVSLkYpNkmaGgaI+pu97CIY kHFwOl/XKSGBYCKuV5asZ9yxODVzxHAhkRrhitT5NfeYaMane36k2yf9/QVif+noAzGB 2v6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=GRjE+wzoopghUAQOsJGrEFWVj8I/OzDHTDnnUh+IUPw=; b=Nska+93Xms8Njr/EuKLN9ScETisxvF/z61OdBZbVRAE9EzQFK/W4JSDV9JyGc9j6pg cGMhlrsD/XRZbLnvT3UeCmfn/QVOMj80SeoHHbv/SkO/VK5ZcrHGbPPpYgSuwRotxpvr cA8U46gAJS2rr9bgGSKFvHhafbbfWDnXY5UDgEy7ou4NhPWqcOI5Xk5Gzix0JIxg4ayM m7uGNOFbD8RQsTWp6B9A6KLTAb4G3mLcqoIMbBZFrudMWk2bDAx3m9PvHiuVzIvlRmF4 g52DCMMTfQ5m9QKY/eA5088c2ccGaww5PIvG1CI0DIGtP/bsGimKUaVmBCu2lacbDNj+ +NSw== X-Gm-Message-State: AKGB3mLFTBixKrUqNBNQxcoUYXHznFjPaPMUzEzF/gqGcslJNt5IIBg8 NR3cYB+2hJB1J0X6lkSM7zJZLQ== X-Google-Smtp-Source: ACJfBotKtSyw8l1CJ3/cXjSvcYIR1/I3yQczPrGm5GuL/6ty1i+3f7G3YDvTBdDsr6LBTpYE/30JBA== X-Received: by 10.223.136.118 with SMTP id e51mr2294706wre.21.1515000577611; Wed, 03 Jan 2018 09:29:37 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id y137sm2231485wme.0.2018.01.03.09.29.35 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 03 Jan 2018 09:29:36 -0800 (PST) Date: Wed, 3 Jan 2018 18:29:24 +0100 From: Adrien Mazarguil To: Shahaf Shuler Cc: nelio.laranjeiro@6wind.com, yskoh@mellanox.com, dev@dpdk.org Message-ID: <20180103172924.GE4256@6wind.com> References: <20171123120252.143695-1-shahafs@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [dpdk-dev] [PATCH v2 6/7] net/mlx4: convert to new Tx offloads API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Jan 2018 17:29:38 -0000 Hi Shahaf, Some relatively minor nits mostly unrelated to functionality, please see below. On Wed, Jan 03, 2018 at 09:16:16AM +0200, Shahaf Shuler wrote: > Ethdev Tx offloads API has changed since: > > commit cba7f53b717d ("ethdev: introduce Tx queue offloads API") > > This commit support the new Tx offloads API. > > Signed-off-by: Shahaf Shuler > --- > drivers/net/mlx4/mlx4_ethdev.c | 7 +--- > drivers/net/mlx4/mlx4_rxtx.h | 1 + > drivers/net/mlx4/mlx4_txq.c | 71 +++++++++++++++++++++++++++++++++++-- > 3 files changed, 70 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/mlx4/mlx4_ethdev.c b/drivers/net/mlx4/mlx4_ethdev.c > index 2f69e7d4f..63e00b1da 100644 > --- a/drivers/net/mlx4/mlx4_ethdev.c > +++ b/drivers/net/mlx4/mlx4_ethdev.c > @@ -767,17 +767,12 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info) > info->max_tx_queues = max; > info->max_mac_addrs = RTE_DIM(priv->mac); > info->rx_offload_capa = 0; > - info->tx_offload_capa = 0; > + info->tx_offload_capa = mlx4_priv_get_tx_port_offloads(priv); > if (priv->hw_csum) { > - info->tx_offload_capa |= (DEV_TX_OFFLOAD_IPV4_CKSUM | > - DEV_TX_OFFLOAD_UDP_CKSUM | > - DEV_TX_OFFLOAD_TCP_CKSUM); > info->rx_offload_capa |= (DEV_RX_OFFLOAD_IPV4_CKSUM | > DEV_RX_OFFLOAD_UDP_CKSUM | > DEV_RX_OFFLOAD_TCP_CKSUM); > } > - if (priv->hw_csum_l2tun) > - info->tx_offload_capa |= DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM; > if (mlx4_get_ifname(priv, &ifname) == 0) > info->if_index = if_nametoindex(ifname); > info->hash_key_size = MLX4_RSS_HASH_KEY_SIZE; > diff --git a/drivers/net/mlx4/mlx4_rxtx.h b/drivers/net/mlx4/mlx4_rxtx.h > index b93e2bcda..91971c4fb 100644 > --- a/drivers/net/mlx4/mlx4_rxtx.h > +++ b/drivers/net/mlx4/mlx4_rxtx.h > @@ -184,6 +184,7 @@ int mlx4_tx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, > uint16_t desc, unsigned int socket, > const struct rte_eth_txconf *conf); > void mlx4_tx_queue_release(void *dpdk_txq); > +uint64_t mlx4_priv_get_tx_port_offloads(struct priv *priv); No need for "priv_" prefixes (or anywhere in function names) in the mlx4 PMD anymore since DPDK 17.11. Visible symbols only need to be prefixed with "mlx4_" to tell them apart from mlx5's. Also the declaration in mlx4_rxtx.h should appear in the same order as in mlx4_txq.c, that is, before mlx4_tx_queue_setup(). > /** > * Get memory region (MR) <-> memory pool (MP) association from txq->mp2mr[]. > diff --git a/drivers/net/mlx4/mlx4_txq.c b/drivers/net/mlx4/mlx4_txq.c > index d651e4980..f74e4a735 100644 > --- a/drivers/net/mlx4/mlx4_txq.c > +++ b/drivers/net/mlx4/mlx4_txq.c > @@ -182,6 +182,53 @@ mlx4_txq_fill_dv_obj_info(struct txq *txq, struct mlx4dv_obj *mlxdv) > } > > /** > + * Returns the per-port supported offloads. > + * > + * @param priv > + * Pointer to private structure. > + * > + * @return > + * Supported Tx offloads. > + */ > +uint64_t > +mlx4_priv_get_tx_port_offloads(struct priv *priv) > +{ Please remove "priv_" as previously described. > + uint64_t offloads = DEV_TX_OFFLOAD_MULTI_SEGS; > + > + if (priv->hw_csum) { > + offloads |= (DEV_TX_OFFLOAD_IPV4_CKSUM | > + DEV_TX_OFFLOAD_UDP_CKSUM | > + DEV_TX_OFFLOAD_TCP_CKSUM); > + } > + if (priv->hw_csum_l2tun) > + offloads |= DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM; > + Unnecessary empty line. > + return offloads; > +} > + > +/** > + * Checks if the per-queue offload configuration is valid. > + * > + * @param priv > + * Pointer to private structure. > + * @param offloads > + * Per-queue offloads configuration. > + * > + * @return > + * 1 if the configuration is valid, 0 otherwise. Better described as "Nonzero when configuration is valid." > + */ > +static int > +priv_is_tx_queue_offloads_allowed(struct priv *priv, uint64_t offloads) s/priv_/mlx4_/ (no prefix also allowed since it's static) Not be super picky, "is" followed by "offloads allowed" sounds odd, how about: [mlx4_]check_tx_queue_offloads() > +{ > + uint64_t port_offloads = priv->dev->data->dev_conf.txmode.offloads; > + uint64_t port_supp_offloads = mlx4_priv_get_tx_port_offloads(priv); Instead of a redundant "port_", how about clarifying it all as follows: offloads -> requested port_offloads -> mandatory port_supp_offloads -> supported > + > + if ((port_offloads ^ offloads) & port_supp_offloads) > + return 0; > + return 1; And simplify this as: return !((mandatory ^ requested) & supported); Maybe I missed something, but there seems to be an inconsistency, e.g. requesting an unsupported offload does not necessarily fail: mandatory = 0x00 requested = 0x40 supported = 0x10 => valid but shouldn't be And requesting a supported offload when there are no mandatory ones should not be a problem: mandatory = 0x00 requested = 0x10 supported = 0x10 => invalid but it should be A naive translation of the above requirements results in the following expression: return (requested | supported) == supported && (requested & mandatory) == mandatory; What's your opinion? > +} > + > +/** > * DPDK callback to configure a Tx queue. > * > * @param dev > @@ -229,9 +276,22 @@ mlx4_tx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, > }; > int ret; > > - (void)conf; /* Thresholds configuration (ignored). */ > DEBUG("%p: configuring queue %u for %u descriptors", > (void *)dev, idx, desc); > + /* > + * Don't verify port offloads for application which > + * use the old API. > + */ > + if (!!(conf->txq_flags & ETH_TXQ_FLAGS_IGNORE) && Enclosing "!!(...)" seems unnecessary, only the fact the result is nonzero matters. > + !priv_is_tx_queue_offloads_allowed(priv, conf->offloads)) { > + rte_errno = ENOTSUP; > + ERROR("%p: Tx queue offloads 0x%lx don't match port " > + "offloads 0x%lx or supported offloads 0x%lx", "%lx" may cause a compilation error depending on the platform, you need to use "%" PRIx64 after including inttypes.h. > + (void *)dev, conf->offloads, > + dev->data->dev_conf.txmode.offloads, > + mlx4_priv_get_tx_port_offloads(priv)); > + return -rte_errno; > + } > if (idx >= dev->data->nb_tx_queues) { > rte_errno = EOVERFLOW; > ERROR("%p: queue index out of range (%u >= %u)", > @@ -281,8 +341,13 @@ mlx4_tx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, > RTE_MIN(MLX4_PMD_TX_PER_COMP_REQ, desc / 4), > .elts_comp_cd_init = > RTE_MIN(MLX4_PMD_TX_PER_COMP_REQ, desc / 4), > - .csum = priv->hw_csum, > - .csum_l2tun = priv->hw_csum_l2tun, > + .csum = priv->hw_csum && > + (conf->offloads & (DEV_TX_OFFLOAD_IPV4_CKSUM | > + DEV_TX_OFFLOAD_UDP_CKSUM | > + DEV_TX_OFFLOAD_TCP_CKSUM)), > + .csum_l2tun = priv->hw_csum_l2tun && > + (conf->offloads & > + DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM), > /* Enable Tx loopback for VF devices. */ > .lb = !!priv->vf, > .bounce_buf = bounce_buf, > -- > 2.12.0 > -- Adrien Mazarguil 6WIND