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

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Sep 29, 2014 at 10:11:38AM +0000, Wodkowski, PawelX wrote:
> > >
> > > Image how you will be damned by someone that not even notice you change
> > > and he Is managing some kind of resource based on returned number of
> > > set/canceled timers. If you suddenly start returning negative values how those
> > > application will behave? Silently changing returned value domain is evil in its
> > > pure form.
> > 
> > As I can see the impact is very limited.
> 
> It is small impact to DPDK but can be huge to user application:

This is why we traditionally have in the release-notes for each release a 
section dedicated to calling out changes from one release to another. [See 
http://dpdk.org/doc/intel/dpdk-release-notes-1.7.0.pdf section 5]. Since 
from release-to-release there are generally only a couple of changes - 
though our next release may be a little different - the actual changes are 
clear enough to read about without wading through pages of documentation. I 
thinking calling out the change in both the release notes and the API docs 
is sufficient even for a change like this.  

Basically, I wouldn't let API stability factor in too much in trying to get 
a proper fix for this issue.

/Bruce


> Ex:
> If someone use this kind of expression in callback (skipping user app serialization part):
> callback () {
> ...
> some_simple_semaphore += rte_alarm_cancel(...));
> ...
> }
> 
> Anywhere in the code:
> ...
> If (some_simple_semapore) {
> 	some_simple_semapore --;
> 	if (rte_eal_alarm_set(...) != 0)
> 		some_simple_semapore ++;
> }
> ...
> 
> 1. Do you notice the change in cancel function?
> 2. How many hours you spend to find this issue in case of big app/system?
> 
> > Only code that does check for (rte_alarm_cancel(...) == 0/ != 0) inside alarm
> > callback function might be affected.
> > From other side, indeed, there could exist situations, when the caller needs to
> > know
> > was the alarm successfully cancelled or not.
> > And if not by what reason.
> > 
> 
> I can extend API of rte alarms to add alarm state checking in next patch,  but for 
> now, since this is not urgent I think original patch  v2 should be enough.
> 
> Pawel