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 BDCBB5A89 for ; Wed, 23 Sep 2015 09:40:38 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga103.fm.intel.com with ESMTP; 23 Sep 2015 00:40:38 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,577,1437462000"; d="scan'208";a="811013579" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by fmsmga002.fm.intel.com with ESMTP; 23 Sep 2015 00:40:38 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.216]) by IRSMSX107.ger.corp.intel.com ([169.254.10.252]) with mapi id 14.03.0248.002; Wed, 23 Sep 2015 08:39:03 +0100 From: "De Lara Guarch, Pablo" To: "Qiu, Michael" , Stephen Hemminger Thread-Topic: [dpdk-dev] [PATCH v2] ethdev: add new RX/TX queue state arrays in rte_eth_dev_data Thread-Index: AQHQ8Mnb7P3LVk/tF0ah0CMFPPx6qJ5JvYDA Date: Wed, 23 Sep 2015 07:39:02 +0000 Message-ID: References: <1442440284-6175-1-git-send-email-pablo.de.lara.guarch@intel.com> <20150921154024.3890eb76@urahara> <533710CFB86FA344BFBF2D6802E602861989289E@SHSMSX101.ccr.corp.intel.com> In-Reply-To: <533710CFB86FA344BFBF2D6802E602861989289E@SHSMSX101.ccr.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] 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: add new RX/TX queue state arrays in rte_eth_dev_data 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: Wed, 23 Sep 2015 07:40:39 -0000 > -----Original Message----- > From: Qiu, Michael > Sent: Wednesday, September 23, 2015 7:57 AM > To: Stephen Hemminger; De Lara Guarch, Pablo > Cc: dev@dpdk.org; thomas.monjalon@6wind.com > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: add new RX/TX queue state > arrays in rte_eth_dev_data >=20 > On 2015/9/22 6:40, Stephen Hemminger wrote: > > On Wed, 16 Sep 2015 22:51:24 +0100 > > Pablo de Lara wrote: > > > >> This is important to avoid trying to start/stop twice a queue, > >> which will result in undefined behaviour > >> (which may cause RX/TX disruption). > >> > >> Mind that only the PMDs which have queue_start/stop functions > >> have been changed to update this field, as the functions will > >> check the queue state before switching it. > >> > >> Signed-off-by: Pablo de Lara > > I agree that the DPDK API should check for buggy manipulation > > in the control path. But this should be done in generic code. > > Anything where you have to change any driver is making more work > > than necessary. >=20 I see little way to set the queue state without touching the PMDs. Basically, because queues can be started either calling directly the function from the PMDs or calling the generic function from ethdev, therefore some code must be placed in the PMDs. I guess there are two possible ways to overcome this: - Use in all PMDs rte_eth_rx_queue_start/stop, instead of directly=20 the internal functions: first, it is necessary the port id, which I am no= t sure if it is always available (plus, when it is, it is available in the specific queue of that PMD, so i= t does not have to be there). Anyway, this option requires changing the PMDs. - Apparently, the only function that calls these queue_start/stop functions= is dev_start/stop. In some PMDs, it is possible to defer the start of some queues, so it wo= uld not initialize these queues, but in other PMDs this is not implemented. We will also be depending at how it is going on within the PMDs, so the code will not be generic either. Any ideas to make this truly generic? > I agree with you, but I have a question, why we need expose the queue > start and stop function to app? >=20 > In my opinion, user app will hardly to start a device but stop the > device queue. what's the purpose of it? This API was added in 1.7 if I am not wrong. The purpose was that user coul= d decide not to start some of the queues in a device at start up, and then a new API= to start=20 these queues was necessary, but there were no checks of their current state= , leading to undefined behaviour. Thanks both Stephen and Michael, Pablo >=20 > Thanks, > Michael