From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 52330B3D7 for ; Thu, 25 Sep 2014 16:20:44 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP; 25 Sep 2014 07:26:59 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,597,1406617200"; d="scan'208";a="608504143" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga002.jf.intel.com with ESMTP; 25 Sep 2014 07:26:59 -0700 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.195.1; Thu, 25 Sep 2014 07:26:59 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx115.amr.corp.intel.com (10.18.116.19) with Microsoft SMTP Server (TLS) id 14.3.195.1; Thu, 25 Sep 2014 07:26:58 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.192]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.230]) with mapi id 14.03.0195.001; Thu, 25 Sep 2014 22:26:57 +0800 From: "Ouyang, Changchun" To: Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH v2] ethdev: Rename RX/TX enable queue field for queue start and stop Thread-Index: AQHP2KN2AoGjGVuw3Eymcd0tnJ8asZwR57Nw Date: Thu, 25 Sep 2014 14:26:56 +0000 Message-ID: References: <1406090907-24347-1-git-send-email-changchun.ouyang@intel.com> <1577833.5iZA1XeDyr@xps13> In-Reply-To: <1577833.5iZA1XeDyr@xps13> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2] ethdev: Rename RX/TX enable queue field for queue start and stop X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Sep 2014 14:20:44 -0000 Hi Thomas, Thanks very much for your comments! I will rework this patch and also update for i40e. Thanks and best regards, Changchun > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Thursday, September 25, 2014 5:31 PM > To: Ouyang, Changchun > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: Rename RX/TX enable queue > field for queue start and stop >=20 > Hi Ouyang, >=20 > 2014-07-23 12:48, Ouyang Changchun: > > Update comments for the field start_rx_per_q for better readability. > > Rename the field name to rx_enable_queue for better readability too. > > Accordingly Update its reference in sample vhost. >=20 > > - uint8_t start_rx_per_q; /**< start rx per queue. */ > > + /**< If rx_enable_queue is true, rte_eth_dev_rx_queue_start > should be > > + invocated to start RX for one queue after rte_eth_dev_start > is > > + invocated, and rte_eth_dev_rx_queue_start instead of > > + rte_eth_dev_start is responsible for allocating mbuf from > > + mempool and setup the DMA physical address. It is useful in > > + such scenario: buffer address is not available at the point of > > + rte_eth_dev_start's invocating but available later, e.g. in > > + VHOST zero copy case, the buffer address used to setup > DMA > > + address is available only after one VM(guest) startup. */ > > + uint8_t rx_enable_queue; > > }; >=20 > I have many comments here. >=20 > The doxygen comment /**< must be used only after the symbol you are > commenting: > http://doxygen.org/manual/docblocks.html#memberdoc >=20 > The comment is too long. > The use case would be in the manual, not in doxygen. > The comment about rte_eth_dev_rx_queue_start would be in the doxygen > comment of rte_eth_dev_rx_queue_start. >=20 > When this variable is set, it doesn't enable anything. It only disables t= he > queue when doing a global start. Its name should be rx_deferred_start. > All fields of this structure are about one queue, so the "queue" word is = not > needed. >=20 > > - uint8_t start_tx_per_q; /**< start tx per queue. */ > > + /**< If tx_enable_queue is true, rte_eth_dev_tx_queue_start must > be > > + invocated to start TX for one queue after rte_eth_dev_start > is > > + invocated. Refer to start_rx_per_q for the use case. */ > > + uint8_t tx_enable_queue; > > }; >=20 > You refer to the old name (start_rx_per_q). > By the way, a one line description (without referral) for both fields sho= uld be > enough. Something like "do not start with rte_eth_dev_start()". >=20 > > @@ -3652,13 +3652,13 @@ ixgbe_dev_rxtx_start(struct rte_eth_dev *dev) > > > > for (i =3D 0; i < dev->data->nb_tx_queues; i++) { > > txq =3D dev->data->tx_queues[i]; > > - if (!txq->start_tx_per_q) > > + if (!txq->tx_enable_queue) > > ixgbe_dev_tx_queue_start(dev, i); > > } >=20 > Here it's clear that this field is about disabling start. >=20 > Please rework this patch and update i40e accordingly. > Thanks > -- > Thomas