From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 6FAB21C85C for ; Mon, 14 May 2018 15:26:45 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 May 2018 06:26:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,400,1520924400"; d="scan'208";a="49088375" Received: from kmsmsx151.gar.corp.intel.com ([172.21.73.86]) by FMSMGA003.fm.intel.com with ESMTP; 14 May 2018 06:26:42 -0700 Received: from pgsmsx111.gar.corp.intel.com ([169.254.2.194]) by KMSMSX151.gar.corp.intel.com ([169.254.10.77]) with mapi id 14.03.0319.002; Mon, 14 May 2018 21:26:41 +0800 From: "Dai, Wei" To: Thomas Monjalon CC: "dev@dpdk.org" , "Yigit, Ferruh" , "Zhang, Qi Z" Thread-Topic: [dpdk-dev] [PATCH v13] ethdev: new Rx/Tx offloads API Thread-Index: AQHT633yLlRxHK4P8U+qmwYBG5FxNqQuqEyAgACKxiA= Date: Mon, 14 May 2018 13:26:40 +0000 Message-ID: <49759EB36A64CF4892C1AFEC9231E8D66CF81CC9@PGSMSX111.gar.corp.intel.com> References: <1525953415-14156-1-git-send-email-wei.dai@intel.com> <1526299235-54090-1-git-send-email-wei.dai@intel.com> <3929007.eCSRkoNXUr@xps> In-Reply-To: <3929007.eCSRkoNXUr@xps> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiY2E4ZmI1YTAtY2RlMC00MzYxLWE4NWMtZjc5NDQyZTc3ZTFmIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6Ilp1dURONUJRQlwvZXpPaHlYek9zb2d0bFErdE1VQW05WEdTcFdOUjcxTmxnPSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [172.30.20.205] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v13] ethdev: new Rx/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: Mon, 14 May 2018 13:26:46 -0000 Hi, Thomas Thanks for your quick feedback. > -----Original Message----- > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Monday, May 14, 2018 8:54 PM > To: Dai, Wei > Cc: dev@dpdk.org; Yigit, Ferruh ; Zhang, Qi Z > > Subject: Re: [dpdk-dev] [PATCH v13] ethdev: new Rx/Tx offloads API >=20 > 14/05/2018 14:00, Wei Dai: > > --- a/doc/guides/prog_guide/poll_mode_drv.rst > > +++ b/doc/guides/prog_guide/poll_mode_drv.rst > > @@ -303,12 +303,12 @@ In the DPDK offload API, offloads are divided > > into per-port and per-queue offloa > > * A pure per-port offloading can't be enabled on a queue and disabled = on > another queue at the same time. > > * A pure per-port offloading must be enabled or disabled on all queues= at > the same time. > > * Any offloading is per-queue or pure per-port type, but can't be both > types at same devices. > > -* A per-port offloading can be enabled or disabled on all queues at th= e > same time. > > -* It is certain that both per-queue and pure per-port offloading are > per-port type. > > +* Port capabilities =3D pre-queue capabilities + pure per-port capabil= ities. >=20 > s/pre/per/ Sorry for the typo error. >=20 > > +* Any supported offloading can be enabled on all queues. > > > > The different offloads capabilities can be queried using > ``rte_eth_dev_info_get()``. > > The ``dev_info->[rt]x_queue_offload_capa`` returned from > ``rte_eth_dev_info_get()`` includes all per-queue offloading capabilities= . > > -The ``dev_info->[rt]x_offload_capa`` returned from > ``rte_eth_dev_info_get()`` includes all per-port and per-queue offloading > capabilities. > > +The ``dev_info->[rt]x_offload_capa`` returned from > ``rte_eth_dev_info_get()`` includes all pure per-port and per-queue > offloading capabilities. >=20 > OK >=20 >=20 > > @@ -1556,29 +1558,29 @@ rte_eth_rx_queue_setup(uint16_t port_id, > uint16_t rx_queue_id, > > * The local_conf.offloads input to underlying PMD only carries > > * those offloadings which are only enabled on this queue and > > * not enabled on all queues. > > - * The underlying PMD must be aware of this point. > > */ >=20 > OK >=20 >=20 > > --- a/lib/librte_ethdev/rte_ethdev.h > > +++ b/lib/librte_ethdev/rte_ethdev.h > > @@ -1067,13 +1067,18 @@ struct rte_eth_dev_info { > > uint16_t max_vfs; /**< Maximum number of VFs. */ > > uint16_t max_vmdq_pools; /**< Maximum number of VMDq pools. */ > > uint64_t rx_offload_capa; > > - /**< All RX offload capabilities including all per queue ones */ > > + /**< > > + * All RX offload capabilities including all per-queue ones. > > + * Any flag in [rt]x_offload_capa and [rt]x_queue_offload_capa > > + * of this structure needn't be repeated in rte_eth_[rt]x_queue_setup= (). >=20 > It is confusing. Better to remove this sentence about queue_setup in port > capa comment. >=20 > > + * A flag enabled at port level can't be disabled at queue level. >=20 > This one too: it is a comment about port capa, not queue setup. >=20 Sorry, I think I have a mistake about your feedback on v12. Will remove above 2 sentences. >=20 > > @@ -1554,9 +1559,7 @@ const char * __rte_experimental > rte_eth_dev_tx_offload_name(uint64_t offload); > > * the [rt]x_offload_capa returned from > rte_eth_dev_infos_get(). > > * Any type of device supported offloading set in the input > argument > > * eth_conf->[rt]xmode.offloads to rte_eth_dev_configure() is > enabled > > - * on all [RT]x queues and it can't be disabled no matter wheth= er > > - * it is cleared or set in the input argument [rt]x_conf->offlo= ads > > - * to rte_eth_[rt]x_queue_setup(). > > + * on all queues and it can't be disabled in > rte_eth_[rt]x_queue_setup(). >=20 > OK >=20 >=20 > Missing: we must explain the "no repeat need" and "no disable port offloa= d > on queue" constraint. > In the last review, I was suggesting such sentences: > No need to repeat flags already enabled at port level. > A flag enabled at port level, cannot be disabled at queue level. > I think it should go in queue setup comments. >=20 > Opinion? >=20 Will add this in the comments of queue_setup( ) in rte_ethdev.h