From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id D7C5B7E18 for ; Fri, 26 Sep 2014 17:36:37 +0200 (CEST) Received: from azsmga001.ch.intel.com ([10.2.17.19]) by orsmga101.jf.intel.com with ESMTP; 26 Sep 2014 08:42:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,604,1406617200"; d="scan'208";a="480222573" Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by azsmga001.ch.intel.com with ESMTP; 26 Sep 2014 08:41:59 -0700 Received: from irsmsx105.ger.corp.intel.com (163.33.3.28) by IRSMSX102.ger.corp.intel.com (163.33.3.155) with Microsoft SMTP Server (TLS) id 14.3.195.1; Fri, 26 Sep 2014 16:41:58 +0100 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.248]) by IRSMSX105.ger.corp.intel.com ([169.254.7.57]) with mapi id 14.03.0195.001; Fri, 26 Sep 2014 16:41:58 +0100 From: "Ananyev, Konstantin" To: Neil Horman , "Wodkowski, PawelX" Thread-Topic: [dpdk-dev] [PATCH v2] Change alarm cancel function to thread-safe: Thread-Index: AQHP2MAqmY9s5RkexE+yRGxj2kXnOpwR4sKAgAAemtCAAAdaAIAAZ8zwgADMQACAAA5cAIAAEWsAgAAnzM6AAAITkA== Date: Fri, 26 Sep 2014 15:41:58 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772582137D88E@IRSMSX104.ger.corp.intel.com> References: <1411649768-8084-1-git-send-email-michalx.k.jastrzebski@intel.com> <20140925150807.GD32725@hmsreliant.think-freely.org> <2601191342CEEE43887BDE71AB977258213769DE@IRSMSX105.ger.corp.intel.com> <20140925172358.GG32725@hmsreliant.think-freely.org> <2601191342CEEE43887BDE71AB97725821378B50@IRSMSX104.ger.corp.intel.com> <20140926114630.GA3930@hmsreliant.think-freely.org> <20140926134014.GB3930@hmsreliant.think-freely.org> <20140926150156.GB5619@hmsreliant.think-freely.org> In-Reply-To: <20140926150156.GB5619@hmsreliant.think-freely.org> Accept-Language: en-IE, 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] Change alarm cancel function to thread-safe: 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: Fri, 26 Sep 2014 15:36:38 -0000 > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman > Sent: Friday, September 26, 2014 4:02 PM > To: Wodkowski, PawelX > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2] Change alarm cancel function to thread= -safe: >=20 > On Fri, Sep 26, 2014 at 02:01:05PM +0000, Wodkowski, PawelX wrote: > > > > > Maybe I don't see something obvious? :) > > > > > > I think you're missing the fact that your patch doesn't do what you a= ssert above > > > either :) > > > > Issue is not in setting alarms but canceling it. If you look closer to = my patch you > > see that it address this issue (look at added *do { lock(); ....; unloc= k(); } while( )* > > part). > > > I get where the issue is, and I'm looking at your patch. I see that you = did > some locking there. The issue I'm pointing out is that, if you call > rte_eal_alarm_cancel on an alarm callback, you will exit the alarm_cancel > function with, by definition, one alarm executing (the one you are curren= tly > running). You're patch works perfectly for the case where another thread= calls > cancel, in that it waits until the executing alarm is complete, but it do= esn't > work in the case where you are calling it from within the alarm callback. Hm, and why do we need it from alarm callback? After cb_func() is finished given alarm entry will be removed anyway. > If you're goal is to guarantee that all the matching alarms are cancelle= d and > complete, you haven't done that, because the recursive state is still unh= andled. >=20 > > > > > > First, lets address rte_alarm_set. There is no notion of "re-arming"= in this > > > alarm implementation, because theres no ability to refer to a specifi= c alarm > > > from the callers perspective. When you call rte_eal_alarm_set you ge= t a new > > > alarm every time. So I don't really see a race there. It might not = be exactly > > > the behavior you want, but its not a race, becuase you're not modifyi= ng an > > > alarm > > > in the middle of execution, you're just creating a new alarm, which i= s safe. > > > > OK, it is safe, but this is not the case. > > > I don't know what you mean by this. We agree its safe, great. But it is= the > case as I've described it, you can see it from the implementation, every = call to > rte_eal_alarm_set starts with a malloc of a new alarm structure. >=20 > > > > > > There is a race in what you describe above, insofar as its possible t= hat you > > > might call rte_eal_alarm_cancel and return without having canceled al= l the > > > matching alarms. I don't see any clear documentation on what the beh= avior is > > > supposed to be, but if you want to ensure that all matching alarms ar= e cancelled > > > or complete on return from rte_eal_alarm_cancel, thats perfectly fine= (in linux > > > API parlance, thats usually denoted as a cancel_sync operation). > > > > Again, look at the patch. I changed documentation to inform about this = behavior. > > >=20 > This is the documentation included in the patch: > Change alarm cancel function to thread-safe. > It eliminates a race between threads using rte_alarm_cancel and > rte_alarm_set. >=20 > neither have you compeltely described the race condition (though you now = have > previously in this thread), nor have you completely addressed it (calling > rte_eal_alarm_cancel and rte_eal_alarm_set still behaves exactly as it di= d > previously with a 2nd thread). >=20 > > > > > > For that race condition, you're correct, my patch doesn't address it,= I see that > > > now. Though your patch doesn't either. If you call rte_eal_alarm_ca= ncel from > > > within a callback function, then, by definition, you can't wait on th= e > > > completion of the active alarm, because thats a deadlock. Its a nece= ssecary > > > evil, I grant you, but it means that you can't be guaranteed the canc= elled and > > > complete (cancel_sync) behavior that you want, at least not with the = current > > > api. If you want that behavior, you need to do one of two things: > > > > This patch does not break any API. It only removes undefined behavior. > > > I never said it did break ABI. I said that to completely fix it you woul= d have > to break ABI. And it doesn't really remove undefined behavior, because y= ou > still have the old behavior in the recursive case (which you may be ok wi= th, I > don't know, but if you really want to address the behavior, you should ad= dress > this aspect of it). >=20 > > > > > > 1) Modify the api to allow callers to individually reference timer in= stances, so > > > that when cancelling, we can return an appropriate return code to ind= icate to > > > the caller that this alarm is in-progress. That way you can guarante= e the > > > caller that the specific alarm that you cancelled is either complete = and cancelled > > > or currently executing. Add an api to expicitly wait on a referenced= alarm as > > > well. This allows developers to know that, when executing an alarm c= allback, an > > > -ECURRENTLYEXECUTING return code is ok, because they are in the curre= ntly > > > executing context. > > > > This would brake API for sure. > Yes, it would. Bruce Richardson just made a major ABI break with his mbu= f > cleanup set. If there was a time to change ABI here, now would be the ti= me I > think. Ok, too many words for me, to be honest :) Can I summarise: As I remember the purpose of the patch was to fix the race condition inside= rte_alarm library. I believe that the patch provided by Michal & Pawel fixes the issues you di= scovered. If you think, that is not the case, could you please provide a list of rema= ining issues? Excluding ones that you just don't like it, and you are not happy with rte_= alarm API in total? =20 If you have any concerns about mbuf reorg/expansion - probably better to co= ntact Bruce and express them. Not to use it as an argument for breaking any existing API without really g= ood reason behind. =20 Konstantin