Improve `posix-futex` debugging #904

skuenzer · 2023-05-23T12:28:38Z

Base target

Architecture(s): [all]
Platform(s): [all]
Application(s): [N/A]

Additional configuration

CONFIG_LIBPOSIX_FUTEX=y
CONFIG_LIBPOSIX_FUTEX_DEBUG=y

Description of changes

This PR adds a set of debug messages to better understand the futex state, like threads getting into blocked state or waking up because of timeouts.

razvand · 2023-05-31T04:51:08Z

@skuenzer , could you please fix the checkpatch issues?

razvand · 2023-06-01T15:04:23Z

@skuenzer , could you please fix the checkpatch issues?

Apart from the checkpatch issues, everything works OK. After fixing the checkpatch issues and after @maniatro111 and @csvancea provide their reviews, I will do the final approval of the PR.

csvancea

Apart from the checkpatch and that minor nitpick -- LGTM

Reviewed-by: Cosmin Vancea csvancea@gmail.com

csvancea · 2023-06-01T15:41:42Z

lib/posix-futex/futex.c

+		uk_pr_debug("FUTEX_WAIT: Wait %"__PRIsnsec" nsec for wake-up\n",
+				(__snsec) (*timeout));
+		uk_thread_block_until(current, (__snsec) (*timeout));


Minor: maybe turn the expression (__snsec) (*timeout) into a variable since it is used multiple times?

With this patch, it is only used a second time in the debug message that I add. However, normally this message is not compiled in, so I find adding a variable that stores the dereferenced value of timeout for a single occurrence unnecessary.

flpostolache

@skuenzer Looks good to me. One small preference. Either add a space before and after the equal sign or remove them from both sides so that the result will be Condition = 2147483664 instead of Condition =2147483664.

skuenzer · 2023-06-07T12:23:06Z

@skuenzer , could you please fix the checkpatch issues?

As far as I can see, these are our standard warnings for long strings. Our checkpatch clone does not yet support detecting that we are having a print command with uk_pr_* that is why it is warning.

skuenzer · 2023-06-07T12:32:18Z

@skuenzer Looks good to me. One small preference. Either add a space before and after the equal sign or remove them from both sides so that the result will be Condition = 2147483664 instead of Condition =2147483664.

Right, sorry, I did not notice it. For sure I am fixing this.

flpostolache

Looks good now. All checkpatch issues are caused by uk_pr_* commands which are not ignored at the moment.

Reviewed-by: Florin Postolache florin.postolache.of@gmail.com

Introduces a configuration option to enable debug messages for `lib/posix-futex`. Signed-off-by: Simon Kuenzer <simon@unikraft.io>

Adds a set of debug messages to better understand the futex state, like threads getting into blocked state or waking up because of timeouts. Signed-off-by: Simon Kuenzer <simon@unikraft.io>

This commit removes a nesting if-statement to increase readability of `futex_wait()`. Signed-off-by: Simon Kuenzer <simon@unikraft.io>

unikraft-bot · 2023-07-05T01:01:49Z

⚠️ Checkpatch passed with warnings

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request but it encountered warnings:

SHA	commit	checkpatch
`a07c073`	lib/posix-futex: Debug config option	✅
`5a3df80`	lib/posix-futex: Increase debug output	⚠️
`9873c24`	lib/posix-futex: Improve `futex_wait()`	⚠️

Truncated logs starting from first warning 5a3df80:

WARNING:LONG_LINE_STRING: line over 80 characters
#39: FILE: lib/posix-futex/futex.c:106:
+		uk_pr_debug("FUTEX_WAIT: Condition =%"PRIu32" met (uaddr: %p)\n",

WARNING:LONG_LINE_STRING: line over 80 characters
#54: FILE: lib/posix-futex/futex.c:120:
+			uk_pr_debug("FUTEX_WAIT: Wait %"__PRIsnsec" nsec for wake-up\n",

WARNING:LONG_LINE_STRING: line over 80 characters
#82: FILE: lib/posix-futex/futex.c:155:
+		uk_pr_debug("FUTEX_WAIT: Condition =%"PRIu32" not met (uaddr: %p)\n",

total: 0 errors, 3 warnings, 63 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/tmp/build/a53d4c5b/patches/0002-lib-posix-futex-Increase-debug-output.patch has style problems, please review.

NOTE: Ignored message types: ASSIGN_IN_IF FILE_PATH_CHANGES NEW_TYPEDEFS OBSOLETE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

View complete logs | Learn more about Unikraft's coding style and contribution guidelines.

razvand

Approved-by: Razvan Deaconescu razvand@unikraft.io

Adds a set of debug messages to better understand the futex state, like threads getting into blocked state or waking up because of timeouts. Signed-off-by: Simon Kuenzer <simon@unikraft.io> Reviewed-by: Florin Postolache <florin.postolache.of@gmail.com> Approved-by: Razvan Deaconescu <razvand@unikraft.io> Tested-by: Unikraft CI <monkey@unikraft.io> GitHub-Closes: #904

This commit removes a nesting if-statement to increase readability of `futex_wait()`. Signed-off-by: Simon Kuenzer <simon@unikraft.io> Reviewed-by: Florin Postolache <florin.postolache.of@gmail.com> Approved-by: Razvan Deaconescu <razvand@unikraft.io> Tested-by: Unikraft CI <monkey@unikraft.io> GitHub-Closes: #904

skuenzer requested a review from a team as a code owner May 23, 2023 12:28

skuenzer requested review from flpostolache and razvand May 23, 2023 12:28

skuenzer assigned razvand May 23, 2023

skuenzer added this to the v0.14.0 (Prometheus) milestone May 23, 2023

razvand requested review from csvancea and removed request for a team May 23, 2023 13:06

razvand added area/lib Internal Unikraft Microlibrary topic/posix Topics pertaining to POSIX standard lang/c Issues or PRs to do with C/C++ topic/testing Topics on testing labels May 23, 2023

razvand removed their request for review May 31, 2023 04:50

csvancea approved these changes Jun 1, 2023

View reviewed changes

flpostolache reviewed Jun 4, 2023

View reviewed changes

skuenzer force-pushed the skuenzer/fix/futex-debugging branch from c81af62 to 2a27cdb Compare June 7, 2023 12:38

razvand requested a review from flpostolache June 28, 2023 12:53

flpostolache approved these changes Jun 28, 2023

View reviewed changes

skuenzer added 3 commits July 5, 2023 01:32

lib/posix-futex: Debug config option

a07c073

Introduces a configuration option to enable debug messages for `lib/posix-futex`. Signed-off-by: Simon Kuenzer <simon@unikraft.io>

lib/posix-futex: Increase debug output

5a3df80

Adds a set of debug messages to better understand the futex state, like threads getting into blocked state or waking up because of timeouts. Signed-off-by: Simon Kuenzer <simon@unikraft.io>

lib/posix-futex: Improve futex_wait()

9873c24

This commit removes a nesting if-statement to increase readability of `futex_wait()`. Signed-off-by: Simon Kuenzer <simon@unikraft.io>

skuenzer force-pushed the skuenzer/fix/futex-debugging branch from 2a27cdb to 9873c24 Compare July 4, 2023 23:32

razvand approved these changes Aug 9, 2023

View reviewed changes

unikraft-bot closed this in b8a755b Aug 9, 2023

unikraft-bot added the ci/merged Merged by CI label Aug 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `posix-futex` debugging #904

Improve `posix-futex` debugging #904

skuenzer commented May 23, 2023

razvand commented May 31, 2023

razvand commented Jun 1, 2023

csvancea left a comment •

edited

Loading

csvancea Jun 1, 2023

skuenzer Jun 7, 2023

flpostolache left a comment

skuenzer commented Jun 7, 2023

skuenzer commented Jun 7, 2023

flpostolache left a comment

unikraft-bot commented Jul 5, 2023

razvand left a comment

Improve posix-futex debugging #904

Improve posix-futex debugging #904

Conversation

skuenzer commented May 23, 2023

Base target

Additional configuration

Description of changes

razvand commented May 31, 2023

razvand commented Jun 1, 2023

csvancea left a comment • edited Loading

Choose a reason for hiding this comment

csvancea Jun 1, 2023

Choose a reason for hiding this comment

skuenzer Jun 7, 2023

Choose a reason for hiding this comment

flpostolache left a comment

Choose a reason for hiding this comment

skuenzer commented Jun 7, 2023

skuenzer commented Jun 7, 2023

flpostolache left a comment

Choose a reason for hiding this comment

unikraft-bot commented Jul 5, 2023

razvand left a comment

Choose a reason for hiding this comment

Improve `posix-futex` debugging #904

Improve `posix-futex` debugging #904

csvancea left a comment •

edited

Loading