Implements more EBUSY on Resource.request() (TEP108) #255

mcerveny · 2014-02-25T19:25:45Z

There is problem with multiple calls to SimpleArbiterP.nc Resource.request().

Problem description - call 3x Resource.request() in one task:

resource is free (state == RES_IDLE )
1. call Resource.request() returns SUCCESS (state == RES_GRANTING) and post is planned
1. call Resource.request() returns SUCCESS !!! (state == RES_GRANTING) and id is queued (Queue.enqueue(id))
1. call Resource.request() returns EBUSY (state == RES_GRANTING) and id is already queued (see ResourceQueue).

TEP108: 3.1 "... If a client ever makes multiple requests before receiving a granted event, an EBUSY value is returned, and the request is not queued. ..."

I suppose that correct return code for 3) is EBUSY too and NOT queue request.
I have problem with that, Resource.granted is called 2 times and if I do everything with first call,
there is no work for the second call.

There are similar problems with ArbiterP.nc (that return SUCCESS instead EBUSY), FcfsPriorityArbiterC.nc.

alignan · 2014-02-25T20:12:59Z

👍

andrasbiro · 2014-02-27T19:56:18Z

+1
However, it seems a bit risky to merge that, it might break stuff.

cire831 · 2014-03-01T07:14:33Z

+1

not sure what you mean by risky to merge it.

it is broken. if existing code takes advantage of this misbehaviour then
it should also be fixed.

On Thu, Feb 27, 2014 at 11:56 AM, András Bíró [email protected]:

+1
However, it seems a bit risky to merge that, it might break stuff.

Reply to this email directly or view it on GitHubhttps://github.com//pull/255#issuecomment-36283980
.

Eric B. Decker
Senior (over 50 :-) Researcher

andrasbiro · 2014-03-03T16:30:50Z

Because there's a lot of lib/driver with no support. Most of the supported lib/driver is supported by one person, if she/he doesn't have time right now, a possible bug won't be fixed for a while.
I think this should be merged, I just don't know how. Probably we should merge it, and see what happens. If something bad, we can still undo it, and reopen this bug.

cire831 · 2014-03-06T00:35:24Z

seems like a reasonable plan to me.

+1

anyone else any thoughts?

If not I'll merge this in at the end of this week.

On Mon, Mar 3, 2014 at 8:30 AM, András Bíró [email protected]:

Because there's a lot of lib/driver with no support. Most of the supported
lib/driver is supported by one person, if she/he doesn't have time right
now, a possible bug won't be fixed for a while.
I think this should be merged, I just don't know how. Probably we should
merge it, and see what happens. If something bad, we can still undo it, and
reopen this bug.

Reply to this email directly or view it on GitHubhttps://github.com//pull/255#issuecomment-36527980
.

Eric B. Decker
Senior (over 50 :-) Researcher

phil-levis · 2014-04-18T07:56:24Z

Was this merged in?

cire831 · 2014-04-18T15:19:10Z

not merged. there was some concern (Biro) that it might break things.

I asked if anyone else had any comments but got nothing back.

given the possibility of breakage, I errored on the side of caution. I
would still like a couple of more eyes thinking about this before we merge
it.

But yes we should merge this.

On Fri, Apr 18, 2014 at 1:56 AM, phil-levis [email protected]:

Was this merged in?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/255?utm_campaign=website&utm_source=sendgrid.com&utm_medium=email#issuecomment-40793510
.

Eric B. Decker
Senior (over 50 :-) Researcher

andrasbiro · 2014-04-18T17:39:03Z

I also said "Probably we should merge it, and see what happens. If something bad, we can still undo it, and reopen this bug."
So, I'm fine with it.

mcerveny · 2014-04-18T18:51:02Z

Hello.

I have some question to coauthor Philip Levis:

My patch allows "queue" request if it is called during "state!= RES_GRANTING" (for example from Resource.granted() task). I think that is correct reaction.
But:
https://github.com/tinyos/tinyos-main/blob/master/tos/system/ArbiterP.nc#L84
-> it returns SUCCESS (not EBUSY) in EVERY state and the request is not queued.
https://github.com/tinyos/tinyos-main/blob/master/tos/system/FcfsPriorityArbiterC.nc#L169
-> it has the same logic as SimpleArbiterP.nc and it can be probably patched the same way.

Can coauthor Philip Levis (or someone else) propose some solution/patch for ArbiterP.nc and FcfsPriorityArbiterC.nc ?

Thanks, Martin Cerveny

phil-levis · 2014-04-20T01:44:15Z

It doesn't always return SUCCESS; it could return non-SUCCESS if the request is not enqueued properly.

If I recall correctly, we moved to returning SUCCESS on requests rather than EBUSY because it makes managing state machines in callers easier. Basically, SUCCESS means "you will receive a granted event." 108 doesn't say EBUSY is a MUST.

I think that logically your fix looks good. But before I pull it, I need you to you could use formatting in line with most TinyOS code (e.g., have braces for the else clause, etc.). Thanks!

mcerveny · 2014-04-20T10:25:32Z

Hello.

On Sat, 19 Apr 2014, phil-levis wrote:

It doesn't always return SUCCESS; it could return non-SUCCESS if the request is not enqueued properly.

More precisely
https://github.com/tinyos/tinyos-main/blob/master/tos/system/ArbiterP.nc#L84
(used from FcfsArbiterC.nc/RoundRobinArbiterC.nc):
return SUCCESS always for the caller with the same id (reqResId == id).

Example:

TestResourceC.nc:

configuration TestResourceC {
}
implementation {
components TestResourceM;

components MainC, new FcfsArbiterC("Test.Resource");

TestResourceM->MainC.Boot;
TestResourceM->FcfsArbiterC.Resource[unique("Test.Resource")];

components SerialPrintfC;

}

TestResourceM.nc:

#define NEW_PRINTF_SEMANTICS 1
#include "printf.h"

module TestResourceM {
uses interface Boot;
uses interface Resource;
}

implementation {
event void Boot.booted() {
printf("1. boot %d\n", call Resource.request());
printf("2. boot %d\n", call Resource.request());
printf("3. boot %d\n", call Resource.request());
printf("4. boot %d\n", call Resource.request());
}

event void Resource.granted() {
    printf("1. granted %d\n", call Resource.request());
    printf("2. granted %d\n", call Resource.request());
    printf("3. granted %d\n", call Resource.request());
    printf("4. granted %d\n", call Resource.request());
    call Resource.release();
}

}

Output:

boot 0
boot 0
boot 0
boot 0
granted 0
granted 0
granted 0
granted 0

Program is stopped and resource is not planned anymore again!

If FcfsArbiterC is replaced by SimpleFcfsArbiterC (based on patched
SimpleArbiterP.nc). Resource is planned again!

Output:

boot 0
boot 5
boot 5
boot 5
granted 0
granted 5
granted 5
granted 5
granted 0
granted 5
granted 5
granted 5
granted 0
granted 5
granted 5
granted 5
granted 0
granted 5
...

If FcfsArbiterC is replaced by SimpleFcfsArbiterC (based on ORIGINAL
unpatched SimpleArbiterP.nc). Resource is planned again (in boot, than in
second granted event) !

boot 0
boot 0
boot 5
boot 5
granted 5
granted 5
granted 5
granted 5
granted 0
granted 5
granted 5
granted 5
granted 0
granted 5
granted 5
granted 5
granted 0
granted 5
granted 5
...

If I recall correctly, we moved to returning SUCCESS on requests rather than EBUSY because it makes managing state machines in callers easier. Basically,
SUCCESS means "you will receive a granted event." 108 doesn't say EBUSY is a MUST.

The caller can simply ignore return code (EBUSY says - already planned).
( also see https://github.com/tinyos/tinyos-main/blob/master/tos/interfaces/Resource.nc#L78 )

I think that logically your fix looks good. But before I pull it, I need you to you could use formatting in line with most TinyOS code (e.g., have braces for
the else clause, etc.). Thanks!

???
The same coding standard is used few lines later:
https://github.com/tinyos/tinyos-main/blob/master/tos/system/SimpleArbiterP.nc#L144

M.C>

phil-levis · 2014-04-20T16:39:36Z

OK, it sounds like the arbiter implementations are completely inconsistent. What's important is to give them the same calling and event behavior. I'd argue that the correct behavior is the middle one, which is from your patch to SimplerArbiterP. The other Arbiters should have this behavior as well. Do you agree?

mcerveny · 2014-04-20T16:48:25Z

On Sun, 20 Apr 2014, phil-levis wrote:

OK, it sounds like the arbiter implementations are completely inconsistent. What's important is to give them the same calling and event behavior. I'd argue
that the correct behavior is the middle one, which is from your patch to SimplerArbiterP. The other Arbiters should have this behavior as well. Do you agree?

Yes.

M.C>

phil-levis · 2014-04-20T17:56:44Z

OK. The one challenging bit here is a lot of moving parts depend on the arbiter interfaces. I'm pegged until May 1, but in May can start spending some time on this. I think moving all of the Arbiters to having a uniform call/response semantics and verifying their use in other components would be a great step forward. The reason this second part is tricky is the integration with power management. Coming up with test cases for the calling semantics is not hard (e.g., yours above) but locks on buses etc. are used in many places.

phil-levis · 2014-07-01T16:13:01Z

Alright, finally emerging from the post-spring-quarter backlog. Let's clean up the arbiters. This is going to be a tricky job, because so many components depend on them. Let's coordinate on Skype? My id is tinyos. Or we can do email.

phil-levis · 2014-08-21T23:43:48Z

Martin, I checked out your patch and it's good. I've also written a patch for the non-simple Arbiter. The trick is going to be testing this across the components of the system. Unfortunately I'm about to leave for 10 days of vacation -- I will do the complete testing when I return. Here's my patch (it includes yours, slightly edited):

diff --git a/tos/system/ArbiterP.nc b/tos/system/ArbiterP.nc
index 7d6f734..4c9fdcd 100644
--- a/tos/system/ArbiterP.nc
+++ b/tos/system/ArbiterP.nc
@@ -82,7 +82,7 @@ implementation {
         reqResId = id;
       }
       else if (reqResId == id) {
-       return SUCCESS;
+       return EBUSY;
       }
       else return call Queue.enqueue(id);
     }
@@ -190,6 +190,7 @@ implementation {
   task void grantedTask() {
     atomic {
       resId = reqResId;
+      reqResId = default_owner_id;
       state = RES_BUSY;
     }
     call ResourceConfigure.configure[resId]();
diff --git a/tos/system/SimpleArbiterP.nc b/tos/system/SimpleArbiterP.nc
index 05c7dce..1ff4ebc 100644
--- a/tos/system/SimpleArbiterP.nc
+++ b/tos/system/SimpleArbiterP.nc
@@ -76,7 +76,13 @@ implementation {
         post grantedTask();
         return SUCCESS;
       }
-      return call Queue.enqueue(id);
+      // Fixes github issue #255
+      else if ((state == RES_GRANTING) && (resId == id)) {
+        return EBUSY;
+      }
+      else { // Someone else has the resource
+        return call Queue.enqueue(id);
+      }
     }
   }

implements EBUSY on Resource.request()

c32208c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements more EBUSY on Resource.request() (TEP108) #255

Implements more EBUSY on Resource.request() (TEP108) #255

mcerveny commented Feb 25, 2014

alignan commented Feb 25, 2014

andrasbiro commented Feb 27, 2014

cire831 commented Mar 1, 2014

andrasbiro commented Mar 3, 2014

cire831 commented Mar 6, 2014

phil-levis commented Apr 18, 2014

cire831 commented Apr 18, 2014

andrasbiro commented Apr 18, 2014

mcerveny commented Apr 18, 2014

phil-levis commented Apr 20, 2014

mcerveny commented Apr 20, 2014

phil-levis commented Apr 20, 2014

mcerveny commented Apr 20, 2014

phil-levis commented Apr 20, 2014

phil-levis commented Jul 1, 2014

phil-levis commented Aug 21, 2014

Implements more EBUSY on Resource.request() (TEP108) #255

Are you sure you want to change the base?

Implements more EBUSY on Resource.request() (TEP108) #255

Conversation

mcerveny commented Feb 25, 2014

alignan commented Feb 25, 2014

andrasbiro commented Feb 27, 2014

cire831 commented Mar 1, 2014

andrasbiro commented Mar 3, 2014

cire831 commented Mar 6, 2014

phil-levis commented Apr 18, 2014

cire831 commented Apr 18, 2014

andrasbiro commented Apr 18, 2014

mcerveny commented Apr 18, 2014

phil-levis commented Apr 20, 2014

mcerveny commented Apr 20, 2014

phil-levis commented Apr 20, 2014

mcerveny commented Apr 20, 2014

phil-levis commented Apr 20, 2014

phil-levis commented Jul 1, 2014

phil-levis commented Aug 21, 2014