Actually, let me correct my previous statement that mutexs and semaphores need critical sections, I think they just need memory barriers. We can look at how the Linux kernel does it to double check our knowledge. Here’s s quick stack overflow link, for what it’s worth. https://stackoverflow.com/questions/33754443/is-linux-mutex-lock-implemented-using-memory-barrier
*Daniel Appiagyei | Embedded Software Engineer *Email: daniel.appiag...@braincorp.com <bog...@braincorporation.com>*Brain* * Corp™ *10182 Telesis Ct, Suite 100 San Diego, CA 92121 (858)-689-7600 www.braincorp.com On Wed, Aug 28, 2024 at 6:15 PM Daniel Appiagyei < daniel.appiag...@braincorp.com> wrote: > Hi Gregory, thank you for the quick response! > > RE: 'lockcount' needing to be in a critical section: > That's good information to know, I wasn't aware before. I'm > searching nuttx for exact matches of the string 'sched_lock();' to find > occurrences and seeing this is not always done. Yeah a critical section ( > up_irq_save > <https://github.com/apache/nuttx/blob/master/arch/arm/include/armv7-m/irq.h#L405>), > at least on arm arch, calls get_base_pri() > <https://github.com/apache/nuttx/blob/master/arch/arm/include/armv7-m/irq.h#L317-L326>, > which does have a memory barrier so that memory will be coherent throughout > the entire system. > > > All places where the lockcount is *incremented* should be within a > critical section > Emphasis mine, I believe all places where lockcount is written OR read > require atomicity and the most up-to-date value in memory with respect to > another, so we'd need a critical section in those cases too. > I like the idea of adding a DEBUGASSERT, my only fear is that others users > such as myself have used 'sched_lock()' in their code and will be met with > a runtime crash when they pull in these changes. > > > If we are in a critical section, then other threads > > will not run at least until the thread blocks or terminates (even in SMP > > mode). > > Yeah, I agree now, the critical section should solve this since it has the > barrier i mentioned above. > > On a similar note, I believe all mutex and semaphore functions should have > a critical section as well. E.g., when nxmutex_is_hold() > <https://github.com/apache/nuttx/blob/master/libs/libc/misc/lib_mutex.c#L147> > checks mutex->holder, it must absolutely have the latest up-to-date value > to verify ownership, otherwise there is a bug. What are your thoughts on > this? > > On Wed, Aug 28, 2024 at 5:28 PM Gregory Nutt <spudan...@gmail.com> wrote: > >> >> On 8/28/2024 6:14 PM, Daniel Appiagyei wrote: >> > Hi all, >> > Looking at sched_lock.c ( >> > >> https://github.com/apache/nuttx/blob/master/sched/sched/sched_lock.c#L187 >> ), >> > , specifically at the non-SMP sched_lock() implementation, how do we >> ensure >> > that: >> > 1. There's no data-race when incrementing the lock count: >> > 'rtcb->lockcount++;', (given that no mutex or critical section is >> > surrounding that code so other tasks or IRQs are free to interrupt the >> > assembly instructions), and >> >> All places where the lockcount is incremented should be within a >> critical section. I believe that is true. But if you find place where >> that is not true, that would indeed be a bug. >> >> Perhaps a DEBUGASSERTion that we are in a critical section before >> incrementing lockcount would be a good verification of this belief. >> >> > 2. The scheduler is 100% prevented from allowing a different task to run >> > after lock_count is incremented given that we don't use a data memory >> > barrier or data synchronization barrier to ensure the write to >> 'lock_count' >> > is visible to the rest of the system in memory? >> Adding a barrier might be a good idea, although I have never seen this >> to be an issue. If we are in a critical section, then other threads >> will not run at least until the thread blocks or terminates (even in SMP >> mode). >> > For 1, see cppreference 'Threads and data races' on data races being >> > undefined behavior ( >> https://en.cppreference.com/w/c/language/memory_model). >> > >> > Best, >> > Daniel > > > > -- > > > *Daniel Appiagyei | Embedded Software Engineer *Email: > daniel.appiag...@braincorp.com > <bog...@braincorporation.com>*Brain* > * Corp™ *10182 Telesis Ct, Suite 100 > San Diego, CA 92121 > > (858)-689-7600 > www.braincorp.com >
