xiaoxiang781216 commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2853378798
> > @jlaitine after this patchset get merged, we can simplify the holder logic for priority inheritance since: > > > > 1. Don't track the multiple holders since mutex always can have one and only one holder > > 2. But need track multiple thread which is blocking/waiting on the mutex with the local variable > > > > Do you think so? > > Yes, cleanup is definitely welcome, but I didn't quite understand yet the idea of the local variable, perhaps you can elaborate a bit more on that? > > My thinking is as follows: > > 1. When a thread comes a holder, it needs to set it to the semaphore; in case of mutex it is just the PID to the val.mholder (the single holder) or adding the holder structure to the semaphore's list in case of counting semaphores. > It doesn't make sense to boost the thread which pass nxsem_wait on a counting semaphore, since the priority boost is meaningful only for a mutex(binary semaphore). That's why I suggest that we can drop the holder of counting semaphores directly. > 2. When a thread blocks on the mutex or counting semaphore, i.e. when the priority boost happens, it needs to add the semaphore to the tcb list of semaphores/mutexes (the "tcb->holdsem" list). All data struct is already defined: - `tcb->waitobj` point to the semaphore - `sem->waitlist` point to all blocking threads - sem->u.mholder point to the holding thread we can implement the priority boost with the above info. > A thread can be a holder of multiple counting semaphores and mutexes, and any of those may have caused priority boost; so this list is always needed for proper priority restoration. > As I said before, we don't need boost the holder of counting semaphore, so all stuff related semholder_s could be removed, recording mholder is enough. > > These two a bit different things (list of sem holders and tracking priority boosts) are now mixed togheter in nxsem_add_holder_tcb/nxsem_allocholder, which just allocates the holder and puts it to both lists, even though for mutexes adding the structure to the semaphores own holder list is redundant, and they don't need to be added at the same time. > > Because 2. is always needed, I found in easier to just use all the existing code and semholder structure for adding the holder and using the priority inheritance code as is, even though it adds the same structure also to the semaphore's list. It doesn't really matter if it is added also to the semaphore's list; you don't save anything by removing it (the holder list pointer is there allocated in sem_s anyway as long as the structure is the same for a mutex and a counting semaphore). > We can drop the support of item 2, which could simplify the code a lot. > But, for the further cleanup / optimization possibilities, these two could be separated. For example making own functions for "nxsem_add_holder_tcb" to add it to tcb->holdsem and "nxsem_add_counting_sem_holder" to add it to semaphores own list. > > Does this make sense? > > Another thing for optimizations, there is one very small and beneficial optimization that we could add; for counting semaphores the "fast path" in libc can be also enabled when the priority inheritance is disabled on a counting semaphore (basically in that case the libc fast functions can just increase/decrease the counter as long as it doesn't block or wake up another threads). This would be beneficial for performance especially in queue implementations (e.g. multiple-writer - single reader, where writers post a signalling semaphore and reader sleeps on the semaphore waiting for the writers). I have code for this ready, if you are interested after this PR. Yes, it's the next step to optimize the counting semaphore usage. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
