-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
net: ipv6: Address and prefixes timeout immediately #22733
net: ipv6: Address and prefixes timeout immediately #22733
Conversation
After enabling CONFIG_NET_IF_LOG_LEVEL_DBG=y, I started to see constant stream of these messages. The system prints two msg every millisecond. <dbg> net_if.prefix_lifetime_timeout: Waiting for 2147483547 ms <dbg> net_if.address_lifetime_timeout: Waiting for 2147483547 ms Not sure what has changed in the system but refactoring the check fixes the issue. Fixes zephyrproject-rtos#22732 Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhedberg: Please consider as a useful bugix to fit into the release.
@@ -1266,7 +1266,7 @@ static void address_lifetime_timeout(struct k_work *work) | |||
is_timeout = address_manage_timeout(current, current_time, | |||
&next_timeout); | |||
if (!is_timeout) { | |||
if (next_timeout < timeout_update) { | |||
if ((s32_t)(next_timeout - timeout_update) < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for the first iteration you're subtracting UINT64_MAX
(the value of timeout_update
) from some value of next_timeout
(a u32_t
variable) and typecasting the result (which I think will implicitly be u64_t
) to s32_t
? I'd like some code comment here explaining why this is correct, since I'm at least struggling to understand this. What exactly was wrong with the original comparison? I'm not really a fan of merging stuff based on "this seems to work better" without understanding the underlying reason. Have you e.g. looked at the resulting assembly code? Should you perhaps have some exception here for the first iteration, and not bother with the subtraction if timeout_update == UINT64_MAX
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, indeed that looks fishy. If next_timeout is u32_t, then it doesn't seem to make sense to use u64_t for timeout_update, should be u32_t either. In worst case, should use s64_t to cast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fully agree that this is fishy as the original code should work as is. I did not investigate too deeply as the refactored code "fixed" the issue. No need to push this to 2.2 as we can look into this more deeply.
Did not see this issue any more, so whatever the root cause was it is no longer there. |
After enabling CONFIG_NET_IF_LOG_LEVEL_DBG=y, I started to see
constant stream of these messages. The system prints two msg
every millisecond.
net_if.prefix_lifetime_timeout: Waiting for 2147483547 ms
net_if.address_lifetime_timeout: Waiting for 2147483547 ms
Not sure what has changed in the system but refactoring the
check fixes the issue.
Fixes #22732
Signed-off-by: Jukka Rissanen jukka.rissanen@linux.intel.com