Refactor criu plugin baseline v1 #2295
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Change made through this commit: - Include copy of flog as a seperate tree. - Modify the makefile to add and compile flog code. Signed-off-by: prakritigoyal19 <[email protected]>
CID 302713 (checkpoint-restore#1 of 1): Missing varargs init or cleanup (VARARGS) va_end was not called for argptr. Signed-off-by: Adrian Reber <[email protected]>
Separate commit for easier criu-dev <-> master transfer. Acked-by: Mike Rapoport <[email protected]> Signed-off-by: Adrian Reber <[email protected]>
It is mapped, not maped. Same applies for mmap I guess. Found by codespell, except it wants to change it to mapped, which will make it less specific. Signed-off-by: Kir Kolyshkin <[email protected]>
Brought to you by
codespell -w
(using codespell v2.1.0).
[v2: use "make indent" on the result]
Signed-off-by: Kir Kolyshkin <[email protected]>
Fixes: checkpoint-restore#2121 Signed-off-by: Pengda Yang <[email protected]>
The TOS(type of service) field in the ip header allows you specify the priority of the socket data. Signed-off-by: Suraj Shirvankar <[email protected]>
Signed-off-by: Suraj Shirvankar <[email protected]>
The pipe_size type is unsigned int, when the fcntl call fails and return -1, it will cause a negative rollover problem. Signed-off-by: zhoujie <[email protected]>
Newer Intel CPUs (Sapphire Rapids) have a much larger xsave area than
before. Looking at older CPUs I see 2440 bytes.
# cpuid -1 -l 0xd -s 0
...
bytes required by XSAVE/XRSTOR area = 0x00000988 (2440)
On newer CPUs (Sapphire Rapids) it grows to 11008 bytes.
# cpuid -1 -l 0xd -s 0
...
bytes required by XSAVE/XRSTOR area = 0x00002b00 (11008)
This increase the xsave area from one page to four pages.
Without this patch the fpu03 test fails, with this patch it works again.
Signed-off-by: Adrian Reber <[email protected]>
Signed-off-by: Adrian Reber <[email protected]>
Signed-off-by: Adrian Reber <[email protected]>
Using the fact that we know criu_pid and criu is a parent of restored process we can create pidfile with pid on caller pidns level. We need to move mount namespace creation to child so that criu-ns can see caller pidns proc. Signed-off-by: Pavel Tikhomirov <[email protected]>
By default, the file name 'amdgpu_plugin.txt' is used also as the name for the corresponding man page (`man amdgpu_plugin`). However, when this man page is installed system-wide it would be more appropriate to have a prefix 'criu-' (e.g., `man criu-amdgpu-plugin`). Signed-off-by: Radostin Stoyanov <[email protected]>
crun wants to set empty_ns and this interface is missing from the library. This adds it to libcriu. Signed-off-by: Adrian Reber <[email protected]>
--criu-binary argument provides a way to supply the CRIU binary location to run_criu(). Related to: checkpoint-restore#1909 Signed-off-by: Dhanuka Warusadura <[email protected]>
These changes remove and update the changes introduced in 7177938 in favor of the Python version in CI. os.waitstatus_to_exitcode() function appeared in Python 3.9 Related to: checkpoint-restore#1909 Signed-off-by: Dhanuka Warusadura <[email protected]>
These changes add test implementations for criu-ns script. Fixes: checkpoint-restore#1909 Signed-off-by: Dhanuka Warusadura <[email protected]>
These changes fix the `ImportError: No module named pathlib` error when executing criu-ns tests located at criu/test/others/criu-ns Signed-off-by: Dhanuka Warusadura <[email protected]>
CentOS 7 CI environment uses Python 2. To execute criu-ns script in CentOS 7 changing the current shebang line to python is required. This reverse the changes made in a15a63f Signed-off-by: Dhanuka Warusadura <[email protected]>
This is a patch proposed by Thomas here: https://lore.kernel.org/all/87ilczc7d9.ffs@tglx/ It removes (created id > desired id) "sanity" check and adds proper checking that ids start at zero and increment by one each time when we create/delete a posix timer. First purpose of it is to fix infinite looping in create_posix_timers on old pre 3.11 kernels. Second purpose is to allow kernel interface of creating posix timers with desired id change from iterating with predictable next id to just setting next id directly. And at the same time removing predictable next id so that criu with this patch would not get to infinite loop in create_posix_timers if this happens. Thanks a lot to Thomas! Signed-off-by: Pavel Tikhomirov <[email protected]>
This hook allows to start image streamer process from an action script. Signed-off-by: Radostin Stoyanov <[email protected]>
…tions This does cgroup namespace creation separately from joining task cgroups. This makes the code more logical, because creating cgroup namespace also involves joining cgroups but these cgroups can be different to task's cgroups as they are cgroup namespace roots (cgns_prefix), and mixing all of them together may lead to misunderstanding. Another positive thing is that we consolidate !item->parent checks in one place in restore_task_with_children. Signed-off-by: Valeriy Vdovin <[email protected]> Signed-off-by: Pavel Tikhomirov <[email protected]>
4.15-based kernels don't allow F_*SEAL for memfds created with MFD_HUGETLB. Since seals are not possible in this case, fake F_GETSEALS result as if it was queried for a non-sealing-enabled memfd. Signed-off-by: Michał Mirosław <[email protected]>
Linux 4.15 doesn't like empty string for cgroup2 mount options. Pass NULL then to satisfy the kernel check. Log the options for easier debugging. Signed-off-by: Michał Mirosław <[email protected]>
The original commit added saving THP_DISABLED flag value, but missed restoring it. There is restoring code, but used only when --lazy_pages mode is enabled. Restore the prctl flag always. While at it, rename the `has_thp_enabled` -> `!thp_disabled` for consistency. Fixes: bbbd597 (2017-06-28 "mem: add dump state of THP_DISABLED prctl") Signed-off-by: Michał Mirosław <[email protected]>
If prctl(SET_THP_DISABLE) is not used due to bad semantics, log it for easier debugging. Signed-off-by: Michał Mirosław <[email protected]>
While at it, don't carry over stale errno to the fail() message. Signed-off-by: Michał Mirosław <[email protected]>
Signed-off-by: Michał Mirosław <[email protected]>
Add a sanity check for THP_DISABLE. This discovered a broken commit in Google's kernel tree. Signed-off-by: Michał Mirosław <[email protected]>
The is_native field is a boolean. Therefore, else if() should can be
changed to a simple else{}.
Signed-off-by: Taemin Ha <[email protected]>
Signed-off-by: Andrei Vagin <[email protected]>
The condition meant to check fd2 instead of fd1, which is checked in line 24. Signed-off-by: Taemin Ha <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
line 131 checks if (ret >= 0). line 133 could be replaced by a simple else statement Signed-off-by: Taemin Ha <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
Eventpollentry's fields are set only when ret == 3 or ret == 6. The remaining cases can be grouped together to an error Signed-off-by: Taemin Ha <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
At this point the correct position is already restored, so reading from the fd results in the position being moved forward by 5 bytes. Fixes: 9191f87 ("criu/files-reg.c: add build-id validation functionality") Signed-off-by: Michal Clapinski <[email protected]>
Signed-off-by: Michal Clapinski <[email protected]>
Signed-off-by: Michal Clapinski <[email protected]>
cgroup_ifpriomap test needs net_prio cgroup, which might not be available. Make the .checkskip script check it. Signed-off-by: Michał Mirosław <[email protected]>
Newer versions of pip use an isolated virtual environment when building Python projects. However, when the source code of CRIT is copied into the isolated environment, the symlink for `../lib/py` (pycriu) becomes invalid. As a workaround, we used the `--no-build-isolation` option for `pip install`. However, this functionality has issues in some versions of PIP [1, 2]. To fix this problem, this patch adds separate packages for pycriu and crit, and each package is installed independently. [1] pypa/pip#8221 [2] pypa/pip#8165 (comment) Signed-off-by: Radostin Stoyanov <[email protected]>
Do not use $(USERCFLAGS) for anything other than what the user provide. Signed-off-by: Marcus Folkesson <[email protected]>
…vices Add a new compilation unit to host symbols and methods that will be needed to C&R DRM devices. Refactor code that indicates support for C&R and checkpoints KFD and DRM devices Signed-off-by: Ramesh Errabolu <[email protected]>
Refactor code used to Checkpoint DRM devices. Code is moved into amdgpu_plugin_drm.c file which hosts various methods to checkpoint and restore a workload. Signed-off-by: Ramesh Errabolu <[email protected]>
rst0git
reviewed
Nov 3, 2023
Fix the typo
|
Could someone advise me as to what is blocking? The 3 tests that are failing are doing so in "build" phase. It is not clear how that is connected to my change. What are the next steps |
You can just enter details section of each check and see errors by yourself. 1 and 3 are obviously introduced by your code, so you should fix them. 4 is definitely unrelated to your code as I saw it in other PRs, 2 is likely unrelated - I triggered a rerun for it. I also approved all checks for your PR, so you may expect some more fails. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## criu-dev #2295 +/- ##
============================================
+ Coverage 70.55% 70.62% +0.06%
============================================
Files 132 133 +1
Lines 33508 33312 -196
============================================
- Hits 23642 23525 -117
+ Misses 9866 9787 -79 ☔ View full report in Codecov by Sentry. |
rst0git
reviewed
Nov 10, 2023
| @@ -1244,7 +1244,7 @@ static bool map_devices(struct tp_system *src_sys, struct tp_system *dest_sys, s | |||
| return true; | |||
| } else { | |||
| /* We could not map remaining nodes in the list. Add dest node back | |||
| * to list and try to map next dest ndoe in list to current src | |||
| * to list and try to map next dest node in list to current src | |||
There was a problem hiding this comment.
Would you be able to apply this change in the previous commit (using git rebase or git commit --amend)?
https://github.com/checkpoint-restore/criu/blob/criu-dev/CONTRIBUTING.md#pull-request-guidelines
There was a problem hiding this comment.
Thanks for pointing out the procedure to look at the details.
I am rather new to submitting to repo on github that goes through an approval.
I was getting an error when I fixed the error (typo) locally and tried to push it again.
The git pull was messing up my local repo branch and thus the commit online on github itself
There was a problem hiding this comment.
After modifying the commit you might need to use git push with -f to update your github branch.
There was a problem hiding this comment.
Given my headaches, will push a new branch which will contain resolution to the various comments.
| return ret; | ||
| } | ||
|
|
||
| void print_kfd_bo_stat(int bo_cnt, struct kfd_criu_bo_bucket *bo_list) |
There was a problem hiding this comment.
This function is not used anywhere.
There was a problem hiding this comment.
I add this as a helper method that could be used in debugging if needed. Is this not allowed
There was a problem hiding this comment.
The problem with adding unused debugging functions upstream is that other people are not going to use them and test if they still work when new changes are being introduced. This makes it more difficult to maintain the code base and creates confusion for new contributors working on the project.
|
A friendly reminder that this PR had no activity for 30 days. |
dayatsin-amd
suggested changes
Dec 13, 2023
| { | ||
| dev_file_cnt--; | ||
| pr_info("\n"); | ||
| pr_info("Sairam: %s(), Number of Checkpoints LEFT: %d\n", __func__, dev_file_cnt); |
There was a problem hiding this comment.
Please remove all prints with Sairam:
| @@ -68,6 +68,13 @@ struct vma_metadata { | |||
|
|
|||
| extern int fd_next; | |||
|
|
|||
| // FD of KFD device used to checkpoint | |||
There was a problem hiding this comment.
Use consistent commenting style. I think rest of this file uses this style:
/**
* Comments
*/
|
|
||
| /* Helper macros to Checkpoint and Restore a ROCm file */ | ||
| #define HSAKMT_SHM_PATH "/dev/shm/hsakmt_shared_mem" | ||
| #define HSAKMT_SHM "/hsakmt_shared_mem" |
There was a problem hiding this comment.
mis-alignment for the right-hand-sides here
| return (dev_file_cnt == 0); | ||
| } | ||
|
|
||
| void decrement_checkpoint_count() |
There was a problem hiding this comment.
Please put this function in the other commit as it is used there
| @@ -840,6 +840,9 @@ void topology_free(struct tp_system *sys) | |||
| list_del(&p2pgroup->listm_system); | |||
| xfree(p2pgroup); | |||
| } | |||
|
|
|||
| /* Update Topology as being freed */ | |||
| sys->parsed = false; | |||
There was a problem hiding this comment.
I don't think parsed is used anywhere else?
|
Overall, the general idea/concept of this refactor is fine. |
|
A friendly reminder that this PR had no activity for 30 days. |
avagin
added
no-auto-close
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.