Change default behavior of semaphores?

[Nathan Hartman <ha...@gmail.com>]

PR-5070 [1] proposes to change the default behavior of semaphores.

If implemented, this will be a breaking change that affects downstream
projects.

Please help review this PR for correctness and standards compliance,
keeping in mind the Inviolable principles of NuttX [2], particularly
the section on POSIX compliance:

[[[

Strict POSIX compliance

- Strict conformance to the portable standard OS interface as defined
  at OpenGroup.org.
- A deeply embedded system requires some special support. Special
  support must be minimized.
- The portable interface must never be compromised only for the sake
  of expediency.
- Expediency or even improved performance are not justifications for
  violation of the strict POSIX interface.

]]]

[1] https://github.com/apache/incubator-nuttx/pull/5070

[2] https://github.com/hartmannathan/incubator-nuttx/blob/master/INVIOLABLES.md

Thanks,
Nathan

Re: Change default behavior of semaphores?

[Nathan Hartman <ha...@gmail.com>]

On Wed, Jan 5, 2022 at 10:30 PM Xiang Xiao <xi...@gmail.com>
wrote:

> On Thu, Jan 6, 2022 at 4:23 AM Gregory Nutt <sp...@gmail.com> wrote:
>
> > > I truly believe that priority inheritance on counting semaphores should
> > be
> >
> > explicitly enabled (disabled by default) and enabling it by default might
> >
> > lead to unexpected priority boost for low priority tasks that violates
> >
> > real-time requirements.
> >
> >
> >
> > It has been enabled by default since day one so nothing will be broken.
> >
>
> No, all discussion and patch already show that it already breaks many
> things and even crashes the system. To avoid the problem, we have to review
> all places which use semaphore and make the adjustion if needed. Even five
> years later, we still found the problem from ostest, the most used
> verification tool on NuttX.
>
>
> > Disabling it now will break things – that is an orthogonal discussion to
> > spec compliance.
>
>
> Yes, but "Strict POSIX compliance" is top one of "Inviolable Principles of
> NuttX". Enabling priority inheritance by default breaks most programs which
> use sem_t.
>
>
> >   Currently, priorioty inheritance is explicitly disabled
> > in all places where it should not be enabled (i.e., signaling semahores).
> >
> >
> Yes, but it's just for the mainline code. It isn't true for 3rd
> party libraries even if they are compliant with POSIX strictly.
>
>
> >
> >
> > If the default changes, then you would need to explicitly enable prority
> > inheritance on all semaphores that are used as locks in order to retain
> the
> > previous behavior.  See
> >
> >
> https://cwiki.apache.org/confluence/display/NUTTX/Signaling+Semaphores+and+Priority+Inheritance
>
>
> Yes, the plan is to fix all mainline code. Actually, we plan to use the
> simple wrapper on top of sem:
> https://github.com/apache/incubator-nuttx/blob/master/include/nuttx/mutex.h
> To make the lock usage inside the kernel more explicitly.
>
>
> >
> > – the POSIX definition does not have this issue because POSIX does not
> > support priority inheritance on counting semaphores.
> >
>

I initially expressed concerns but now I am convinced it should be fixed.
We just need to make sure the docs are up to date and release notes are
very clear what needs to change in non-mainline code that depends on the
old non-compliant behavior.

Cheers,
Nathan

Re: Change default behavior of semaphores?

[Xiang Xiao <xi...@gmail.com>]

On Thu, Jan 6, 2022 at 4:23 AM Gregory Nutt <sp...@gmail.com> wrote:

> > I truly believe that priority inheritance on counting semaphores should
> be
>
> explicitly enabled (disabled by default) and enabling it by default might
>
> lead to unexpected priority boost for low priority tasks that violates
>
> real-time requirements.
>
>
>
> It has been enabled by default since day one so nothing will be broken.
>

No, all discussion and patch already show that it already breaks many
things and even crashes the system. To avoid the problem, we have to review
all places which use semaphore and make the adjustion if needed. Even five
years later, we still found the problem from ostest, the most used
verification tool on NuttX.


> Disabling it now will break things – that is an orthogonal discussion to
> spec compliance.


Yes, but "Strict POSIX compliance" is top one of "Inviolable Principles of
NuttX". Enabling priority inheritance by default breaks most programs which
use sem_t.


>   Currently, priorioty inheritance is explicitly disabled
> in all places where it should not be enabled (i.e., signaling semahores).
>
>
Yes, but it's just for the mainline code. It isn't true for 3rd
party libraries even if they are compliant with POSIX strictly.


>
>
> If the default changes, then you would need to explicitly enable prority
> inheritance on all semaphores that are used as locks in order to retain the
> previous behavior.  See
>
> https://cwiki.apache.org/confluence/display/NUTTX/Signaling+Semaphores+and+Priority+Inheritance


Yes, the plan is to fix all mainline code. Actually, we plan to use the
simple wrapper on top of sem:
https://github.com/apache/incubator-nuttx/blob/master/include/nuttx/mutex.h
To make the lock usage inside the kernel more explicitly.


>
> – the POSIX definition does not have this issue because POSIX does not
> support priority inheritance on counting semaphores.
>

Re: Change default behavior of semaphores?

[Petro Karashchenko <pe...@gmail.com>]

Hi,

Yes, POSIX does not support priority inheritance on counting semaphores,
but priority inheritance things seems to be already broken for a long time.
I mean the changes like https://github.com/apache/incubator-nuttx/pull/5170
just highlight the top of an iceberg.

For the POSIX compliance of pthread mutexes default state to have
PTHREAD_PRIO_NONE
as a default protocol I will spin another discussion and will not overload
this thread with irrelevant information. That changes will definitely
affect many many applications, but we have to make a hard choice anyway.
What I meant that POSIX pthread mutex case just shows that in general
features that affect scheduling should be disabled by default and should be
enabled in each and any usecase that requires it. At least this is my
opinion.

Best regards,
Petro


On Wed, Jan 5, 2022, 10:30 PM Gregory Nutt <sp...@gmail.com> wrote:

> More background:
>
> https://nuttx.yahoogroups.narkive.com/3hggphCi/problem-related-semaphore-and-priority-inheritance
>
> On Wed, Jan 5, 2022 at 2:22 PM Gregory Nutt <sp...@gmail.com> wrote:
>
> > > I truly believe that priority inheritance on counting semaphores should
> > be
> >
> > explicitly enabled (disabled by default) and enabling it by default might
> >
> > lead to unexpected priority boost for low priority tasks that violates
> >
> > real-time requirements.
> >
> >
> >
> > It has been enabled by default since day one so nothing will be broken.
> > Disabling it now will break things – that is an orthogonal discussion to
> > spec compliance.  Currently, priorioty inheritance is explicitly disabled
> > in all places where it should not be enabled (i.e., signaling semahores).
> >
> >
> >
> > If the default changes, then you would need to explicitly enable prority
> > inheritance on all semaphores that are used as locks in order to retain
> the
> > previous behavior.  See
> >
> https://cwiki.apache.org/confluence/display/NUTTX/Signaling+Semaphores+and+Priority+Inheritance
> > – the POSIX definition does not have this issue because POSIX does not
> > support priority inheritance on counting semaphores.
> >
> >
> >
>

Re: Change default behavior of semaphores?

[Gregory Nutt <sp...@gmail.com>]

More background:
https://nuttx.yahoogroups.narkive.com/3hggphCi/problem-related-semaphore-and-priority-inheritance

On Wed, Jan 5, 2022 at 2:22 PM Gregory Nutt <sp...@gmail.com> wrote:

> > I truly believe that priority inheritance on counting semaphores should
> be
>
> explicitly enabled (disabled by default) and enabling it by default might
>
> lead to unexpected priority boost for low priority tasks that violates
>
> real-time requirements.
>
>
>
> It has been enabled by default since day one so nothing will be broken.
> Disabling it now will break things – that is an orthogonal discussion to
> spec compliance.  Currently, priorioty inheritance is explicitly disabled
> in all places where it should not be enabled (i.e., signaling semahores).
>
>
>
> If the default changes, then you would need to explicitly enable prority
> inheritance on all semaphores that are used as locks in order to retain the
> previous behavior.  See
> https://cwiki.apache.org/confluence/display/NUTTX/Signaling+Semaphores+and+Priority+Inheritance
> – the POSIX definition does not have this issue because POSIX does not
> support priority inheritance on counting semaphores.
>
>
>

Re: Change default behavior of semaphores?

[Gregory Nutt <sp...@gmail.com>]

> I truly believe that priority inheritance on counting semaphores should be

explicitly enabled (disabled by default) and enabling it by default might

lead to unexpected priority boost for low priority tasks that violates

real-time requirements.



It has been enabled by default since day one so nothing will be broken.
Disabling it now will break things – that is an orthogonal discussion to
spec compliance.  Currently, priorioty inheritance is explicitly disabled
in all places where it should not be enabled (i.e., signaling semahores).



If the default changes, then you would need to explicitly enable prority
inheritance on all semaphores that are used as locks in order to retain the
previous behavior.  See
https://cwiki.apache.org/confluence/display/NUTTX/Signaling+Semaphores+and+Priority+Inheritance
– the POSIX definition does not have this issue because POSIX does not
support priority inheritance on counting semaphores.

Re: Change default behavior of semaphores?

[Petro Karashchenko <pe...@gmail.com>]

Going more forward from POSIX pthread mutex protocol description:

The protocol attribute defines the protocol to be followed in utilizing
mutexes. The value of protocol may be one of:
 - PTHREAD_PRIO_INHERIT
 - PTHREAD_PRIO_NONE
 - PTHREAD_PRIO_PROTECT
which are defined in the <pthread.h> header. The default value of the
attribute shall be PTHREAD_PRIO_NONE.

So it is reasonable to change default behavior for semaphores in order to
restore POSIX compliance for pthread mutexes as well.

Best regards,
Petro

On Wed, Jan 5, 2022, 8:17 PM Petro Karashchenko <
petro.karashchenko@gmail.com> wrote:

> Hi team,
>
> Sorry I probably missed some part of discussion. So it was decided to keep
> priority inheritance ON for counting semaphores by default if the feature
> is enabled? And we are modifying all the POSIX by adding explicit setting
> to SEM_PRIO_NONE?
>
> I truly believe that priority inheritance on counting semaphores should be
> explicitly enabled (disabled by default) and enabling it by default might
> lead to unexpected priority boost for low priority tasks that violates
> real-time requirements.
>
> The https://github.com/apache/incubator-nuttx/pull/5070 is definitely one
> of the possible solutions and we can continue with it or decide to go
> without TCB size increase and add pid into semholder struct +
> "(pholder->htcb == nxsched_get_tcb(pholder->pid))" check to ensure that
> htcb still points to original task that became a holder.
>
> Best regards,
> Petro
>
> On Wed, Jan 5, 2022, 1:54 PM Xiang Xiao <xi...@gmail.com> wrote:
>
>> Here are the two patch which demo that POSIX compliant code crash directly
>> when CONFIG_PRIORITY_INHERITANCE is enabled:
>> https://github.com/apache/incubator-nuttx/pull/5170
>> https://github.com/apache/incubator-nuttx-apps/pull/960
>> Please give your more feedback at
>> https://github.com/apache/incubator-nuttx/pull/5070.
>> If there is no more concern, we will start the full modification in the
>> next week.
>>
>> Thanks
>> Xiang
>>
>>
>>
>> On Sat, Dec 25, 2021 at 5:47 AM Gregory Nutt <sp...@gmail.com> wrote:
>>
>> > > Please help review this PR for correctness and standards compliance,
>> > > keeping in mind the Inviolable principles of NuttX [2], particularly
>> > > the section on POSIX compliance:
>> > >
>> >
>> > I don't think POSIX addresses priority inheritance on counting
>> semaphores.
>> > POSIX only addresses priority inheritance on pthread mutexes.  I need to
>> > double check that.
>> >
>> > Priority inheritance on counting semaphores is a natural extension and
>> > certainly required for a well-behaved embedded system.  But I don't
>> think
>> > there is a good spec-based argument, is there?
>> >
>>
>

Re: Change default behavior of semaphores?

[Petro Karashchenko <pe...@gmail.com>]

Hi team,

Sorry I probably missed some part of discussion. So it was decided to keep
priority inheritance ON for counting semaphores by default if the feature
is enabled? And we are modifying all the POSIX by adding explicit setting
to SEM_PRIO_NONE?

I truly believe that priority inheritance on counting semaphores should be
explicitly enabled (disabled by default) and enabling it by default might
lead to unexpected priority boost for low priority tasks that violates
real-time requirements.

The https://github.com/apache/incubator-nuttx/pull/5070 is definitely one
of the possible solutions and we can continue with it or decide to go
without TCB size increase and add pid into semholder struct +
"(pholder->htcb == nxsched_get_tcb(pholder->pid))" check to ensure that
htcb still points to original task that became a holder.

Best regards,
Petro

On Wed, Jan 5, 2022, 1:54 PM Xiang Xiao <xi...@gmail.com> wrote:

> Here are the two patch which demo that POSIX compliant code crash directly
> when CONFIG_PRIORITY_INHERITANCE is enabled:
> https://github.com/apache/incubator-nuttx/pull/5170
> https://github.com/apache/incubator-nuttx-apps/pull/960
> Please give your more feedback at
> https://github.com/apache/incubator-nuttx/pull/5070.
> If there is no more concern, we will start the full modification in the
> next week.
>
> Thanks
> Xiang
>
>
>
> On Sat, Dec 25, 2021 at 5:47 AM Gregory Nutt <sp...@gmail.com> wrote:
>
> > > Please help review this PR for correctness and standards compliance,
> > > keeping in mind the Inviolable principles of NuttX [2], particularly
> > > the section on POSIX compliance:
> > >
> >
> > I don't think POSIX addresses priority inheritance on counting
> semaphores.
> > POSIX only addresses priority inheritance on pthread mutexes.  I need to
> > double check that.
> >
> > Priority inheritance on counting semaphores is a natural extension and
> > certainly required for a well-behaved embedded system.  But I don't think
> > there is a good spec-based argument, is there?
> >
>

Re: Change default behavior of semaphores?

[Xiang Xiao <xi...@gmail.com>]

Here are the two patch which demo that POSIX compliant code crash directly
when CONFIG_PRIORITY_INHERITANCE is enabled:
https://github.com/apache/incubator-nuttx/pull/5170
https://github.com/apache/incubator-nuttx-apps/pull/960
Please give your more feedback at
https://github.com/apache/incubator-nuttx/pull/5070.
If there is no more concern, we will start the full modification in the
next week.

Thanks
Xiang



On Sat, Dec 25, 2021 at 5:47 AM Gregory Nutt <sp...@gmail.com> wrote:

> > Please help review this PR for correctness and standards compliance,
> > keeping in mind the Inviolable principles of NuttX [2], particularly
> > the section on POSIX compliance:
> >
>
> I don't think POSIX addresses priority inheritance on counting semaphores.
> POSIX only addresses priority inheritance on pthread mutexes.  I need to
> double check that.
>
> Priority inheritance on counting semaphores is a natural extension and
> certainly required for a well-behaved embedded system.  But I don't think
> there is a good spec-based argument, is there?
>

Re: Change default behavior of semaphores?

[Gregory Nutt <sp...@gmail.com>]

> Please help review this PR for correctness and standards compliance,
> keeping in mind the Inviolable principles of NuttX [2], particularly
> the section on POSIX compliance:
>

I don't think POSIX addresses priority inheritance on counting semaphores.
POSIX only addresses priority inheritance on pthread mutexes.  I need to
double check that.

Priority inheritance on counting semaphores is a natural extension and
certainly required for a well-behaved embedded system.  But I don't think
there is a good spec-based argument, is there?

Re: Change default behavior of semaphores?

[Gregory Nutt <sp...@gmail.com>]

If implemented, this will be a breaking change that affects downstream
> projects.
>

Some background for those interested:
https://cwiki.apache.org/confluence/display/NUTTX/Signaling+Semaphores+and+Priority+Inheritance

Make sure we keep documentation in sync whatever you all decide to do.

Nathan is right.. this will break every project running on NuttX.