From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com [66.111.4.26]) by dpdk.org (Postfix) with ESMTP id BD5706CC9 for ; Thu, 26 Apr 2018 10:18:40 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 28C13211D7; Thu, 26 Apr 2018 04:18:40 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Thu, 26 Apr 2018 04:18:40 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc; s=mesmtp; bh=STZH7LCw8xvit+9K6gHdC7Irxz 6YUiM2aJqXqSa/W3Q=; b=eUK4pT16qKM2h0W67JhiP/hOG7l+a41D1q1olC601m rUxnh4mEZglY6Qnnzq4A34YIfteeuWF8+S5mFIdgm0YEaklVF4fhbqdMWtkBgMlZ zyNNg5GZElFhlMDyNAKQcK5solbNJMWsm7Fe5xnmrWXSFJVTFBbYb+guZf/xHofH A= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; bh=STZH7L Cw8xvit+9K6gHdC7Irxz6YUiM2aJqXqSa/W3Q=; b=FnOkq0kRf7cI6S4IKpJNbS DAfTosaksSOEsFDWHL4Ns5QDNj45B5EIWuKgJkDXbZ+H0OwqFVNNretfkzjooTKY HIcY3NMcjh0uyYLgAuWp9fyK/8YUWD8AFTrLquOKagBx7BRRJgXlhG/lhkDz0X0N GIxymOyKqN64HBFA6Xg7sV4MS1bUc94WaZe7k0B95JPA7AMc0uQ+wx+/VAWEKTg7 RMfUD2luyVuGfFMcZMdcCdipTkKO8J9s/ph6rzK1dikZVVTUe5WOHH2aM66+qtAY 7G2vfCiCY/slOVeGIFvs0JK8iCfCJ6qshigX+NNOcTDgM160nZx48vPmbjTQAzNQ == X-ME-Sender: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 61215E4412; Thu, 26 Apr 2018 04:18:39 -0400 (EDT) From: Thomas Monjalon To: "Zhang, Qi Z" Cc: "Yigit, Ferruh" , "Dai, Wei" , "dev@dpdk.org" Date: Thu, 26 Apr 2018 10:18:38 +0200 Message-ID: <169440796.JqTBcgK5P9@xps> In-Reply-To: <039ED4275CED7440929022BC67E70611531A8BAE@SHSMSX103.ccr.corp.intel.com> References: <20180328085709.28310-1-wei.dai@intel.com> <4bdecf05-3a7b-1e5d-8a3b-e71e0c37a74d@intel.com> <039ED4275CED7440929022BC67E70611531A8BAE@SHSMSX103.ccr.corp.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v4] ethdev: check Rx/Tx offloads 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: Thu, 26 Apr 2018 08:18:41 -0000 26/04/2018 09:59, Zhang, Qi Z: > > > -----Original Message----- > > From: Yigit, Ferruh > > Sent: Thursday, April 26, 2018 1:05 AM > > To: Dai, Wei ; thomas@monjalon.net; Zhang, Qi Z > > > > Cc: dev@dpdk.org > > Subject: Re: [PATCH v4] ethdev: check Rx/Tx offloads > > > > 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 > > > > > > --- > > > 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? > > Should we do like below > local_conf.offloads |= dev->data->dev_conf.rxmode.offloads; > local_conf.offloads &= dev_info.rx_queue_offload_capa > > I thinks it's better to only strip port offloads. But keep all queue offload, > since this is exact we going to configure the queue and during device start, it can simply iterate on each bit on local_conf.offloads to > turn on queue offload and don't need to worry about rxmode.offloads. No The offloads which are already enabled at port level does not need to be enabled again at queue level. But the PMD can decide to not configure the offload at port level for real, and configure the port offloads in every queue setups. It is an implementation choice, and can be different per-offload. So it is simpler to filter such request for queue setups. This way, we will be sure that all offloads, requested in queue setup PMD function, must be setup for the queue. The PMD implementation will need to setup all the requested offloads in queue setup, plus the port offloads which were deferred to all queues. Hope it's clear.