Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libs/native: sem race #42

Open
beefdeadbeef opened this issue May 24, 2024 · 7 comments
Open

libs/native: sem race #42

beefdeadbeef opened this issue May 24, 2024 · 7 comments

Comments

@beefdeadbeef
Copy link

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 <bugurt.h>
+#include <stdatomic.h>
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
Copy link
Owner

shkolnick-kun commented May 24, 2024

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
Copy link
Owner

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

@beefdeadbeef
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

shkolnick-kun commented May 25, 2024

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
Copy link
Owner

shkolnick-kun commented May 25, 2024

I must correct *_cs functions docs.

@shkolnick-kun shkolnick-kun reopened this May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants