From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 36F579ACC for ; Thu, 9 Jun 2016 09:51:05 +0200 (CEST) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1bAumH-0001wD-9v; Thu, 09 Jun 2016 09:53:22 +0200 To: "Lu, Wenzhuo" , Stephen Hemminger References: <1465191653-28408-1-git-send-email-wenzhuo.lu@intel.com> <1465191653-28408-3-git-send-email-wenzhuo.lu@intel.com> <20160607191553.10993efe@xeon-e3> <6A0DE07E22DDAD4C9103DF62FEBC090903483A7F@shsmsx102.ccr.corp.intel.com> Cc: "dev@dpdk.org" , "Tao, Zhe" From: Olivier Matz Message-ID: <57591FE1.9070908@6wind.com> Date: Thu, 9 Jun 2016 09:50:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.6.0 MIME-Version: 1.0 In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC090903483A7F@shsmsx102.ccr.corp.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 2/8] lib/librte_ether: defind RX/TX lock mode 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, 09 Jun 2016 07:51:05 -0000 Hi, On 06/08/2016 09:34 AM, Lu, Wenzhuo wrote: > Hi Stephen, > > >> -----Original Message----- >> From: Stephen Hemminger [mailto:stephen@networkplumber.org] >> Sent: Wednesday, June 8, 2016 10:16 AM >> To: Lu, Wenzhuo >> Cc: dev@dpdk.org; Tao, Zhe >> Subject: Re: [dpdk-dev] [PATCH 2/8] lib/librte_ether: defind RX/TX lock mode >> >> On Mon, 6 Jun 2016 13:40:47 +0800 >> Wenzhuo Lu wrote: >> >>> Define lock mode for RX/TX queue. Because when resetting the device we >>> want the resetting thread to get the lock of the RX/TX queue to make >>> sure the RX/TX is stopped. >>> >>> Using next ABI macro for this ABI change as it has too much impact. 7 >>> APIs and 1 global variable are impacted. >>> >>> Signed-off-by: Wenzhuo Lu >>> Signed-off-by: Zhe Tao >> >> Why does this patch set make a different assumption the rest of the DPDK? >> >> The rest of the DPDK operates on the principle that the application is smart >> enough to stop the device before making changes. There is no equivalent to the >> Linux kernel RTNL mutex. The API assumes application threads are well behaved >> and will not try and sabotage each other. >> >> If you restrict the reset operation to only being available when RX/TX is stopped, >> then no lock is needed. >> >> The fact that it requires lots more locking inside each device driver implies to me >> this is not correct way to architect this. +1 I'm not sure adding locks is the proper way to do. This is the application responsibility to ensure that: - control functions are not called concurrently on the same port - rx/tx functions are not called when the device is stopped/reset/... However, I do think the usage paradigms of the ethdev api should be better documented in rte_ethdev.h (ex: which functions can be called concurrently). This would be a first step. If we really want a helper API to do that in DPDK, the _next_ step could be to add them in the ethdev api to achieve this. Maybe something like (the function names could be better): - to be called on one control thread: rte_eth_stop_rxtx(port) rte_eth_start_rxtx(port) rte_eth_get_rxtx_state(port) -> return "running" if at least one core is inside the rx/tx code -> return "stopped" if all cores are outside the rx/tx code - to be called on dataplane cores: /* same than rte_eth_rx_burst(), but checks if rx/tx is allowed * first, else do nothing */ rte_eth_rx_burst_interruptible() rte_eth_tx_burst_interruptible() The code of control thread could be: rte_eth_stop_rxtx(port); /* wait that all dataplane cores finished their processing */ while (rte_eth_get_rxtx_state(port) != stopped) ; rte_eth_some_control_operation(port); rte_eth_start_rxtx(port); I think this could be done without any lock, just with the proper memory barriers and a per-core status. But this API may impose a paradigm to the application, and I'm not sure the DPDK should do that. Regards, Olivier