[dpdk-dev] [PATCH v2] Change alarm cancel function to thread-safe:
Bruce Richardson
bruce.richardson at intel.com
Mon Sep 29 12:33:15 CEST 2014
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
More information about the dev
mailing list