Re: [dpdk-dev] [PATCH v2] Change alarm cancel function to thread-safe:

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Sep 26, 2014 at 03:41:58PM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----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:
> > 
> > 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