You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "jlaitine (via GitHub)" <gi...@apache.org> on 2023/01/30 07:08:45 UTC
[GitHub] [nuttx] jlaitine opened a new issue, #8346: Regression in many places due to change of nxsem_timedwait_uninterruptible with nxsem_tickwait_uninterruptible
jlaitine opened a new issue, #8346:
URL: https://github.com/apache/nuttx/issues/8346
It seems that several timeout functions have broken recently due to this commit:
commit 1fb8c13e5e80ee4cd5758e76821378162451c2fa
Author: Xiang Xiao <xi...@xiaomi.com>
Date: Tue May 10 09:32:31 2022 +0800
Replace nxsem_timedwait_uninterruptible with nxsem_tickwait_uninterruptible
Signed-off-by: Xiang Xiao <xi...@xiaomi.com>
For example: in net/utils/net_lock.c the change looks like this:
```
--- a/net/utils/net_lock.c
+++ b/net/utils/net_lock.c
@@ -97,27 +97,15 @@ _net_timedwait(sem_t *sem, bool interruptible, unsigned int timeout)
if (timeout != UINT_MAX)
{
- struct timespec abstime;
-
- DEBUGVERIFY(clock_gettime(CLOCK_REALTIME, &abstime));
-
- abstime.tv_sec += timeout / MSEC_PER_SEC;
- abstime.tv_nsec += timeout % MSEC_PER_SEC * NSEC_PER_MSEC;
- if (abstime.tv_nsec >= NSEC_PER_SEC)
- {
- abstime.tv_sec++;
- abstime.tv_nsec -= NSEC_PER_SEC;
- }
-
/* Wait until we get the lock or until the timeout expires */
if (interruptible)
{
- ret = nxsem_timedwait(sem, &abstime);
+ ret = nxsem_tickwait(sem, MSEC2TICK(timeout));
}
else
{
- ret = nxsem_timedwait_uninterruptible(sem, &abstime);
+ ret = nxsem_tickwait_uninterruptible(sem, MSEC2TICK(timeout));
}
}
```
In the original code, the _net_timedwait alwas went to sleep if the timeout is > 0. In the changed code, if MSEC2TICK(timeout) rounds to 0, the nxsem_tickwait doesn't sleep at all.
I noticed this problem when i2c driver broke for stm32f7 board, and fixed only this in PR #8303 . However, we are also experiencing some networking problems, which may be due to the code above
There are several alternatives to fix this problem:
1)
In all of the places be changed into rounding up the MSEC2TICK / USEC2TICK return value, for example "MSEC2TICK(timeout + (MSEC_PER_TICK / 2) - 1).
Pros:
- Doesn't break anything unknown
Cons:
- The rounding creeps in to almost everywhere where the macros are used
2)
We could change the functionality of MSEC2TIC/USEC2TICK to round up by default (actually some OS's do this, see e..g linux jiffy calculation).
Pros:
- Cleanest code, fixes all the places at once
Cons:
- May affect off-tree/private code of nuttx users
- The functionality of current MSEC2TIC/USEC2TICK has been as it is since 90's
3)
We could revert the change
Pros:
-Fixes the original problem
Cons:
- The original change greatly simplifies the code
- Would probably lead to rebase problems
--
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-unsubscribe@nuttx.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on issue #8346: Regression in many places due to change of nxsem_timedwait_uninterruptible with nxsem_tickwait_uninterruptible
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on issue #8346:
URL: https://github.com/apache/nuttx/issues/8346#issuecomment-1408119188
I think the best way is just to always just round up, not round to nearest as what is currently done.
The problem now happens always; e.g. if tick is 10ms and user wants to sleep 19ms, the code only sleeps 10ms. Previously it did 20ms. As this is mostly used as a timeout, it is very large error.
If user wants to sleep 9ms, the code doesn't sleep at all, which breaks more or less all the i2c drivers at least.
So if we want to change the *SEC2TICK macros, it would be something like:
```
#define NSEC2TICK(nsec) (((nsec)+(NSEC_PER_TICK/2))/NSEC_PER_TICK) /* Rounds */
#define USEC2TICK(usec) (((usec)+(USEC_PER_TICK/2))/USEC_PER_TICK) /* Rounds */
#if (MSEC_PER_TICK * USEC_PER_MSEC) == USEC_PER_TICK
# define MSEC2TICK(msec) (((msec)+(MSEC_PER_TICK/2))/MSEC_PER_TICK) /* Rounds */
#else
# define MSEC2TICK(msec) USEC2TICK((msec) * USEC_PER_MSEC) /* Rounds */
#endif
```
into
```
#define NSEC2TICK(nsec) (((nsec)+(NSEC_PER_TICK - 1))/NSEC_PER_TICK) /* Rounds */
#define USEC2TICK(usec) (((usec)+(USEC_PER_TICK - 1))/USEC_PER_TICK) /* Rounds */
#if (MSEC_PER_TICK * USEC_PER_MSEC) == USEC_PER_TICK
# define MSEC2TICK(msec) (((msec)+(MSEC_PER_TICK - 1))/MSEC_PER_TICK) /* Rounds */
#else
# define MSEC2TICK(msec) USEC2TICK((msec) * USEC_PER_MSEC) /* Rounds */
#endif
```
But I would like to see that before decision like this is done, there are more eyes looking into it
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on issue #8346: Regression in many places due to change of nxsem_timedwait_uninterruptible with nxsem_tickwait_uninterruptible
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on issue #8346:
URL: https://github.com/apache/nuttx/issues/8346#issuecomment-1408109463
This problem only happen when the sleep smaller than one tick?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 closed issue #8346: Regression in many places due to change of nxsem_timedwait_uninterruptible with nxsem_tickwait_uninterruptible
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 closed issue #8346: Regression in many places due to change of nxsem_timedwait_uninterruptible with nxsem_tickwait_uninterruptible
URL: https://github.com/apache/nuttx/issues/8346
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on issue #8346: Regression in many places due to change of nxsem_timedwait_uninterruptible with nxsem_tickwait_uninterruptible
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on issue #8346:
URL: https://github.com/apache/nuttx/issues/8346#issuecomment-1408121075
@davids5 : Related to this, I see that PX4 changed all the boards to have 1ms systick lately in the main branch. It causes a huge CPU load overhead, and I am not able to use that in some internal configurations of ours; maybe this issue is the reason why it was done?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on issue #8346: Regression in many places due to change of nxsem_timedwait_uninterruptible with nxsem_tickwait_uninterruptible
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on issue #8346:
URL: https://github.com/apache/nuttx/issues/8346#issuecomment-1409853979
I decided to make a PR #8371 of the proposed change
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on issue #8346: Regression in many places due to change of nxsem_timedwait_uninterruptible with nxsem_tickwait_uninterruptible
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on issue #8346:
URL: https://github.com/apache/nuttx/issues/8346#issuecomment-1408129351
Your proposal look reasonable, let's wait more feedback from other.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] davids5 commented on issue #8346: Regression in many places due to change of nxsem_timedwait_uninterruptible with nxsem_tickwait_uninterruptible
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on issue #8346:
URL: https://github.com/apache/nuttx/issues/8346#issuecomment-1408448749
> @davids5 : Related to this, I see that PX4 changed all the boards to have 1ms systick lately in the main branch. It causes a huge CPU load overhead, and I am not able to use that in some internal configurations of ours; maybe this issue is the reason why it was done? All the i2c drivers are basically broken if you have 10ms systick atm.
We have always run at 1 mS since day one. So it is un related.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on issue #8346: Regression in many places due to change of nxsem_timedwait_uninterruptible with nxsem_tickwait_uninterruptible
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on issue #8346:
URL: https://github.com/apache/nuttx/issues/8346#issuecomment-1408506072
> We have always run at 1 mS since day one. So it is un related.
That's right, sorry I confused something.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org