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 67AC0C6EE for ; Wed, 15 Jun 2016 16:22:12 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP; 15 Jun 2016 07:20:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,476,1459839600"; d="scan'208";a="976247988" Received: from irsmsx110.ger.corp.intel.com ([163.33.3.25]) by orsmga001.jf.intel.com with ESMTP; 15 Jun 2016 07:20:31 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.51]) by irsmsx110.ger.corp.intel.com ([163.33.3.25]) with mapi id 14.03.0248.002; Wed, 15 Jun 2016 15:20:28 +0100 From: "Ananyev, Konstantin" To: Ivan Boule , "Richardson, Bruce" CC: 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///6WgIAACpMAgAASJJA= Date: Wed, 15 Jun 2016 14:20:27 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836B7191F@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> In-Reply-To: <57616118.7080806@6wind.com> 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:22:13 -0000 > -----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 Rx/Tx= callback lists >=20 > 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 protect R= x/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 w= ould 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 t= he > >>> function "rte_eth_rx_burst()" while parsing the "post_rx_burst_cbs" l= ist > >>> 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->ne= xt" > >>> that you assume to be atomic operations, which is certainly true on m= ost > >>> CPUs. > >>> I see the 2 important following issues: > >>> > >>> 1) the "rte_eth_rxtx_callback" data structure associated with a remov= ed > >>> RX callback could still be accessed in the callback parsing loop of t= he > >>> function "rte_eth_rx_burst()" after having been freed in parallel. > >>> > >>> BUT SUCH A BAD SITUATION WILL NOT CURRENTLY HAPPEN, THANKS TO THE NIC= E > >>> MEMORY LEAK BUG in the function "rte_eth_remove_rx_callback()" that > >>> does not free the "rte_eth_rxtx_callback" data structure associated w= ith > >>> 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 arg= ue with > > the design, but it was done for a definite reason, so as to avoid payin= g the > > penalty of having locks. It pushes more responsibility onto the app, bu= t 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 t= he cost > > of having locks, even if better options for freeing the memory are avai= lable. > > > > /Bruce > > >=20 > -1 (not to say 0xFFFFFFFF) >=20 > 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 structures= . > This problem can easily be addressed by just changing the API as follows: >=20 > Change > void * > rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id, > rte_rx_callback_fn fn, void *user_param) >=20 > to > int > rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id, > struct rte_eth_rxtx_callback *cb) >=20 > In addition of solving the problem, this approach makes the API > consistent and let the application allocate "rte_eth_rxtx_callback" data > structures through any appropriate mean. That might make API a bit cleaner, but I don't see how it fixes the generic= problem: there is still no way to know by the upper layer when it is safe to free/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 As you said in previous mail:=20 > This is an example of a well-known more generic object deletion problem > which must arrange to guarantee that a deleted object is not used and > not accessible for use anymore before being actually deleted (freed, for > instance). Konstantin >=20 > Regards, > Ivan >=20 > -- > Ivan Boule > 6WIND Development Engineer