libs/native: sem race

[beefdeadbeef]

with bgrt_sem_free_cs() in play (i.e. from isr), bgrt_sem_lock() starts skipping some loops. for me, on stm32-ish m4f with 1ms tick, 300-400 secs of test run (see below) was enough to spot the error:

--- %< --- static bgrt_sem_t sem; static bgrt_cnt_t icounter, pcounter;

static void idle(void *ctx) { (void)ctx; timer_enable_counter(TIM3); loop: asm volatile ("wfe":::"memory"); goto loop; }

static void heartbeat(void *ctx) { (void)ctx; loop: bgrt_wait_time(500ul); gpio_toggle(GPIOC, GPIO13); debugf("i=%d p=%d d=%d s=%d\n", icounter, pcounter, icounter-pcounter, sem.counter); goto loop; }

static void semtest(void *ctx) { (void)ctx; bgrt_st_t st;

loop: st = bgrt_sem_lock(&sem); if (st != BGRT_ST_OK) { debugf("st=%d\n", st); asm("bkpt #0"); } pcounter++; if ((pcounter % 89) == 0) bgrt_wait_time(8); if ((pcounter % 325) == 0) bgrt_wait_time(12); if ((pcounter % 617) == 0) bgrt_wait_time(16); goto loop; }

void tim3_isr(void) { timer_clear_flag(TIM3, TIM_SR_UIF); bgrt_sem_free_cs(&sem); icounter++; }

int main() { bgrt_init();

   /*... hw init ... */

    bgrt_sem_init_cs(&sem, 0);

    /* macro wrapping init_sc + run_sc */
bgrt_initial_proc(HB, heartbeat);
bgrt_initial_proc(IDLE, idle);
bgrt_initial_proc(PROC0, semtest);

    debugf("init done\n");
    bgrt_start();
    return 0;

}

--- %< ---

and test output looks like (system ticks in square brackets):

--- %< --- [00000000] main(): init done [000001f4] heartbeat(): i=750 p=750 d=0 s=0 [000003e8] heartbeat(): i=1500 p=1500 d=0 s=0 [000005dc] heartbeat(): i=2250 p=2250 d=0 s=0 [000007d0] heartbeat(): i=3000 p=3000 d=0 s=0 [000009c4] heartbeat(): i=3750 p=3750 d=0 s=0 [00000bb8] heartbeat(): i=4500 p=4500 d=0 s=0 [00000dac] heartbeat(): i=5250 p=5250 d=0 s=0 [00000fa0] heartbeat(): i=6000 p=6000 d=0 s=0 [00001194] heartbeat(): i=6750 p=6750 d=0 s=0 ..... (skip to some busy part) ... [00020d64] heartbeat(): i=201746 p=201746 d=0 s=0 [00020f58] heartbeat(): i=202496 p=202475 d=21 s=21 [0002114c] heartbeat(): i=203246 p=203246 d=0 s=0 [00021340] heartbeat(): i=203996 p=203988 d=8 s=8 [00021534] heartbeat(): i=204746 p=204746 d=0 s=0 .... (and to first skipped run) ... [0005f370] heartbeat(): i=584988 p=584988 d=0 s=0 [0005f564] heartbeat(): i=585738 p=585738 d=0 s=0 [0005f758] heartbeat(): i=586488 p=586488 d=0 s=0 [0005f94c] heartbeat(): i=587238 p=587238 d=0 s=0 [0005fb40] heartbeat(): i=587988 p=587988 d=0 s=0 [0005fd34] heartbeat(): i=588738 p=588735 d=3 s=2 <<<<<< [0005ff28] heartbeat(): i=589488 p=589487 d=1 s=0 [0006011c] heartbeat(): i=590238 p=590237 d=1 s=0 [00060310] heartbeat(): i=590988 p=590987 d=1 s=0 ... (and so on, up to seventh ) ... [000f53d4] heartbeat(): i=1506719 p=1506700 d=19 s=12 [000f55c8] heartbeat(): i=1507469 p=1507462 d=7 s=0 [000f57bc] heartbeat(): i=1508219 p=1508212 d=7 s=0 [000f59b0] heartbeat(): i=1508969 p=1508962 d=7 s=0 [000f5ba4] heartbeat(): i=1509719 p=1509707 d=12 s=5 [000f5d98] heartbeat(): i=1510469 p=1510462 d=7 s=0

--- %< ---

making sem->counter atomic, like below, sort of fixes this issue for me, but i suspect there's no atomics for poor little 8-bit micros

--- %< --- diff --git a/libs/native/sem.h b/libs/native/sem.h index d1319b1..27ea874 100644 --- a/libs/native/sem.h +++ b/libs/native/sem.h @@ -83,6 +83,7 @@ sMMM+........................-hmMo/ds oMo.-o :h s:h Nysd.-Ny-h:...... \brief \~russian Заголовок счётных семафоров. \~english A counting semaphores header. */

include

+#include BGRT_CDECL_BEGIN /Семафор/ typedef struct bgrt_priv_sem_t bgrt_sem_t;/!< \~russian Смотри #bgrt_priv_sem_t; \~english See #bgrt_priv_sem_t; / @@ -106,7 +107,7 @@ A counting semaphore can be locked by one process and freed by another. struct bgrt_priv_sem_t { bgrt_sync_t wait;/!< \~russian Список ожидающих процессов. \~english A list of waiting processes. /

• bgrt_cnt_t counter;/!< \~russian Счётчик ресурсов. \~english A resource counter. /
• _Atomic(bgrt_cnt_t) counter;/!< \~russian Счётчик ресурсов. \~english A resource counter. /

  ifdef BGRT_CONFIG_MP

  bgrt_lock_t lock;/!< \~russian Спин-блокировка. \~english A sync spin-lock. /

  endif / BGRT_CONFIG_MP /

--- %< ---

and

--- %< --- diff --git a/libs/native/sem.c b/libs/native/sem.c index 0b5d01d..e996b9a 100644 --- a/libs/native/sem.c +++ b/libs/native/sem.c @@ -115,7 +115,7 @@ bgrt_st_t bgrt_sem_try_lock(bgrt_sem_t * sem)

 if (sem->counter)
 {

• sem->counter--;
• atomic_fetch_sub(&sem->counter, 1); ret = BGRT_ST_OK; }

@@ -154,7 +154,7 @@ static bgrt_st_t _sem_lock_fsm(bgrt_va_wr_t* va)

     if (sem->counter)
     {

• sem->counter--;

• atomic_fetch_sub(&sem->counter, 1); BGRT_SPIN_FREE(sem);

       return BGRT_ST_OK;

  @@ -207,7 +207,7 @@ static bgrt_st_t _sem_free_body(bgrt_sem_t sem) ret = bgrt_priv_sync_wake((bgrt_sync_t )sem, (bgrt_proc_t *)0, (bgrt_flag_t)0); if (BGRT_ST_EEMPTY == ret) {

• sem->counter++;
• atomic_fetch_add(&sem->counter, 1); ret = BGRT_ST_OK; } return ret; --- %<---

[shkolnick-kun]

Hi!

I'm sorry but bgrt_sem_free_cs documentation must be rewritten. Even if you rewrite

sem->counter++;

with

atomic_fetch_add(&sem->counter, 1);

etc. you won't get rid of race conditions!

Since BuguRTOS uses preemptible kernel which is run in kernel threads (one thread per CPU core) everything that calls the private API must be run by kernel threads or in critical sections of pocesses.

This design slows down interrupt handling in cases when you need to interact with the kernel or some threads but when you don't you'll probably get shorter worst case interrupt handling times as only some ines of code can't be preempted by high priority ISRs.

In fact, bgrt_sem_free_cs must be called in a kernel thread by either bgrt_vic_iterator here or BGRT_KBLOCK_LPFIC_HOOK here.

Also, you don't need to write your own idle loop as kernel threads do this job by calling BGRT_CONFIG_SAVE_POWER here.

[shkolnick-kun]

And yes, if you want your ISR to communicate with the kernel then you should use BGRT_ISR wrapper.

[beefdeadbeef]

well, that's somewhat dicouraging, honestly -- but hey, nobody's perfect. as for idle thread -- yes, at first i had

define BGRT_CONFIG_SAVE_POWER() do { for(;;) asm ("wfe":::"memory"); } while(0)

with no visible (according to orbtop tool from orbuculum project) difference -- thus dedicated idle loop.

[beefdeadbeef]

so, with vint and stuff i can't have more than one vint run per system tick, right ? quite tight loop , sigh. anyway, thanks for clarifications, closing.

[shkolnick-kun]

so, with vint and stuff i can't have more than one vint run per system tick, right ? 

No, you can have as much virtual interrupts per systick as your platform can handle.

In fact virtual interrupts have greater priorities than scheduler or syscalls. Only BGRT_KBLOCK_LPFIC_HOOK have greater priority than virtual interrupts.

[shkolnick-kun]

well, that's somewhat dicouraging, honestly -- but hey, nobody's perfect. as for idle thread -- yes, at first i had #define BGRT_CONFIG_SAVE_POWER() do { for(;;) asm ("wfe":::"memory"); } while(0) with no visible (according to orbtop tool from orbuculum project) difference -- thus dedicated idle loop.

That is strange... If you have only PROC0 locked on the sem then there won't be any running process and then BGRT_CONFIG_SAVE_POWER() will be called...

This code may be the reason:

if ((pcounter % 89) == 0) bgrt_wait_time(8);
if ((pcounter % 325) == 0) bgrt_wait_time(12);
if ((pcounter % 617) == 0) bgrt_wait_time(16);

If PROC0 hasn't been initialized with is_rt flag set then it is ready to run during bgrt_wait_time execution and the kernel won't call BGRT_CONFIG_SAVE_POWER().

[shkolnick-kun]

I must correct *_cs functions docs.