search

checkpoint-restore / criu

Checkpoint/Restore tool
criu.org
Other
2.79k stars 565 forks source link

dump: use MEMBARRIER_CMD_GET_REGISTRATIONS when available #2263

Closed mclapinski closed 10 months ago

mclapinski commented 10 months ago

MEMBARRIER_CMD_GET_REGISTRATIONS can tell us whether or not the process used MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED unlike the old probing method.

Falls back to the old method when MEMBARRIER_CMD_GET_REGISTRATIONS is unavailable.

avagin commented 10 months ago

can we add a test to check that MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED is C/R-ed?

mclapinski commented 10 months ago

I can't check it the same way we check other membarrier commands because invoking MEMBARRIER_CMD_GLOBAL_EXPEDITED without MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED is valid.

I could try to check that the feature itself works (the membarrier is executed) but I don't know of a good way to write such a test.

I could check it using MEMBARRIER_CMD_GET_REGISTRATIONS but it would be duplicating the criu code so I'm not sure if it's valuable. LMK.

avagin commented 10 months ago

I could check it using MEMBARRIER_CMD_GET_REGISTRATIONS but it would be duplicating the criu code so I'm not sure if it's valuable. LMK.

@mclapinski I think it is valuable, we need to check that the criu part works as expected.

codecov-commenter commented 10 months ago

Codecov Report

Patch coverage: 66.66% and project coverage change: -0.19% :warning:

Comparison is base (82bfb67) 70.67% compared to head (132871b) 70.48%.

:exclamation: Current head 132871b differs from pull request most recent head 6f131aa. Consider uploading reports for the commit 6f131aa to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## criu-dev #2263 +/- ## ============================================ - Coverage 70.67% 70.48% -0.19% ============================================ Files 133 132 -1 Lines 33330 33506 +176 ============================================ + Hits 23556 23618 +62 - Misses 9774 9888 +114 ``` | [Files Changed](https://app.codecov.io/gh/checkpoint-restore/criu/pull/2263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=checkpoint-restore) | Coverage Δ | | |---|---|---| | [criu/include/parasite.h](https://app.codecov.io/gh/checkpoint-restore/criu/pull/2263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=checkpoint-restore#diff-Y3JpdS9pbmNsdWRlL3BhcmFzaXRlLmg=) | `100.00% <ø> (ø)` | | | [criu/kerndat.c](https://app.codecov.io/gh/checkpoint-restore/criu/pull/2263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=checkpoint-restore#diff-Y3JpdS9rZXJuZGF0LmM=) | `57.83% <62.50%> (ø)` | | | [criu/parasite-syscall.c](https://app.codecov.io/gh/checkpoint-restore/criu/pull/2263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=checkpoint-restore#diff-Y3JpdS9wYXJhc2l0ZS1zeXNjYWxsLmM=) | `86.06% <100.00%> (+0.04%)` | :arrow_up: | ... and [20 files with indirect coverage changes](https://app.codecov.io/gh/checkpoint-restore/criu/pull/2263/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=checkpoint-restore)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mclapinski commented 10 months ago

@avagin Done. I think the CI failures are unrelated to my PR.

avagin commented 10 months ago

Merged. Thanks.