From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id C4B7EC448 for ; Wed, 15 Jun 2016 15:29:57 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP; 15 Jun 2016 06:29:33 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,476,1459839600"; d="scan'208";a="987987712" Received: from fjafari-mobl2.ger.corp.intel.com ([10.252.16.213]) by fmsmga001.fm.intel.com with SMTP; 15 Jun 2016 06:29:30 -0700 Received: by (sSMTP sendmail emulation); Wed, 15 Jun 2016 14:29:29 +0025 Date: Wed, 15 Jun 2016 14:29:29 +0100 From: Bruce Richardson To: "Ananyev, Konstantin" Cc: Ivan Boule , Thomas Monjalon , "Pattan, Reshma" , "dev@dpdk.org" Message-ID: <20160615132928.GA11536@bricha3-MOBL3> References: <1465575534-23605-1-git-send-email-reshma.pattan@intel.com> <10886152.VH5xYhdqG2@xps13> <2601191342CEEE43887BDE71AB97725836B714ED@irsmsx105.ger.corp.intel.com> <2907169.iIEIeOfXh7@xps13> <576146CD.8060008@6wind.com> <2601191342CEEE43887BDE71AB97725836B717B0@irsmsx105.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB97725836B717B0@irsmsx105.ger.corp.intel.com> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v9 1/8] ethdev: use locks to protect Rx/Tx callback lists 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, 15 Jun 2016 13:29:58 -0000 On Wed, Jun 15, 2016 at 12:40:16PM +0000, Ananyev, Konstantin wrote: > Hi Ivan, > > > -----Original Message----- > > From: Ivan Boule [mailto:ivan.boule@6wind.com] > > Sent: Wednesday, June 15, 2016 1:15 PM > > To: Thomas Monjalon; Ananyev, Konstantin > > Cc: Pattan, Reshma; dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v9 1/8] ethdev: use locks to protect Rx/Tx callback lists > > > > On 06/15/2016 10:48 AM, Thomas Monjalon wrote: > > > > >> > > >>> I think the read access would need locking but we do not want it > > >>> in fast path. > > >> > > >> I don't think it would be needed. > > >> As I said - read/write interaction didn't change from what we have right now. > > >> But if you have some particular scenario in mind that you believe would cause > > >> a race condition - please speak up. > > > > > > If we add/remove a callback during a burst? Is it possible that the next > > > pointer would have a wrong value leading to a crash? > > > Maybe we need a comment to state that we should not alter burst > > > callbacks while running burst functions. > > > > > > > Hi Reshma, > > > > You claim that the "rte_eth_rx_cb_lock" does not need to be hold in the > > function "rte_eth_rx_burst()" while parsing the "post_rx_burst_cbs" list > > of RX callbacks associated with the polled RX queue to safely remove RX > > callback(s) in parallel. > > The problem is not [only] with the setting and the loading of "cb->next" > > that you assume to be atomic operations, which is certainly true on most > > CPUs. > > I see the 2 important following issues: > > > > 1) the "rte_eth_rxtx_callback" data structure associated with a removed > > RX callback could still be accessed in the callback parsing loop of the > > function "rte_eth_rx_burst()" after having been freed in parallel. > > > > BUT SUCH A BAD SITUATION WILL NOT CURRENTLY HAPPEN, THANKS TO THE NICE > > MEMORY LEAK BUG in the function "rte_eth_remove_rx_callback()" that > > does not free the "rte_eth_rxtx_callback" data structure associated with > > the removed callback ! > > Yes, though it is documented behaviour, someone can probably > refer it as a feature, not a bug ;) > +1 This is definitely not a bug, this is absolutely by design. One may argue with the design, but it was done for a definite reason, so as to avoid paying the penalty of having locks. It pushes more responsibility onto the app, but it does allow the app to choose the best solution for managing the freeing of memory for its situation. The alternative is to force all apps to pay the cost of having locks, even if better options for freeing the memory are available. /Bruce