From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id EC6EDC6EE for ; Wed, 15 Jun 2016 16:28:55 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP; 15 Jun 2016 07:27:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,476,1459839600"; d="scan'208";a="828598710" Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31]) by orsmga003.jf.intel.com with ESMTP; 15 Jun 2016 07:27:16 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.51]) by IRSMSX106.ger.corp.intel.com ([169.254.8.145]) with mapi id 14.03.0248.002; Wed, 15 Jun 2016 15:27:16 +0100 From: "Ananyev, Konstantin" To: "Richardson, Bruce" CC: Ivan Boule , Thomas Monjalon , "Pattan, Reshma" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v9 1/8] ethdev: use locks to protect Rx/Tx callback lists Thread-Index: AQHRxsb8c+QpcxnBPE2qkSBe1aWjuJ/qHtqAgAAUOJD///QYAIAAOaKAgAAWL0D///6WgIAACpMAgAASJJD///IRgAACHVig Date: Wed, 15 Jun 2016 14:27:15 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836B7194A@irsmsx105.ger.corp.intel.com> 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> <20160615132928.GA11536@bricha3-MOBL3> <57616118.7080806@6wind.com> <2601191342CEEE43887BDE71AB97725836B7191F@irsmsx105.ger.corp.intel.com> <20160615142223.GC11536@bricha3-MOBL3> In-Reply-To: <20160615142223.GC11536@bricha3-MOBL3> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 14:28:56 -0000 > -----Original Message----- > From: Richardson, Bruce > Sent: Wednesday, June 15, 2016 3:22 PM > To: Ananyev, Konstantin > Cc: Ivan Boule; Thomas Monjalon; Pattan, Reshma; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v9 1/8] ethdev: use locks to protect Rx/Tx= callback lists >=20 > On Wed, Jun 15, 2016 at 03:20:27PM +0100, Ananyev, Konstantin wrote: > > > > > > > -----Original Message----- > > > From: Ivan Boule [mailto:ivan.boule@6wind.com] > > > Sent: Wednesday, June 15, 2016 3:07 PM > > > To: Richardson, Bruce; Ananyev, Konstantin > > > Cc: Thomas Monjalon; Pattan, Reshma; dev@dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH v9 1/8] ethdev: use locks to protect R= x/Tx callback lists > > > > > > On 06/15/2016 03:29 PM, Bruce Richardson wrote: > > > > 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 prote= ct 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 h= ave right now. > > > >>>>> But if you have some particular scenario in mind that you belie= ve 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_cb= s" list > > > >>> of RX callbacks associated with the polled RX queue to safely rem= ove 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 r= emoved > > > >>> 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()" t= hat > > > >>> does not free the "rte_eth_rxtx_callback" data structure associat= ed 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 p= aying 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 fre= eing of > > > > memory for its situation. The alternative is to force all apps to p= ay the cost > > > > of having locks, even if better options for freeing the memory are = available. > > > > > > > > /Bruce > > > > > > > > > > -1 (not to say 0xFFFFFFFF) > > > > > > This is definitely an API design bug ! > > > I would say that if you don't know how to free a resource that you > > > allocate, it is very likely that you are wrong allocating it. > > > And this is exactly what happens here with RX/TX callback data struct= ures. > > > This problem can easily be addressed by just changing the API as foll= ows: > > > > > > Change > > > void * > > > rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id, > > > rte_rx_callback_fn fn, void *user_param) > > > > > > to > > > int > > > rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id, > > > struct rte_eth_rxtx_callback *cb) > > > > > > In addition of solving the problem, this approach makes the API > > > consistent and let the application allocate "rte_eth_rxtx_callback" d= ata > > > structures through any appropriate mean. > > > > That might make API a bit cleaner, but I don't see how it fixes the gen= eric problem: > > there is still no way to know by the upper layer when it is safe to fre= e/re-use > > removed callback, but to make sure that all IO on that queue is stopped > > (I.E. some external synchronisation around the queue). >=20 > Actually, it allows other, more creative solutions, like an app having a > statically allocated set/pool of callback structures that it doesn't need= to > allocate or free at all. I understand that, and as I said I am not against that change. But it doesn't solve the problem in general. Ok, if you have a static object, you wouldn't need to call free() for it, but you still want to re-use it after remove_cb() has finished, right? So there is no actual difference - the main question is: at what point after remove_cb() it is safe to modify it's contents. Konstantin >=20 > /Bruce