On Fri, Sep 26, 2014 at 03:41:58PM +0000, Ananyev, Konstantin wrote: > > > > -----Original Message----- > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Neil Horman > > Sent: Friday, September 26, 2014 4:02 PM > > To: Wodkowski, PawelX > > Cc: dev at dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v2] Change alarm cancel function to > > thread-safe: > > > > 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 > > > > assert 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(); ....; > > > unlock(); } 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 currently > > 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 > > doesn'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? Because you might not know if you're in an alarm callback or not. Pawel explained that the point of the patch was to ensure that alarms are canceled and complete when you call rte_eal_alarm_cancel, and thats not always going to be the case, even whith this patch.
> After cb_func() is finished given alarm entry will be removed anyway. > Yes, but thats true with or without this patch. > > If you're goal is to guarantee that all the matching alarms are cancelled > > and > > complete, you haven't done that, because the recursive state is still > > unhandled. > > > > > > > > > > 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 specific > > > > alarm > > > > from the callers perspective. When you call rte_eal_alarm_set you get > > > > 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 modifying > > > > an > > > > alarm > > > > in the middle of execution, you're just creating a new alarm, which is > > > > 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. > > > > > > > > > > There is a race in what you describe above, insofar as its possible > > > > that you > > > > might call rte_eal_alarm_cancel and return without having canceled all > > > > the > > > > matching alarms. I don't see any clear documentation on what the > > > > behavior is > > > > supposed to be, but if you want to ensure that all matching alarms are > > > > 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. > > > > > > > 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. > > > > 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 did > > previously with a 2nd thread). > > > > > > > > > > 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_cancel from > > > > within a callback function, then, by definition, you can't wait on the > > > > completion of the active alarm, because thats a deadlock. Its a > > > > necessecary > > > > evil, I grant you, but it means that you can't be guaranteed the > > > > cancelled 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 would > > have > > to break ABI. And it doesn't really remove undefined behavior, because you > > still have the old behavior in the recursive case (which you may be ok > > with, I > > don't know, but if you really want to address the behavior, you should > > address > > this aspect of it). > > > > > > > > > > 1) Modify the api to allow callers to individually reference timer > > > > instances, so > > > > that when cancelling, we can return an appropriate return code to > > > > indicate to > > > > the caller that this alarm is in-progress. That way you can guarantee > > > > 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 > > > > callback, an > > > > -ECURRENTLYEXECUTING return code is ok, because they are in the > > > > currently > > > > executing context. > > > > > > This would brake API for sure. > > Yes, it would. Bruce Richardson just made a major ABI break with his mbuf > > cleanup set. If there was a time to change ABI here, now would be the time > > I > > think. > > Ok, too many words for me, to be honest :) Yeah, its getting a bit verbose :) > 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 > discovered. > If you think, that is not the case, could you please provide a list of > remaining issues? > Excluding ones that you just don't like it, and you are not happy with > rte_alarm API in total? Gladly. As Pawel explained the race, its possible that, after calling rte_eal_alarm_cancel, an in-flight execution of an alarm callback may still be running. The problem with that ostensibly is that data which is being accessed by the callback might be then accessed in parallel with another process leading to data corruption or some other problem. The issue I have with his patch is that it doesn't completely close the race. While it does close the race for the condition in whcih thread B is running the alarm callback while thread A is executing the cancel operation, it does not close the case for when a single thread B is running the cancel operation, as the in-flight execution itself is still active. If such a cancellation occurs via an intermediary function (i.e. one which is not aware that it is explicitly running an alarm callback, which signals another thread to execute via some other method (ipc communication, etc), the same data corruption may occur, because the canceled and complete guarantee has been violated. > > If you have any concerns about mbuf reorg/expansion - probably better to > contact Bruce and express them. > Not to use it as an argument for breaking any existing API without really > good reason behind. > No, no concerns at all, only pointing out that we've already broken ABI in this release, which requires application writers to rebuild and adjust their applications, so if we were going to adjust this api, now would be the time, rather than in a futre release, requiring multiple application rebuilds. Neil
