You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by chao an <ma...@gmail.com> on 2025/02/05 05:06:29 UTC
The behavior of spin_lock needs everyone's advice
Hi,
The behavior of spin_lock needs everyone's advice
After PR14578 <https://github.com/apache/nuttx/pull/14578> was merged into
the NuttX, the behavior of spin_lock() and spin_lock_irqsave() added the
feature of disable the preemption:
https://github.com/apache/nuttx/pull/14578
The spin lock behavior changed from:
*spin_lock: spin lockspin_lock_irqsave: spin lock + irqsave*
to:
*spin_lock: spin lock + sched_lockspin_lock_irqsave: spin lock + irqsave +
sched_lock*
This change has two key issues
1. *Crash*: Since spin_lock depends on sched_lock, the code using this type
of API needs to re-evaluate the scope of impact, especially when tcb is not
initialized at the startup stage, sched_lock will cause crash
https://github.com/apache/nuttx/pull/15728
2. *Performance degradation*: spin_lock/spin_lock_irqsave is widely used in
the kernel, and more than 90% of the code protected by spin_lock will not
cause context switching, so after the introduction of sched_lock, the
kernel part has introduced new performance overhead
https://github.com/apache/nuttx/pull/15684
Due to the change in API semantics, we need to further optimize all the
locations in the kernel that use spin_lock()/spin_lock_irqsave() to solve
performance issue in evolution.
In PR15705 <https://github.com/apache/nuttx/pull/15705>, Xiaoxiang had some
arguments about API naming with me.
https://github.com/apache/nuttx/pull/15705
My point of view is to restore the original behavior of spin_lock() and
spin_lock_irqsave(), which brings the following benefits:
1. The API semantics remain unchanged, and independent developers and
projects that use these APIs outside the nuttx repository can keep their
code without any adjustment
2. The API naming is consistent with the internal implementation, and the
caller can know what is happening inside the function from the API naming
*Option 1:*
*spin_lock: spin lockspin_lock_nopreempt:
spin_lock + sched_lockspin_lock_irqsave: spin lock + irq
savespin_lock_irqsave_nopreempt: spin_lock + irq save + sched_lock*
@xiaoxiang suggested changing the semantics of the API to hold sched_lock
by default, consistent with Linux:
1. This means that all locations in the kernel that call spin_lock and
spin_lock_irqsave need to be changed
spin_lock_irqsave -> spin_lock_irqsave_preempt
2. It is impossible to know what happens inside the function from the API
naming
*Option 2:*
*spin_lock: spin lock + sched_lockspin_lock_preempt:
spin_lockspin_lock_irqsave: spin lock + irq save +
sched_lockspin_lock_irqsave_preempt: spin_lock + irq save*
I don't know which definition is better, or if any wise person has a better
choice, please give some advice.
Re: The behavior of spin_lock needs everyone's advice
Posted by Sebastien Lorquet <se...@lorquet.fr>.
Tomek,
Regarding this pull request:I'm trying to test it quickly on our
STM32F427 based board, I cant pull the jtag probe before monday.
I have checked out the apache_8 branch of
https://github.com/hujun260/nuttx/tree/apache_8
-the tools/process_config.sh file is still broken, I could work around
that by replacing the file with my version
-the build completes without updating my apps
-the build completes on my colleague's machine
-THE BOARD BOOTS to NSH (not our app, but thats probably another config
update we can fix easily)
So the runtime problem we had is actually caused by these changes.
Conclusion: I validate this revert, It fixes our build.
Thank you all.
Sebastien
On 06/02/2025 04:19, Tomek CEDRO wrote:
> On Wed, Feb 5, 2025 at 12:18 PM hujun260 <hu...@163.com> wrote:
>> I reverted the relevant changes.
>> https://github.com/apache/nuttx/pull/15767
> Sebastien, could you please verify this fix? That change seems to
> impact you directly?
>
> Note this is not a full revert, some optimizations are left in place.
>
> Until resolution is agreed on mailing list here, and proper real world
> hardware validation proof is provided, I have blocked the PR, sorry.
>
Re: The behavior of spin_lock needs everyone's advice
Posted by Tomek CEDRO <to...@cedro.info>.
On Wed, Feb 5, 2025 at 12:18 PM hujun260 <hu...@163.com> wrote:
> I reverted the relevant changes.
> https://github.com/apache/nuttx/pull/15767
Sebastien, could you please verify this fix? That change seems to
impact you directly?
Note this is not a full revert, some optimizations are left in place.
Until resolution is agreed on mailing list here, and proper real world
hardware validation proof is provided, I have blocked the PR, sorry.
--
CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
Re: The behavior of spin_lock needs everyone's advice
Posted by Tomek CEDRO <to...@cedro.info>.
Thank you guys for bringing this discussion on the mailing list! We
really should first discuss any breaking change on the list long
enough until everyone is happy.. and we are sure not to break anything
:-)
Suggestions to verify in depth on a real world hardware any subtle
issue will be soon our standard requirement. For now we need to take
care of this manually. Staging environment could be a good choice too.
This is true, many projects, even industrial ones (I will make one
soon big scale, I did already small scale) depend on NuttX API
stability, that means long term self-compatibility and trust, this is
as important as runtime stability, maybe even more in these
applications. This is something unique to NuttX and we should really
respect and nurture :-)
Not many of us have this privilege to be paid for every commit, but if
things get broken all of us will loose money and customers and trust.
Therefore we should really avoid any breaking changes.. and focusing
on alternatives instead.
From "user" point of view, if the change breaks kernel api, it should
be rejected, also reverted if already applied. If this is the case
right now and reverting will fix the release we should revert the
change and make a patch release 12.8.1. Is this the main problem right
now and will that fix 12.8.0? Then we should go for it :-)
For the later part I need to quote two parts so we have a solid
reference point :-)
Part 1:
On Wed, Feb 5, 2025 at 12:59 PM Guiding Li <li...@gmail.com> wrote:
> *Option 1:*
> spin_lock: spin lock
> spin_lock_nopreempt: spin_lock + sched_lock
> spin_lock_irqsave: spin lock + irqsave
> spin_lock_irqsave_nopreempt: spin_lock + irq save + sched_lock
>
> *Option 2:*
> spin_lock: spin lock + sched_lock
> spin_lock_preempt: spin_lock
> spin_lock_irqsave: spin lock + irq save +sched_lock
> spin_lock_irqsave_preempt: spin_lock + irq save
>
> From the correctness level:
> The Option2 is correct, because if you call sem_post()/syslog()/... within
> spin_lock() will cause a system crash.
>
> From the performance level:
> The Option2 has lower efficiency as you said in the NON-SMP case.
>
> Then can we have a method that takes both ?
> Based on option1, we add a check if someone called sem_post()/syslog()...
> then system ASSERT. Alert the people who should change their usage.
> And also the performance will be considered.
Part 2:
On Wed, Feb 5, 2025 at 7:46 AM Xiang Xiao <xi...@gmail.com> wrote:
> On Wed, Feb 5, 2025 at 1:06 PM chao an <ma...@gmail.com> wrote:
> > My point of view is to restore the original behavior of spin_lock() and
> > spin_lock_irqsave(), which brings the following benefits:
> > 1. The API semantics remain unchanged, and independent developers and
> > projects that use these APIs outside the nuttx repository can keep their
> > code without any adjustment
> > 2. The API naming is consistent with the internal implementation, and the
> > caller can know what is happening inside the function from the API naming
> >
> > *Option 1:*
> >
> > *spin_lock: spin lockspin_lock_nopreempt:
> > spin_lock + sched_lockspin_lock_irqsave: spin lock + irq
> > savespin_lock_irqsave_nopreempt: spin_lock + irq save + sched_lock*
> >
> > @xiaoxiang suggested changing the semantics of the API to hold sched_lock
> > by default, consistent with Linux:
> > 1. This means that all locations in the kernel that call spin_lock and
> > spin_lock_irqsave need to be changed
> >
> Since holding sched locks make spinlock more safe, the change doesn't
> require the change from the caller in general.
>
> > spin_lock_irqsave -> spin_lock_irqsave_preempt
> > 2. It is impossible to know what happens inside the function from the API
> > naming
> >
> > *Option 2:*
> >
> > *spin_lock: spin lock + sched_lockspin_lock_preempt:
> > spin_lockspin_lock_irqsave: spin lock + irq save +
> > sched_lockspin_lock_irqsave_preempt: spin_lock + irq save*
> >
> > I don't know which definition is better, or if any wise person has a better
> > choice, please give some advice.
>
> Here is the summary why I prefer to add sched_lock in
> spin_lock/spin_lock_irqsave by default:
>
> - The default(short) api should be safe since not all people understand
> the schedule behaviour deeply, please see the related discussion:
>
> https://github.com/apache/nuttx/issues/9531
> https://github.com/apache/nuttx/issues/1138
> https://www.kernel.org/doc/Documentation/preempt-locking.txt
>
>
> - spin_lock without sched_lock is unusable in the normal thread context,
> since the scheduler may suspend the spinlock owner thread at any time
> - API semantic align with Linux to avoid Linux developer use the unsafe
> api without any notification
> - the caller of enter_critical section can call sem_post/mq_send without
> problem, spin_lock/spin_lock_irqsave can only achieve the same behaviour by
> holding sched lock
With the above examples my main concerns are:
1. Functions are not quite elegant and self-explanatory (i.e. why
there are two spinlocks in spin_lockspin_lock_irqsave name? Who will
understand things like spin lock + sched_lockspin_lock_preempt what
does it even mean?).
2. Functions hide a "default" inside, something assumed not explicit,
that is not labeled by the function name.
3. If we change the "default" hidden behavior things for sure get
broken and nobody will know why.
4. We are changing existing kernel api anyways which is bad by design
and I am strongly against it :-)
Why not create backward-compatible variants with fine grained
functionality with self-explanatory names? Users will have a choice on
building blocks to choose, and the responsibility will be on the user
part.. and on our part how well we document it with every possible
explanation and example.. maybe even example application to keep
things crystal clear :-)
spin_lock = spin_lock
spin_lock_shed_lock = spin_lock -> shed_lock
spin_lock_irqsave = spin_lock -> irqsave
spin_lock_irqsave_shed_lock = spin_lock -> irqsave -> shed_lock
spin_lock_shed_lock_irqsave = spin_lock -> shed_lock -> irqsave
(..etc)
This way:
* We maintain backward kernel api compatiblity, all old stuff works as expected.
* Things that need optimization can change without breaking existing
stuff, step by step block by block where needed.
* Function names are elegant and self-explanatory (i.e. spin_lock
tells there is spin_lock, spin_lock_shed_lock tells there will be
spin_lock and no preemption, spin_lock_irqsave tells there is
spin_lock and irqsave, spin_lock_irqsave_shed_lock tells there is
spin_lock with irqsave and no preemption).
* Call order inside is self-explanatory.
* There is zero hidden assumed default behavior.
* This should cover all necessary scenarios.
* This enables Linux compatible kernel API where necessary (linux call
names just wrap and call our calls).
* It will be easier to write and understand documentation.
* It will be easier to write and understand examples.
* It will be easy to provide examples of bad use.
* It will be easy to catch bad commits right away.
* And last but not least it may be easy to identify and wrap SMP code
with ifdefs and simply have no SMP code on no-SMP hardware?
What do you think? :-)
Tomek
--
CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
Re: The behavior of spin_lock needs everyone's advice
Posted by Tomek CEDRO <to...@cedro.info>.
On Thu, Feb 6, 2025 at 2:16 AM Nathan Hartman <ha...@gmail.com> wrote:
> Even though there are some problems, I am glad that the community is
> actively addressing them and that we are discussing how to improve in the
> future.
Exactly :-) When doing R&D you need thousands of failures to get
something working.. but then.. maaaan its joy.. and only then you
precisely understand why this is the best choice (at this point in
time he he). We all know "This Is The Way"^TM.. not only to get things
done.. but to get them done right :-)
I feel too that recent hard months will just make NuttX stronger and
NuttX Community even more united :-)
Remember, problem is not a problem, the real problem is what you do
with the problem :-)
--
CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
Re: The behavior of spin_lock needs everyone's advice
Posted by Nathan Hartman <ha...@gmail.com>.
Even though there are some problems, I am glad that the community is
actively addressing them and that we are discussing how to improve in the
future.
I can't offer advice on the spinlock question at this time because I
haven't studied it, but when I ship my current project I should have more
time to look into OS automated testing. I'd encourage others to think of
ways to test the OS performance so that hard-to-find bugs can be exposed.
Cheers,
Nathan
On Wed, Feb 5, 2025 at 2:16 PM Tiago Medicci Serrano <
tiago.medicci@gmail.com> wrote:
> Thanks everyone for bringing light to this discussion!
>
> Well, I support reverting the introduced changes as in
> https://github.com/apache/nuttx/pull/15767 and continue working on the
> https://github.com/apache/nuttx/pull/15705
>
> In summary, I prefer to use Option 1:
>
> *Option 1:*
> >
> > spin_lock: spin lock
> > spin_lock_nopreempt: spin_lock + sched_lock
> > spin_lock_irqsave: spin lock + irqsave
> > spin_lock_irqsave_nopreempt: spin_lock + irq save + sched_lock
>
>
> I liked ligd's proposal too:
>
> Based on option1, we add a check if someone called sem_post()/syslog()...
> > then system ASSERT. Alert the people who should change their usage.
> > And also the performance will be considered.
>
>
> Best regards,
>
> Em qua., 5 de fev. de 2025 às 12:30, Sebastien Lorquet <
> sebastien@lorquet.fr>
> escreveu:
>
> > Hello,
> >
> > On 05/02/2025 15:43, chao an wrote:
> > > I just want to solve them. I know it's difficult for every developer.
> So
> > if
> > > you come across similar issues, you
> > > don't need to spend too much time trying to solve them on your own.
> > Yes, I understand very well, but fixing too fast can lead to bugs
> > elsewhere. NuttX is a complex code base, we need coordinated efforts. We
> > are a common project.
> > If you dont get feedback you need patience until you get feedback, and
> > you need to trigger more alarms. You cant just say "I didnt get the
> > feedback I required, this is fine, lets push this anyway".
> >
> >
> > On 05/02/2025 15:43, chao an wrote:
> > > Agree with you. This is precisely why I sent the spin_lock issue to the
> > > mailing list. It's to keep the semantics of
> > > the existing API consistent with the previous ones, so as to prevent
> > > individual developers and 3-party projects from
> > > spending more time debugging problems caused by NuttX upgrades.
> >
> > Thank you for forwarding to the list.
> >
> > However for this particular issue, it would be dishonest for me to give
> > advice because I just dont know.
> >
> > I hope the answers to your issues can be obtained from other
> contributors.
> >
> > Sebastien
> >
> >
>
Re: The behavior of spin_lock needs everyone's advice
Posted by Tiago Medicci Serrano <ti...@gmail.com>.
Thanks everyone for bringing light to this discussion!
Well, I support reverting the introduced changes as in
https://github.com/apache/nuttx/pull/15767 and continue working on the
https://github.com/apache/nuttx/pull/15705
In summary, I prefer to use Option 1:
*Option 1:*
>
> spin_lock: spin lock
> spin_lock_nopreempt: spin_lock + sched_lock
> spin_lock_irqsave: spin lock + irqsave
> spin_lock_irqsave_nopreempt: spin_lock + irq save + sched_lock
I liked ligd's proposal too:
Based on option1, we add a check if someone called sem_post()/syslog()...
> then system ASSERT. Alert the people who should change their usage.
> And also the performance will be considered.
Best regards,
Em qua., 5 de fev. de 2025 às 12:30, Sebastien Lorquet <se...@lorquet.fr>
escreveu:
> Hello,
>
> On 05/02/2025 15:43, chao an wrote:
> > I just want to solve them. I know it's difficult for every developer. So
> if
> > you come across similar issues, you
> > don't need to spend too much time trying to solve them on your own.
> Yes, I understand very well, but fixing too fast can lead to bugs
> elsewhere. NuttX is a complex code base, we need coordinated efforts. We
> are a common project.
> If you dont get feedback you need patience until you get feedback, and
> you need to trigger more alarms. You cant just say "I didnt get the
> feedback I required, this is fine, lets push this anyway".
>
>
> On 05/02/2025 15:43, chao an wrote:
> > Agree with you. This is precisely why I sent the spin_lock issue to the
> > mailing list. It's to keep the semantics of
> > the existing API consistent with the previous ones, so as to prevent
> > individual developers and 3-party projects from
> > spending more time debugging problems caused by NuttX upgrades.
>
> Thank you for forwarding to the list.
>
> However for this particular issue, it would be dishonest for me to give
> advice because I just dont know.
>
> I hope the answers to your issues can be obtained from other contributors.
>
> Sebastien
>
>
Re: The behavior of spin_lock needs everyone's advice
Posted by Sebastien Lorquet <se...@lorquet.fr>.
Hello,
On 05/02/2025 15:43, chao an wrote:
> I just want to solve them. I know it's difficult for every developer. So if
> you come across similar issues, you
> don't need to spend too much time trying to solve them on your own.
Yes, I understand very well, but fixing too fast can lead to bugs
elsewhere. NuttX is a complex code base, we need coordinated efforts. We
are a common project.
If you dont get feedback you need patience until you get feedback, and
you need to trigger more alarms. You cant just say "I didnt get the
feedback I required, this is fine, lets push this anyway".
On 05/02/2025 15:43, chao an wrote:
> Agree with you. This is precisely why I sent the spin_lock issue to the
> mailing list. It's to keep the semantics of
> the existing API consistent with the previous ones, so as to prevent
> individual developers and 3-party projects from
> spending more time debugging problems caused by NuttX upgrades.
Thank you for forwarding to the list.
However for this particular issue, it would be dishonest for me to give
advice because I just dont know.
I hope the answers to your issues can be obtained from other contributors.
Sebastien
Re: The behavior of spin_lock needs everyone's advice
Posted by chao an <ma...@gmail.com>.
> The real prejudice is on my side when it takes one week to update from a
> 6 month old nuttx and the resulting image does not even run. Can you
> imagine this burden and frustration and stress? My boss is not happy
> about this and it's not my fault. I know it's an open source project
> without warranty, but clearly, pushing too many untested code is not
> helping.
I understand your situation and can really empathize with you. However,
every time I encounter similar problems,
I just want to solve them. I know it's difficult for every developer. So if
you come across similar issues, you
don't need to spend too much time trying to solve them on your own. You
have a community, and everyone from community
will actively step up to fix the problems.
Moreover, you can see that some core-developers fix regression issues
within 24 hours. What we're actually worried
about is that too many problems are lurking in the community.
I'm really honored to hear you complaining about what has happened recently.
> My goal is to see both evolution and stability of nuttx of all users,
> because I'm not against changes, but I want to see a better way to
> integrate the changes.
Agree with you. This is precisely why I sent the spin_lock issue to the
mailing list. It's to keep the semantics of
the existing API consistent with the previous ones, so as to prevent
individual developers and 3-party projects from
spending more time debugging problems caused by NuttX upgrades.
BRs,
Sebastien Lorquet <se...@lorquet.fr> 于2025年2月5日周三 22:27写道：
> Hello,
>
> The real prejudice is on my side when it takes one week to update from a
> 6 month old nuttx and the resulting image does not even run. Can you
> imagine this burden and frustration and stress? My boss is not happy
> about this and it's not my fault. I know it's an open source project
> without warranty, but clearly, pushing too many untested code is not
> helping.
>
> Can you please take steps so we can see an effort from your company to
> establish a more concerted contribution initiative instead of a constant
> stream of changes every day, with the great risk to introduce unwanted
> bugs? Honestly, we all know the automatic tests run today are not enough
> to ensure all your changes are working. Something has to be done.
>
> Pre-staging your changes for some time before contributing would be a
> great step in getting everything better. It would allow more time for
> reaction.
>
>
> On 05/02/2025 15:11, chao an wrote:
> >> This is not an Apache Project communication channel, so it does not
> >> exist as far as we are all concerned.
> > actually we've had extensive discussions on the community PR, but no one
> > has ever cared about this issue. I would feel highly honored if I could
> get
> > some tips from you.
> > https://github.com/apache/nuttx/pull/15705
>
> I am not a specialist of spinlocks. But I know changing it can break
> many things in subtle ways and I know it's better to be safe than sorry.
> This project has actual users and we need to be careful.
>
> If no one has ever cared about the issue, and you know someone has to
> care, then the solution is not to push it anyway. You need to force us
> to care.
>
> I am honest, there are too many pull requests and commit, I do not even
> know where and how to start. I cannot spend full time looking at all
> your contributions, it's too much, it's complex and subtle.
>
> A pre-contribution repository would help greatly to see the desired
> contributions, discuss them and select the best options. The only long
> term solution is to slow down. If you cant slow down, then we need a
> "buffer" to adapt your rate of contribution to the rate of review
> available from nuttx contributions.
>
>
> >
> >> As a general rule, please stop testing your changes in prod, I know you
> >> refuse to reply to me but you read anyway.
> > I just haven't figured out how to reply to you. I agree with your
> opinion,
> > However, could you please refrain from criticizing others with
> prejudice? I
> > could have chosen not to send this email. But I hope that we can have
> > further discussions to make the revisions even more perfect.
>
> We have different cultures and reactions so it is expected that this
> kind of friction will occur. This is an expression of my long term
> frustration (several years).
>
> I know it is not the best way to react but I have not figured any other
> method to wake up people about this perceived problem.
>
> My goal is to see both evolution and stability of nuttx of all users,
> because I'm not against changes, but I want to see a better way to
> integrate the changes.
>
> Sebastien
>
>
> >
> >> If I understand correctly spinlocks are basically a no-op when no SMP is
> > involved, which is the majority of CPUs supported by NuttX. You shall not
> > break all platforms.
> >> I urge you all to revert any commit that might have broken something in
> > this domain, redesign it carefully on your own code fork, and then push a
> > final working version to nuttx.
> >
> > I'm also really grateful for your advice. I believe everything is getting
> > better.
> >
> > BRs,
> >
> > Sebastien Lorquet <se...@lorquet.fr> 于2025年2月5日周三 22:01写道：
> >
> >> Hi,
> >>
> >> On 05/02/2025 13:03, chao an wrote:
> >>> I talked with
> >>> Xiaoxiang in WeChat before.
> >> This is not an Apache Project communication channel, so it does not
> >> exist as far as we are all concerned.
> >>
> >>
> >> These changes MUST be tested in a separate code branch so all platforms
> >> can be tested and validated for several weeks.
> >>
> >> We need an explicit GO on every supported arch (SMP and not SMP) before
> >> this goes into main/master.
> >>
> >> These are critical core changes that have potential to introduce very
> >> nasty bugs that will be very hard to track.
> >>
> >> It may take time to validate all of this, and this delay is fine.
> >>
> >>
> >> As a general rule, please stop testing your changes in prod, I know you
> >> refuse to reply to me but you read anyway.
> >>
> >>
> >> Sebastien
> >>
> >>
>
Re: The behavior of spin_lock needs everyone's advice
Posted by Sebastien Lorquet <se...@lorquet.fr>.
Hello,
The real prejudice is on my side when it takes one week to update from a
6 month old nuttx and the resulting image does not even run. Can you
imagine this burden and frustration and stress? My boss is not happy
about this and it's not my fault. I know it's an open source project
without warranty, but clearly, pushing too many untested code is not
helping.
Can you please take steps so we can see an effort from your company to
establish a more concerted contribution initiative instead of a constant
stream of changes every day, with the great risk to introduce unwanted
bugs? Honestly, we all know the automatic tests run today are not enough
to ensure all your changes are working. Something has to be done.
Pre-staging your changes for some time before contributing would be a
great step in getting everything better. It would allow more time for
reaction.
On 05/02/2025 15:11, chao an wrote:
>> This is not an Apache Project communication channel, so it does not
>> exist as far as we are all concerned.
> actually we've had extensive discussions on the community PR, but no one
> has ever cared about this issue. I would feel highly honored if I could get
> some tips from you.
> https://github.com/apache/nuttx/pull/15705
I am not a specialist of spinlocks. But I know changing it can break
many things in subtle ways and I know it's better to be safe than sorry.
This project has actual users and we need to be careful.
If no one has ever cared about the issue, and you know someone has to
care, then the solution is not to push it anyway. You need to force us
to care.
I am honest, there are too many pull requests and commit, I do not even
know where and how to start. I cannot spend full time looking at all
your contributions, it's too much, it's complex and subtle.
A pre-contribution repository would help greatly to see the desired
contributions, discuss them and select the best options. The only long
term solution is to slow down. If you cant slow down, then we need a
"buffer" to adapt your rate of contribution to the rate of review
available from nuttx contributions.
>
>> As a general rule, please stop testing your changes in prod, I know you
>> refuse to reply to me but you read anyway.
> I just haven't figured out how to reply to you. I agree with your opinion,
> However, could you please refrain from criticizing others with prejudice? I
> could have chosen not to send this email. But I hope that we can have
> further discussions to make the revisions even more perfect.
We have different cultures and reactions so it is expected that this
kind of friction will occur. This is an expression of my long term
frustration (several years).
I know it is not the best way to react but I have not figured any other
method to wake up people about this perceived problem.
My goal is to see both evolution and stability of nuttx of all users,
because I'm not against changes, but I want to see a better way to
integrate the changes.
Sebastien
>
>> If I understand correctly spinlocks are basically a no-op when no SMP is
> involved, which is the majority of CPUs supported by NuttX. You shall not
> break all platforms.
>> I urge you all to revert any commit that might have broken something in
> this domain, redesign it carefully on your own code fork, and then push a
> final working version to nuttx.
>
> I'm also really grateful for your advice. I believe everything is getting
> better.
>
> BRs,
>
> Sebastien Lorquet <se...@lorquet.fr> 于2025年2月5日周三 22:01写道：
>
>> Hi,
>>
>> On 05/02/2025 13:03, chao an wrote:
>>> I talked with
>>> Xiaoxiang in WeChat before.
>> This is not an Apache Project communication channel, so it does not
>> exist as far as we are all concerned.
>>
>>
>> These changes MUST be tested in a separate code branch so all platforms
>> can be tested and validated for several weeks.
>>
>> We need an explicit GO on every supported arch (SMP and not SMP) before
>> this goes into main/master.
>>
>> These are critical core changes that have potential to introduce very
>> nasty bugs that will be very hard to track.
>>
>> It may take time to validate all of this, and this delay is fine.
>>
>>
>> As a general rule, please stop testing your changes in prod, I know you
>> refuse to reply to me but you read anyway.
>>
>>
>> Sebastien
>>
>>
Re: The behavior of spin_lock needs everyone's advice
Posted by chao an <ma...@gmail.com>.
> This is not an Apache Project communication channel, so it does not
> exist as far as we are all concerned.
actually we've had extensive discussions on the community PR, but no one
has ever cared about this issue. I would feel highly honored if I could get
some tips from you.
https://github.com/apache/nuttx/pull/15705
> As a general rule, please stop testing your changes in prod, I know you
> refuse to reply to me but you read anyway.
I just haven't figured out how to reply to you. I agree with your opinion,
However, could you please refrain from criticizing others with prejudice? I
could have chosen not to send this email. But I hope that we can have
further discussions to make the revisions even more perfect.
> If I understand correctly spinlocks are basically a no-op when no SMP is
involved, which is the majority of CPUs supported by NuttX. You shall not
break all platforms.
> I urge you all to revert any commit that might have broken something in
this domain, redesign it carefully on your own code fork, and then push a
final working version to nuttx.
I'm also really grateful for your advice. I believe everything is getting
better.
BRs,
Sebastien Lorquet <se...@lorquet.fr> 于2025年2月5日周三 22:01写道：
> Hi,
>
> On 05/02/2025 13:03, chao an wrote:
> > I talked with
> > Xiaoxiang in WeChat before.
>
> This is not an Apache Project communication channel, so it does not
> exist as far as we are all concerned.
>
>
> These changes MUST be tested in a separate code branch so all platforms
> can be tested and validated for several weeks.
>
> We need an explicit GO on every supported arch (SMP and not SMP) before
> this goes into main/master.
>
> These are critical core changes that have potential to introduce very
> nasty bugs that will be very hard to track.
>
> It may take time to validate all of this, and this delay is fine.
>
>
> As a general rule, please stop testing your changes in prod, I know you
> refuse to reply to me but you read anyway.
>
>
> Sebastien
>
>
Re: The behavior of spin_lock needs everyone's advice
Posted by Sebastien Lorquet <se...@lorquet.fr>.
Hi,
On 05/02/2025 13:03, chao an wrote:
> I talked with
> Xiaoxiang in WeChat before.
This is not an Apache Project communication channel, so it does not
exist as far as we are all concerned.
These changes MUST be tested in a separate code branch so all platforms
can be tested and validated for several weeks.
We need an explicit GO on every supported arch (SMP and not SMP) before
this goes into main/master.
These are critical core changes that have potential to introduce very
nasty bugs that will be very hard to track.
It may take time to validate all of this, and this delay is fine.
As a general rule, please stop testing your changes in prod, I know you
refuse to reply to me but you read anyway.
Sebastien
Re: The behavior of spin_lock needs everyone's advice
Posted by chao an <ma...@gmail.com>.
> Then can we have a method that takes both ?
> Based on option1, we add a check if someone called sem_post()/syslog()...
> then system ASSERT. Alert the people who should change their usage.
> And also the performance will be considered.
Hi, Guiding
I fully agree with your opinion. This is also the suggestion I talked with
Xiaoxiang in WeChat before. We need to check whether the irq is saveed or
the preemption is disabled in *spin_lock()*, and trigger the assertion if
we find the wrong usage.
BRs,
Guiding Li <li...@gmail.com> 于2025年2月5日周三 19:57写道：
> Hi Chao,
>
> For these two options:
>
> *Option 1:*
>
> spin_lock: spin lock
> spin_lock_nopreempt: spin_lock + sched_lock
> spin_lock_irqsave: spin lock + irqsave
> spin_lock_irqsave_nopreempt: spin_lock + irq save + sched_lock
>
>
> *Option 2:*
>
> spin_lock: spin lock + sched_lock
> spin_lock_preempt: spin_lock
> spin_lock_irqsave: spin lock + irq save +sched_lock
> spin_lock_irqsave_preempt: spin_lock + irq save
>
> From the correctness level:
> The Option2 is correct, because if you call sem_post()/syslog()/... within
> spin_lock() will cause a system crash.
>
> From the performance level:
> The Option2 has lower efficiency as you said in the NON-SMP case.
>
> Then can we have a method that takes both ?
> Based on option1, we add a check if someone called sem_post()/syslog()...
> then system ASSERT. Alert the people who should change their usage.
> And also the performance will be considered.
>
> BRs,
> ligd
>
> chao an <ma...@gmail.com> 于2025年2月5日周三 19:34写道：
>
> > I do not agree with the revert related changes. Local locks are very
> > helpful for system real-time performance. I just have some suggestions on
> > API semantics.
> >
> > Changes of kernel API semantics need to be carefully considered,
> especially
> > if these functions will be used by individual developers and projects.
> The
> > new semantics should be named with a new API to minimize the impact.
> >
> > BRs,
> >
> > hujun260 <hu...@163.com> 于2025年2月5日周三 19:19写道：
> >
> > > I reverted the relevant changes.
> > >
> > >
> > > https://github.com/apache/nuttx/pull/15767
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > At 2025-02-05 13:06:29, "chao an" <ma...@gmail.com> wrote:
> > > >Hi,
> > > >
> > > >The behavior of spin_lock needs everyone's advice
> > > >
> > > >After PR14578 <https://github.com/apache/nuttx/pull/14578> was merged
> > > into
> > > >the NuttX, the behavior of spin_lock() and spin_lock_irqsave() added
> the
> > > >feature of disable the preemption:
> > > >
> > > >https://github.com/apache/nuttx/pull/14578
> > > >
> > > >The spin lock behavior changed from:
> > > >
> > > >
> > > >*spin_lock: spin lockspin_lock_irqsave: spin lock + irqsave*
> > > >
> > > >to:
> > > >
> > > >
> > > >*spin_lock: spin lock + sched_lockspin_lock_irqsave: spin lock +
> > irqsave +
> > > >sched_lock*
> > > >
> > > >This change has two key issues
> > > >1. *Crash*: Since spin_lock depends on sched_lock, the code using this
> > > type
> > > >of API needs to re-evaluate the scope of impact, especially when tcb
> is
> > > not
> > > >initialized at the startup stage, sched_lock will cause crash
> > > >
> > > >https://github.com/apache/nuttx/pull/15728
> > > >
> > > >2. *Performance degradation*: spin_lock/spin_lock_irqsave is widely
> used
> > > in
> > > >the kernel, and more than 90% of the code protected by spin_lock will
> > not
> > > >cause context switching, so after the introduction of sched_lock, the
> > > >kernel part has introduced new performance overhead
> > > >
> > > >https://github.com/apache/nuttx/pull/15684
> > > >
> > > >
> > > >Due to the change in API semantics, we need to further optimize all
> the
> > > >locations in the kernel that use spin_lock()/spin_lock_irqsave() to
> > solve
> > > >performance issue in evolution.
> > > >In PR15705 <https://github.com/apache/nuttx/pull/15705>, Xiaoxiang
> had
> > > some
> > > >arguments about API naming with me.
> > > >
> > > >https://github.com/apache/nuttx/pull/15705
> > > >
> > > >My point of view is to restore the original behavior of spin_lock()
> and
> > > >spin_lock_irqsave(), which brings the following benefits:
> > > >1. The API semantics remain unchanged, and independent developers and
> > > >projects that use these APIs outside the nuttx repository can keep
> their
> > > >code without any adjustment
> > > >2. The API naming is consistent with the internal implementation, and
> > the
> > > >caller can know what is happening inside the function from the API
> > naming
> > > >
> > > >*Option 1:*
> > > >
> > > >
> > > >
> > > >*spin_lock: spin lockspin_lock_nopreempt:
> > > >spin_lock + sched_lockspin_lock_irqsave: spin lock + irq
> > > >savespin_lock_irqsave_nopreempt: spin_lock + irq save + sched_lock*
> > > >
> > > >@xiaoxiang suggested changing the semantics of the API to hold
> > sched_lock
> > > >by default, consistent with Linux:
> > > >1. This means that all locations in the kernel that call spin_lock and
> > > >spin_lock_irqsave need to be changed
> > > >spin_lock_irqsave -> spin_lock_irqsave_preempt
> > > >2. It is impossible to know what happens inside the function from the
> > API
> > > >naming
> > > >
> > > >*Option 2:*
> > > >
> > > >
> > > >
> > > >*spin_lock: spin lock + sched_lockspin_lock_preempt:
> > > > spin_lockspin_lock_irqsave: spin lock + irq save +
> > > >sched_lockspin_lock_irqsave_preempt: spin_lock + irq save*
> > > >
> > > >I don't know which definition is better, or if any wise person has a
> > > better
> > > >choice, please give some advice.
> > >
> >
>
Re: The behavior of spin_lock needs everyone's advice
Posted by Guiding Li <li...@gmail.com>.
Hi Chao,
For these two options:
*Option 1:*
spin_lock: spin lock
spin_lock_nopreempt: spin_lock + sched_lock
spin_lock_irqsave: spin lock + irqsave
spin_lock_irqsave_nopreempt: spin_lock + irq save + sched_lock
*Option 2:*
spin_lock: spin lock + sched_lock
spin_lock_preempt: spin_lock
spin_lock_irqsave: spin lock + irq save +sched_lock
spin_lock_irqsave_preempt: spin_lock + irq save
From the correctness level:
The Option2 is correct, because if you call sem_post()/syslog()/... within
spin_lock() will cause a system crash.
From the performance level:
The Option2 has lower efficiency as you said in the NON-SMP case.
Then can we have a method that takes both ?
Based on option1, we add a check if someone called sem_post()/syslog()...
then system ASSERT. Alert the people who should change their usage.
And also the performance will be considered.
BRs,
ligd
chao an <ma...@gmail.com> 于2025年2月5日周三 19:34写道：
> I do not agree with the revert related changes. Local locks are very
> helpful for system real-time performance. I just have some suggestions on
> API semantics.
>
> Changes of kernel API semantics need to be carefully considered, especially
> if these functions will be used by individual developers and projects. The
> new semantics should be named with a new API to minimize the impact.
>
> BRs,
>
> hujun260 <hu...@163.com> 于2025年2月5日周三 19:19写道：
>
> > I reverted the relevant changes.
> >
> >
> > https://github.com/apache/nuttx/pull/15767
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > At 2025-02-05 13:06:29, "chao an" <ma...@gmail.com> wrote:
> > >Hi,
> > >
> > >The behavior of spin_lock needs everyone's advice
> > >
> > >After PR14578 <https://github.com/apache/nuttx/pull/14578> was merged
> > into
> > >the NuttX, the behavior of spin_lock() and spin_lock_irqsave() added the
> > >feature of disable the preemption:
> > >
> > >https://github.com/apache/nuttx/pull/14578
> > >
> > >The spin lock behavior changed from:
> > >
> > >
> > >*spin_lock: spin lockspin_lock_irqsave: spin lock + irqsave*
> > >
> > >to:
> > >
> > >
> > >*spin_lock: spin lock + sched_lockspin_lock_irqsave: spin lock +
> irqsave +
> > >sched_lock*
> > >
> > >This change has two key issues
> > >1. *Crash*: Since spin_lock depends on sched_lock, the code using this
> > type
> > >of API needs to re-evaluate the scope of impact, especially when tcb is
> > not
> > >initialized at the startup stage, sched_lock will cause crash
> > >
> > >https://github.com/apache/nuttx/pull/15728
> > >
> > >2. *Performance degradation*: spin_lock/spin_lock_irqsave is widely used
> > in
> > >the kernel, and more than 90% of the code protected by spin_lock will
> not
> > >cause context switching, so after the introduction of sched_lock, the
> > >kernel part has introduced new performance overhead
> > >
> > >https://github.com/apache/nuttx/pull/15684
> > >
> > >
> > >Due to the change in API semantics, we need to further optimize all the
> > >locations in the kernel that use spin_lock()/spin_lock_irqsave() to
> solve
> > >performance issue in evolution.
> > >In PR15705 <https://github.com/apache/nuttx/pull/15705>, Xiaoxiang had
> > some
> > >arguments about API naming with me.
> > >
> > >https://github.com/apache/nuttx/pull/15705
> > >
> > >My point of view is to restore the original behavior of spin_lock() and
> > >spin_lock_irqsave(), which brings the following benefits:
> > >1. The API semantics remain unchanged, and independent developers and
> > >projects that use these APIs outside the nuttx repository can keep their
> > >code without any adjustment
> > >2. The API naming is consistent with the internal implementation, and
> the
> > >caller can know what is happening inside the function from the API
> naming
> > >
> > >*Option 1:*
> > >
> > >
> > >
> > >*spin_lock: spin lockspin_lock_nopreempt:
> > >spin_lock + sched_lockspin_lock_irqsave: spin lock + irq
> > >savespin_lock_irqsave_nopreempt: spin_lock + irq save + sched_lock*
> > >
> > >@xiaoxiang suggested changing the semantics of the API to hold
> sched_lock
> > >by default, consistent with Linux:
> > >1. This means that all locations in the kernel that call spin_lock and
> > >spin_lock_irqsave need to be changed
> > >spin_lock_irqsave -> spin_lock_irqsave_preempt
> > >2. It is impossible to know what happens inside the function from the
> API
> > >naming
> > >
> > >*Option 2:*
> > >
> > >
> > >
> > >*spin_lock: spin lock + sched_lockspin_lock_preempt:
> > > spin_lockspin_lock_irqsave: spin lock + irq save +
> > >sched_lockspin_lock_irqsave_preempt: spin_lock + irq save*
> > >
> > >I don't know which definition is better, or if any wise person has a
> > better
> > >choice, please give some advice.
> >
>
Re: The behavior of spin_lock needs everyone's advice
Posted by chao an <ma...@gmail.com>.
I do not agree with the revert related changes. Local locks are very
helpful for system real-time performance. I just have some suggestions on
API semantics.
Changes of kernel API semantics need to be carefully considered, especially
if these functions will be used by individual developers and projects. The
new semantics should be named with a new API to minimize the impact.
BRs,
hujun260 <hu...@163.com> 于2025年2月5日周三 19:19写道：
> I reverted the relevant changes.
>
>
> https://github.com/apache/nuttx/pull/15767
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> At 2025-02-05 13:06:29, "chao an" <ma...@gmail.com> wrote:
> >Hi,
> >
> >The behavior of spin_lock needs everyone's advice
> >
> >After PR14578 <https://github.com/apache/nuttx/pull/14578> was merged
> into
> >the NuttX, the behavior of spin_lock() and spin_lock_irqsave() added the
> >feature of disable the preemption:
> >
> >https://github.com/apache/nuttx/pull/14578
> >
> >The spin lock behavior changed from:
> >
> >
> >*spin_lock: spin lockspin_lock_irqsave: spin lock + irqsave*
> >
> >to:
> >
> >
> >*spin_lock: spin lock + sched_lockspin_lock_irqsave: spin lock + irqsave +
> >sched_lock*
> >
> >This change has two key issues
> >1. *Crash*: Since spin_lock depends on sched_lock, the code using this
> type
> >of API needs to re-evaluate the scope of impact, especially when tcb is
> not
> >initialized at the startup stage, sched_lock will cause crash
> >
> >https://github.com/apache/nuttx/pull/15728
> >
> >2. *Performance degradation*: spin_lock/spin_lock_irqsave is widely used
> in
> >the kernel, and more than 90% of the code protected by spin_lock will not
> >cause context switching, so after the introduction of sched_lock, the
> >kernel part has introduced new performance overhead
> >
> >https://github.com/apache/nuttx/pull/15684
> >
> >
> >Due to the change in API semantics, we need to further optimize all the
> >locations in the kernel that use spin_lock()/spin_lock_irqsave() to solve
> >performance issue in evolution.
> >In PR15705 <https://github.com/apache/nuttx/pull/15705>, Xiaoxiang had
> some
> >arguments about API naming with me.
> >
> >https://github.com/apache/nuttx/pull/15705
> >
> >My point of view is to restore the original behavior of spin_lock() and
> >spin_lock_irqsave(), which brings the following benefits:
> >1. The API semantics remain unchanged, and independent developers and
> >projects that use these APIs outside the nuttx repository can keep their
> >code without any adjustment
> >2. The API naming is consistent with the internal implementation, and the
> >caller can know what is happening inside the function from the API naming
> >
> >*Option 1:*
> >
> >
> >
> >*spin_lock: spin lockspin_lock_nopreempt:
> >spin_lock + sched_lockspin_lock_irqsave: spin lock + irq
> >savespin_lock_irqsave_nopreempt: spin_lock + irq save + sched_lock*
> >
> >@xiaoxiang suggested changing the semantics of the API to hold sched_lock
> >by default, consistent with Linux:
> >1. This means that all locations in the kernel that call spin_lock and
> >spin_lock_irqsave need to be changed
> >spin_lock_irqsave -> spin_lock_irqsave_preempt
> >2. It is impossible to know what happens inside the function from the API
> >naming
> >
> >*Option 2:*
> >
> >
> >
> >*spin_lock: spin lock + sched_lockspin_lock_preempt:
> > spin_lockspin_lock_irqsave: spin lock + irq save +
> >sched_lockspin_lock_irqsave_preempt: spin_lock + irq save*
> >
> >I don't know which definition is better, or if any wise person has a
> better
> >choice, please give some advice.
>
Re:The behavior of spin_lock needs everyone's advice
Posted by hujun260 <hu...@163.com>.
I reverted the relevant changes.
https://github.com/apache/nuttx/pull/15767
At 2025-02-05 13:06:29, "chao an" <ma...@gmail.com> wrote:
>Hi,
>
>The behavior of spin_lock needs everyone's advice
>
>After PR14578 <https://github.com/apache/nuttx/pull/14578> was merged into
>the NuttX, the behavior of spin_lock() and spin_lock_irqsave() added the
>feature of disable the preemption:
>
>https://github.com/apache/nuttx/pull/14578
>
>The spin lock behavior changed from:
>
>
>*spin_lock: spin lockspin_lock_irqsave: spin lock + irqsave*
>
>to:
>
>
>*spin_lock: spin lock + sched_lockspin_lock_irqsave: spin lock + irqsave +
>sched_lock*
>
>This change has two key issues
>1. *Crash*: Since spin_lock depends on sched_lock, the code using this type
>of API needs to re-evaluate the scope of impact, especially when tcb is not
>initialized at the startup stage, sched_lock will cause crash
>
>https://github.com/apache/nuttx/pull/15728
>
>2. *Performance degradation*: spin_lock/spin_lock_irqsave is widely used in
>the kernel, and more than 90% of the code protected by spin_lock will not
>cause context switching, so after the introduction of sched_lock, the
>kernel part has introduced new performance overhead
>
>https://github.com/apache/nuttx/pull/15684
>
>
>Due to the change in API semantics, we need to further optimize all the
>locations in the kernel that use spin_lock()/spin_lock_irqsave() to solve
>performance issue in evolution.
>In PR15705 <https://github.com/apache/nuttx/pull/15705>, Xiaoxiang had some
>arguments about API naming with me.
>
>https://github.com/apache/nuttx/pull/15705
>
>My point of view is to restore the original behavior of spin_lock() and
>spin_lock_irqsave(), which brings the following benefits:
>1. The API semantics remain unchanged, and independent developers and
>projects that use these APIs outside the nuttx repository can keep their
>code without any adjustment
>2. The API naming is consistent with the internal implementation, and the
>caller can know what is happening inside the function from the API naming
>
>*Option 1:*
>
>
>
>*spin_lock: spin lockspin_lock_nopreempt:
>spin_lock + sched_lockspin_lock_irqsave: spin lock + irq
>savespin_lock_irqsave_nopreempt: spin_lock + irq save + sched_lock*
>
>@xiaoxiang suggested changing the semantics of the API to hold sched_lock
>by default, consistent with Linux:
>1. This means that all locations in the kernel that call spin_lock and
>spin_lock_irqsave need to be changed
>spin_lock_irqsave -> spin_lock_irqsave_preempt
>2. It is impossible to know what happens inside the function from the API
>naming
>
>*Option 2:*
>
>
>
>*spin_lock: spin lock + sched_lockspin_lock_preempt:
> spin_lockspin_lock_irqsave: spin lock + irq save +
>sched_lockspin_lock_irqsave_preempt: spin_lock + irq save*
>
>I don't know which definition is better, or if any wise person has a better
>choice, please give some advice.
Re: The behavior of spin_lock needs everyone's advice
Posted by Sebastien Lorquet <se...@lorquet.fr>.
Hello Xiaomi dev team,
If I understand correctly spinlocks are basically a no-op when no SMP is
involved, which is the majority of CPUs supported by NuttX. You shall
not break all platforms.
I urge you all to revert any commit that might have broken something in
this domain, redesign it carefully on your own code fork, and then push
a final working version to nuttx.
Even if nuttx has releases, you are basically pushing untested changes
to a main branch (in core code!) that is used by many people.
You are critically breaking nuttx for everyone.
You are a large company with financial resources, so PLEASE do your job,
install a gitea or gitlab on a raspberry pi in your company and do your
dev in your private fork.
It is too many influence to push THAT MUCH amount of breakage to anyone.
The NuttX project is mature, it has real industrial users, it is not a
playground for your funny OS hacking.
PLEASE RESPOND and ACT to save the project. This problem need to be
adressed at the root level.
Let me remind you that NuttX 12.8.0 is fully broken on my stm32f4 board
and that I cannot repair it until next week because my schedule is fully
booked on other critical project.
Our customer does not understand why adding tcp keepalive requires that
much work. We are going to loose customers because of your NuttX bad
community behaviour.
I do not want to additionnally debug your locking crap on a cpu that
does not need any of it.
PLEASE DO SOMETHING RESPONSIBLE.
Sebastien
On 05/02/2025 08:13, chao an wrote:
>
> >It isn't an initialization problem, the real cause is some code abusing
> >spin lock(lock/unlock in the different thread).
> >After holding sched_lock in spinlock_irqsave, the api requires that the
> >lock/unlock come from the same thread.
>
> So this brings up a potential problem with spin_lock, right?
>
> >All performance critical code is evaluating carefully to skip the
> >sched_lock/sched_unlock, like this:
> >https://github.com/apache/nuttx/pull/15695
> >And the optimization of sched_lock is under heavy development.
>
>
> *raw_spin_lock_irqsave()* is not equivalent to without *sched_lock()*.
> On the contrary, in the Linux kernel, raw_spin_lock_irqsave() means
> that preemption is disabled by default.
> https://github.com/torvalds/linux/blob/master/include/linux/spinlock_api_smp.h#L104-L113
>
> image.png
>
>
> >Here is the summary why I prefer to add sched_lock in
> >spin_lock/spin_lock_irqsave by default:
>
> > - The default(short) api should be safe since not all people
> understand
> > the schedule behaviour deeply, please see the related discussion:
>
> > https://github.com/apache/nuttx/issues/9531
> > https://github.com/apache/nuttx/issues/1138
> > https://www.kernel.org/doc/Documentation/preempt-locking.txt
>
> > - spin_lock without sched_lock is unusable in the normal thread
> context,
> > since the scheduler may suspend the spinlock owner thread at any time
>
> 1. spin_lock can be applied in scenarios requiring more fine-grained
> control. For example, in some cases, we can conditionally decide
> whether to hold the spin_lock, rather than simply forcing all users to
> use the version with sched_lock:
> image.png
>
> > - API semantic align with Linux to avoid Linux developer use the
> unsafe
> > api without any notification
>
> 2. NuttX is not Linux. We need to enable API users to truly understand
> what happens inside the API just by looking at its name.
>
> > - the caller of enter_critical section can call sem_post/mq_send
> without
> > problem, spin_lock/spin_lock_irqsave can only achieve the same
> behaviour by
> > holding sched lock
>
> 3. We should handle scenarios that may cause scheduling differently.
> We should use *spin_lock_irqsave_nopreempt()* instead of providing
> developers with a low - performance version by default.
>
Re: The behavior of spin_lock needs everyone's advice
Posted by chao an <ma...@gmail.com>.
>It isn't an initialization problem, the real cause is some code abusing
>spin lock(lock/unlock in the different thread).
>After holding sched_lock in spinlock_irqsave, the api requires that the
>lock/unlock come from the same thread.
So this brings up a potential problem with spin_lock, right?
>All performance critical code is evaluating carefully to skip the
>sched_lock/sched_unlock, like this:
>https://github.com/apache/nuttx/pull/15695
>And the optimization of sched_lock is under heavy development.
*raw_spin_lock_irqsave()* is not equivalent to without *sched_lock()*. On
the contrary, in the Linux kernel, raw_spin_lock_irqsave() means that
preemption is disabled by default.
https://github.com/torvalds/linux/blob/master/include/linux/spinlock_api_smp.h#L104-L113
[image: image.png]
>Here is the summary why I prefer to add sched_lock in
>spin_lock/spin_lock_irqsave by default:
> - The default(short) api should be safe since not all people understand
> the schedule behaviour deeply, please see the related discussion:
> https://github.com/apache/nuttx/issues/9531
> https://github.com/apache/nuttx/issues/1138
> https://www.kernel.org/doc/Documentation/preempt-locking.txt
> - spin_lock without sched_lock is unusable in the normal thread context,
> since the scheduler may suspend the spinlock owner thread at any time
1. spin_lock can be applied in scenarios requiring more fine-grained
control. For example, in some cases, we can conditionally decide whether to
hold the spin_lock, rather than simply forcing all users to use the version
with sched_lock:
[image: image.png]
> - API semantic align with Linux to avoid Linux developer use the unsafe
> api without any notification
2. NuttX is not Linux. We need to enable API users to truly understand what
happens inside the API just by looking at its name.
> - the caller of enter_critical section can call sem_post/mq_send without
> problem, spin_lock/spin_lock_irqsave can only achieve the same
behaviour by
> holding sched lock
3. We should handle scenarios that may cause scheduling differently. We
should use *spin_lock_irqsave_nopreempt()* instead of providing developers
with a low - performance version by default.
Re: The behavior of spin_lock needs everyone's advice
Posted by Xiang Xiao <xi...@gmail.com>.
On Wed, Feb 5, 2025 at 1:06 PM chao an <ma...@gmail.com> wrote:
> Hi,
>
> The behavior of spin_lock needs everyone's advice
>
> After PR14578 <https://github.com/apache/nuttx/pull/14578> was merged into
> the NuttX, the behavior of spin_lock() and spin_lock_irqsave() added the
> feature of disable the preemption:
>
> https://github.com/apache/nuttx/pull/14578
>
> The spin lock behavior changed from:
>
>
> *spin_lock: spin lockspin_lock_irqsave: spin lock + irqsave*
>
> to:
>
>
> *spin_lock: spin lock + sched_lockspin_lock_irqsave: spin lock + irqsave +
> sched_lock*
>
> This change has two key issues
> 1. *Crash*: Since spin_lock depends on sched_lock, the code using this type
> of API needs to re-evaluate the scope of impact, especially when tcb is not
> initialized at the startup stage, sched_lock will cause crash
>
> https://github.com/apache/nuttx/pull/15728
>
>
It isn't an initialization problem, the real cause is some code abusing
spin lock(lock/unlock in the different thread).
After holding sched_lock in spinlock_irqsave, the api requires that the
lock/unlock come from the same thread.
2. *Performance degradation*: spin_lock/spin_lock_irqsave is widely used in
> the kernel, and more than 90% of the code protected by spin_lock will not
> cause context switching, so after the introduction of sched_lock, the
> kernel part has introduced new performance overhead
>
> https://github.com/apache/nuttx/pull/15684
>
> Due to the change in API semantics, we need to further optimize all the
> locations in the kernel that use spin_lock()/spin_lock_irqsave() to solve
> performance issue in evolution.
>
All performance critical code is evaluating carefully to skip the
sched_lock/sched_unlock, like this:
https://github.com/apache/nuttx/pull/15695
And the optimization of sched_lock is under heavy development.
> In PR15705 <https://github.com/apache/nuttx/pull/15705>, Xiaoxiang had
> some
> arguments about API naming with me.
>
> https://github.com/apache/nuttx/pull/15705
>
> My point of view is to restore the original behavior of spin_lock() and
> spin_lock_irqsave(), which brings the following benefits:
> 1. The API semantics remain unchanged, and independent developers and
> projects that use these APIs outside the nuttx repository can keep their
> code without any adjustment
> 2. The API naming is consistent with the internal implementation, and the
> caller can know what is happening inside the function from the API naming
>
> *Option 1:*
>
>
>
> *spin_lock: spin lockspin_lock_nopreempt:
> spin_lock + sched_lockspin_lock_irqsave: spin lock + irq
> savespin_lock_irqsave_nopreempt: spin_lock + irq save + sched_lock*
>
> @xiaoxiang suggested changing the semantics of the API to hold sched_lock
> by default, consistent with Linux:
> 1. This means that all locations in the kernel that call spin_lock and
> spin_lock_irqsave need to be changed
>
Since holding sched locks make spinlock more safe, the change doesn't
require the change from the caller in general.
> spin_lock_irqsave -> spin_lock_irqsave_preempt
> 2. It is impossible to know what happens inside the function from the API
> naming
>
> *Option 2:*
>
>
>
> *spin_lock: spin lock + sched_lockspin_lock_preempt:
> spin_lockspin_lock_irqsave: spin lock + irq save +
> sched_lockspin_lock_irqsave_preempt: spin_lock + irq save*
>
> I don't know which definition is better, or if any wise person has a better
> choice, please give some advice.
>
Here is the summary why I prefer to add sched_lock in
spin_lock/spin_lock_irqsave by default:
- The default(short) api should be safe since not all people understand
the schedule behaviour deeply, please see the related discussion:
https://github.com/apache/nuttx/issues/9531
https://github.com/apache/nuttx/issues/1138
https://www.kernel.org/doc/Documentation/preempt-locking.txt
- spin_lock without sched_lock is unusable in the normal thread context,
since the scheduler may suspend the spinlock owner thread at any time
- API semantic align with Linux to avoid Linux developer use the unsafe
api without any notification
- the caller of enter_critical section can call sem_post/mq_send without
problem, spin_lock/spin_lock_irqsave can only achieve the same behaviour by
holding sched lock