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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----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?
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 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 :)
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?      

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.  

Konstantin