Refactor criu plugin baseline v1 #2295

rerrabolu · 2023-11-01T23:32:36Z

No description provided.

…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 <Ramesh.Errabolu@amd.com>

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 <Ramesh.Errabolu@amd.com>

plugins/amdgpu/amdgpu_plugin_topology.c

Fix the typo

rerrabolu · 2023-11-09T22:28:05Z

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

Snorch · 2023-11-10T02:38:17Z

# 1  DCO
Commit sha: [ff42a4f](https://github.com/checkpoint-restore/criu/pull/2295/commits/ff42a4f97f41849c45f5331dfa3c1b5840baa028), Author: Ramesh Errabolu, Committer: GitHub; The sign-off is missing.

# 2 CentOS Stream 8 based test
------------------------ grep Error ------------------------
b'(00.011876)     53: cg: setting cgns prefix to /system.slice/google-startup-scripts.service'
b'(00.011883)     53: cg: setting cgns prefix to /system.slice/google-startup-scripts.service'
b'(00.011890)     53: cg: setting cgns prefix to /system.slice/google-startup-scripts.service'
b'(00.011898)     53: cg: setting cgns prefix to /test'
b"(00.011905)     53: Error (criu/cgroup.c:1135): cg: Can't move 53 into zdtmtst//test/cgroup.procs (-1/-1): No such file or directory"
b"(00.011907)     53: Error (criu/cgroup.c:1191): cg: couldn't set cgns prefix zdtmtst//test/cgroup.procs: No such file or directory"
b'(00.011909)     53: Error (criu/cgroup.c:1282): cg: failed preparing cgns'
b'(00.012183) Error (criu/cr-restore.c:1513): 53 exited, status=1'
b'(00.012190) Error (criu/cr-restore.c:2557): Restoring FAILED.'
b'(00.016876) Error (criu/cgroup.c:1970): cg: cgroupd: recv req error: No such file or directory'
------------------------ ERROR OVER ------------------------
################ Test zdtm/static/cgroupns FAIL at CRIU restore ################

# 3 CentOS Stream 9 based test
gcc -g -Wall -Werror -D _GNU_SOURCE -shared -nostartfiles -fPIC -DCR_PLUGIN_DEFAULT="/usr/local/lib/criu" -I ../../compel/include/uapi amdgpu_plugin.c amdgpu_plugin_drm.c amdgpu_plugin_topology.c amdgpu_plugin_util.c criu-amdgpu.pb-c.c -o amdgpu_plugin.so -iquote../../include -iquote../../criu/include -iquote../../criu/arch/x86/include/ -iquote../../ -lpthread -lrt -ldrm -ldrm_amdgpu -I/usr/include/libdrm
/usr/bin/ld: /tmp/ccrGAnOx.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_util.c:52: multiple definition of `fd_next'; /tmp/cczVlqFR.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_topology.c:69: first defined here
/usr/bin/ld: /tmp/ccrGAnOx.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_util.c:54: multiple definition of `kfd_fw_version_check'; /tmp/cczVlqFR.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_topology.c:50: first defined here
/usr/bin/ld: /tmp/ccrGAnOx.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_util.c:55: multiple definition of `kfd_sdma_fw_version_check'; /tmp/cczVlqFR.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_topology.c:52: first defined here
/usr/bin/ld: /tmp/ccrGAnOx.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_util.c:56: multiple definition of `kfd_caches_count_check'; /tmp/cczVlqFR.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_topology.c:54: first defined here
/usr/bin/ld: /tmp/ccrGAnOx.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_util.c:57: multiple definition of `kfd_num_gws_check'; /tmp/cczVlqFR.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_topology.c:56: first defined here
/usr/bin/ld: /tmp/ccrGAnOx.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_util.c:58: multiple definition of `kfd_vram_size_check'; /tmp/cczVlqFR.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_topology.c:58: first defined here
/usr/bin/ld: /tmp/ccrGAnOx.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_util.c:59: multiple definition of `kfd_numa_check'; /tmp/cczVlqFR.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_topology.c:60: first defined here
/usr/bin/ld: /tmp/ccrGAnOx.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_util.c:60: multiple definition of `kfd_capability_check'; /tmp/cczVlqFR.o:/tmp/criu/plugins/amdgpu/amdgpu_plugin_topology.c:62: first defined here
collect2: error: ld returned 1 exit status
make[3]: *** [Makefile:32: amdgpu_plugin.so] Error 1
make[2]: *** [Makefile:347: amdgpu_plugin] Error 2
make[2]: Leaving directory '/tmp/criu'
make[1]: *** [Makefile:4: run] Error 2
make[1]: Leaving directory '/tmp/criu/test/others/make'
+ cleanup_cgroup
+ ./test/zdtm_umount_cgroups 12456
make: *** [Makefile:2: local] Error 2
make: Leaving directory '/tmp/criu/scripts/ci'

# 4 Vagrant Fedora Rawhide based test 
------------------------ grep Error ------------------------
b'(00.031043)     59: net: \tRunning ip rule delete table local'
b'(00.034514)     59: net: \tRunning ip rule restore'
b'(00.051113)     59: net: \tRunning iptables-restore -w for iptables-restore -w'
b'(00.055342)     59: net: \tRunning ip6tables-restore -w for ip6tables-restore -w'
b'(00.060395)     59: Error (criu/libnetlink.c:54): -16 reported by netlink: Device or resource busy'
b"(00.060445)     59: Error (criu/util.c:1495): Can't wait or bad status: errno=0, status=65280"
b'(00.060792) Error (criu/cr-restore.c:2557): Restoring FAILED.'
------------------------ ERROR OVER ------------------------
######### Test zdtm/static/socket-tcp-nfconntrack FAIL at CRIU restore #########

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-commenter · 2023-11-10T02:54:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c474816) 70.55% compared to head (3ad74b3) 70.62%.

❗ Current head 3ad74b3 differs from pull request most recent head ff42a4f. Consider uploading reports for the commit ff42a4f to get more accurate results

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

see 21 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

plugins/amdgpu/amdgpu_plugin_topology.c

plugins/amdgpu/amdgpu_plugin.c

plugins/amdgpu/criu-amdgpu.proto

plugins/amdgpu/amdgpu_plugin_util.h

rst0git · 2023-11-10T11:59:15Z

plugins/amdgpu/amdgpu_plugin_util.c

+	return ret;
+}
+
+void print_kfd_bo_stat(int bo_cnt, struct kfd_criu_bo_bucket *bo_list)


This function is not used anywhere.

I add this as a helper method that could be used in debugging if needed. Is this not allowed

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.

plugins/amdgpu/amdgpu_plugin_drm.c

plugins/amdgpu/amdgpu_plugin_util.c

github-actions · 2023-12-13T00:40:31Z

A friendly reminder that this PR had no activity for 30 days.

dayatsin-amd · 2023-12-13T03:51:51Z

plugins/amdgpu/amdgpu_plugin_util.c

+{
+	dev_file_cnt--;
+	pr_info("\n");
+	pr_info("Sairam: %s(),  Number of Checkpoints LEFT: %d\n", __func__, dev_file_cnt);


Please remove all prints with Sairam:

dayatsin-amd · 2023-12-13T03:56:06Z

plugins/amdgpu/amdgpu_plugin.c

@@ -68,6 +68,13 @@ struct vma_metadata {

 extern int fd_next;

+// FD of KFD device used to checkpoint


Use consistent commenting style. I think rest of this file uses this style:

/** * Comments */

dayatsin-amd · 2023-12-13T04:07:55Z

plugins/amdgpu/amdgpu_plugin_util.h

+
+/* Helper macros to Checkpoint and Restore a ROCm file */
+#define HSAKMT_SHM_PATH			"/dev/shm/hsakmt_shared_mem"
+#define HSAKMT_SHM				"/hsakmt_shared_mem"


mis-alignment for the right-hand-sides here

dayatsin-amd · 2023-12-13T04:14:04Z

plugins/amdgpu/amdgpu_plugin_util.c

+	return (dev_file_cnt == 0);
+}
+
+void decrement_checkpoint_count()


Please put this function in the other commit as it is used there

dayatsin-amd · 2023-12-13T04:16:14Z

plugins/amdgpu/amdgpu_plugin_topology.c

@@ -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;


I don't think parsed is used anywhere else?

dayatsin-amd · 2023-12-13T14:40:56Z

Overall, the general idea/concept of this refactor is fine.

github-actions · 2024-01-13T00:40:29Z

A friendly reminder that this PR had no activity for 30 days.

rerrabolu added 2 commits November 1, 2023 19:13

amdgpu_plugin: Refactor code used to implement Checkpoint

272b127

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 <Ramesh.Errabolu@amd.com>

rst0git reviewed Nov 3, 2023

View reviewed changes

plugins/amdgpu/amdgpu_plugin_topology.c Outdated Show resolved Hide resolved

Update amdgpu_plugin_topology.c

ff42a4f

Fix the typo

rst0git reviewed Nov 10, 2023

View reviewed changes

avagin requested review from dayatsin-amd and fdavid-amd November 12, 2023 03:11

github-actions bot added the stale-pr label Dec 13, 2023

dayatsin-amd suggested changes Dec 13, 2023

View reviewed changes

github-actions bot removed the stale-pr label Dec 14, 2023

github-actions bot added the stale-pr label Jan 13, 2024

avagin added no-auto-close Don't auto-close as a stale issue and removed stale-pr labels Jan 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor criu plugin baseline v1 #2295

Refactor criu plugin baseline v1 #2295

rerrabolu commented Nov 1, 2023

rerrabolu commented Nov 9, 2023

Snorch commented Nov 10, 2023

codecov-commenter commented Nov 10, 2023

rst0git Nov 10, 2023

rerrabolu Nov 10, 2023

rst0git Nov 11, 2023

github-actions bot commented Dec 13, 2023

dayatsin-amd Dec 13, 2023

dayatsin-amd Dec 13, 2023

dayatsin-amd Dec 13, 2023

dayatsin-amd Dec 13, 2023

dayatsin-amd Dec 13, 2023

dayatsin-amd commented Dec 13, 2023

github-actions bot commented Jan 13, 2024

		@@ -68,6 +68,13 @@ struct vma_metadata {

		extern int fd_next;

		// FD of KFD device used to checkpoint

Refactor criu plugin baseline v1 #2295

Are you sure you want to change the base?

Refactor criu plugin baseline v1 #2295

Conversation

rerrabolu commented Nov 1, 2023

rerrabolu commented Nov 9, 2023

Snorch commented Nov 10, 2023

codecov-commenter commented Nov 10, 2023

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dayatsin-amd commented Dec 13, 2023

github-actions bot commented Jan 13, 2024