Re: [PR] sched/spin_lock: rename raw_spin_* to spin_*_notrace [nuttx]

[via GitHub]

xiaoxiang781216 commented on PR #15705:
URL: https://github.com/apache/nuttx/pull/15705#issuecomment-2622074717

   > > https://docs.kernel.org/locking/locktypes.html contain the detailed 
information, here is the keypoint: <img alt="image" width="633" 
src="https://private-user-images.githubusercontent.com/18066964/407624628-9c3314ff-e647-4578-a9b5-d4ed80b9c3ef.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzgxNTA1NTksIm5iZiI6MTczODE1MDI1OSwicGF0aCI6Ii8xODA2Njk2NC80MDc2MjQ2MjgtOWMzMzE0ZmYtZTY0Ny00NTc4LWE5YjUtZDRlZDgwYjljM2VmLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAxMjklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMTI5VDExMzA1OVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTJkYTY4ZTUwZTNkYzE3NjY4NjBjNDFlMDlhNDhkMDEzZGEyZDFmMGQyYzU3N2JjMmUzNzRmZTcyZDU5NTQ2ODEmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.d5XaJ6ss_uy5uxVCKCQS8_cbh8sgOphN1t7LbtlfvUo";>
 <img alt="image" width="643" src="https://private-user-images.githubuserc
 
ontent.com/18066964/407625036-f11f4513-49f7-4cd1-be87-d8199a31d5d2.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzgxNTA1NTksIm5iZiI6MTczODE1MDI1OSwicGF0aCI6Ii8xODA2Njk2NC80MDc2MjUwMzYtZjExZjQ1MTMtNDlmNy00Y2QxLWJlODctZDgxOTlhMzFkNWQyLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAxMjklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMTI5VDExMzA1OVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTcyZjJkNTA0ZWZkY2MyYzZiMTMzMDQ5OTBlNmE1ZGM0NmZhNGRhNWVkYmYyMmI3MzczYzkwNTlhNTRkNzZmYTcmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.TLVfsui2MfTDM3DzG2DaCJCIaBdNQpZZ479-F1IvWMg">
   > 
   > 1. so did you notice that **sched_lock()** was removed from 
**PREEMPT_RT**? This is the beginning of **RT**, because nuttx did not have 
**sched_lock()** in **spin_lock()** involved before, why does your change make 
the entire system bear the overhead of **sched_lock()**?
   
   Yes, but you need first change the exuction of ALL IRQ from the interupt 
context to the thread context. Before that happen, the sched lock must be taken 
at the saem time we hold spinlock.
   
   > 2. Replacing mutex with spin_lock is the **second** stage of optimization, 
which does not conflict with the current PR
   
   Let's emphasis the key point:
   
   1. The busy loop lock(e.g. spinlock) must hold sched lock too
   2. The sleep lock(e.g. mutex) don't/shouldn't hold sched lock
   
   So, you can't do the change in seperate step without breaking the code:
   
   1. Remove sched lock first
   2. Change spinlock to mutex at the late time
   
   Let's consider a VERY COMMON case in SMP:
   
   1. thread0 on CPU0 hold a spinlock without hold sched lock
   2. thread1 on CPU1 wake up thread2
   3. OS decide suspend thread0 and resume thread2 on CPU0
   4. thread1 on CPU1 want to hold the spinlock hold on thread0
   
   but since thread0 is already in suspend state, and hard to know when it can 
resume.
   It's VERY dangerous to schedule the thread which hold spinlock.
   
   > 3. You can look at the spin_lock implementation in other RTOS, no one will 
disable preemption(**zephyr**)
   >    
https://github.com/zephyrproject-rtos/zephyr/blob/c66a0bb2a9269dc483079d5cc24ba77b8c332a9a/include/zephyr/spinlock.h#L182-L214
   > 
   
   zephyr's k_spin_lock disable irq too, but nuttx's spin_lock doesn't disable 
irq.
   
   > Now we have 4 APIs available to developers
   > 
   > 1.spin_lock 2. spin_lock_nopreempt 3. spin_lock_irqsave 4. 
spin_lock_irqsave_nopreempt
   > 
   > If you think the code needs to hold sched_lock, please use 
*_spin_lock__nopreempt__ instead of **spin_lock()**
   
   No, the correct semantics(same as Linux) is:
   
   1. spin_lock
   2. spin_lock_preempt
   3. spin_lock_irqsave
   4. spin_lock_irqsave_preempt
   
   Otherwise, the most code will sufer the random deadlock or long time busy 
loop like the example I mention previously.


-- 
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