Re: [PATCH 2/3] plugin/amdgpu: Add topology dump file
"Francis, David" <[email protected]> Fri, 17 Apr 2026 13:21:39 +0000
| Newsgroups | dev.linux.lists.criu |
|---|---|
| Message-ID | <SA1PR12MB8144AEFF3808F16D4FA60FDDEF202@SA1PR12MB8144.namprd12.prod.outlook.com> |
>>> Ah neat, but I am guessing the data comes from kfd? At some point we >>> will need to bite the bullet and start designing uapi for an amdgpu onl= y >>> world. >> >> No, the data comes solely from sysfs in this case. > > But exported by kfd, no? Ie. /sys/class/kfd/kfd/topology/nodes/. I > understand the goal is to migrate away from kfd which is why I wondered > if it makes sense to add an image based of both internal and external > kfd data. I think current plans are to deprecate the kfd device file first - /sys/class/kfd will continue to exist for a good while longer. > In one aspect it is better than my hack of allow restore from a single > to a single GPU system, although even there the gpu id is not stable, > but on the other hand it does add an image format which looks like a > dead end from a design persective. > > Hm, how does this patch handle gpu id changing across reboots? It does the whole amdgpu_plugin_topology matching thing to find a mapping between the old GPUs and the new ones. The renderD minor numbers of the new GPUs determine which files are opened to replace the open renderD handles. It doesn't really matter what the gpu ids are at that point. To target a specific GPU in amdgpu, you don't need gpu id, you just point the operation at the right renderD file. ________________________________________ From: Tvrtko Ursulin <[email protected]> Sent: Friday, April 17, 2026 5:07 AM To: Francis, David; [email protected] Subject: Re: [PATCH 2/3] plugin/amdgpu: Add topology dump file On 16/04/2026 19:26, Francis, David wrote: >> Ah neat, but I am guessing the data comes from kfd? At some point we >> will need to bite the bullet and start designing uapi for an amdgpu only >> world. > > No, the data comes solely from sysfs in this case. But exported by kfd, no? Ie. /sys/class/kfd/kfd/topology/nodes/. I understand the goal is to migrate away from kfd which is why I wondered if it makes sense to add an image based of both internal and external kfd data. > No urgency on merging this since it doesn't do much without more patches > but also unlikely to cause problems on its own - if kfd is present, the p= lugin > will continue to make the same dump it always did. > > Review would be appreciated, thanks. In one aspect it is better than my hack of allow restore from a single to a single GPU system, although even there the gpu id is not stable, but on the other hand it does add an image format which looks like a dead end from a design persective. Hm, how does this patch handle gpu id changing across reboots? Regards, Tvrtko > ________________________________________ > From: Tvrtko Ursulin <[email protected]> > Sent: Thursday, April 16, 2026 10:54 AM > To: Francis, David; [email protected] > Subject: Re: [PATCH 2/3] plugin/amdgpu: Add topology dump file > > > On 10/04/2026 15:45, David Francis wrote: >> The state of the source topology (the GPUs, CPUs, and links >> between them) is saved by the plugin as part of kfd dump. >> >> If there is no kfd dump, we need to save the topology anyways. >> >> Do so in new file amdgpu-topology.img. > > Ah neat, but I am guessing the data comes from kfd? At some point we > will need to bite the bullet and start designing uapi for an amdgpu only > world. > > In the meantime, would you like me to review this in detail to perhaps > have it merged in the interim? Although in that case backward > compatibility gets more complicated. > > Regards, > > Tvrtko > >> >> Signed-off-by: David Francis <[email protected]> >> --- >> plugins/amdgpu/amdgpu_plugin.c | 84 ++++++++++++++++++++++++++--= - >> plugins/amdgpu/amdgpu_plugin_drm.c | 64 ++++++++++++++++++++-- >> plugins/amdgpu/amdgpu_plugin_util.h | 9 ++++ >> plugins/amdgpu/criu-amdgpu.proto | 5 ++ >> 4 files changed, 151 insertions(+), 11 deletions(-) >> >> diff --git a/plugins/amdgpu/amdgpu_plugin.c b/plugins/amdgpu/amdgpu_plug= in.c >> index 89ab10dac..1e9785440 100644 >> --- a/plugins/amdgpu/amdgpu_plugin.c >> +++ b/plugins/amdgpu/amdgpu_plugin.c >> @@ -91,6 +91,9 @@ int current_pid; >> */ >> bool parallel_disabled =3D false; >> >> +bool kfd_dump_complete =3D false; >> +bool amdgpu_topology_dump_complete =3D false; >> + >> pthread_t parallel_thread =3D 0; >> int parallel_thread_result =3D 0; >> /********************************************************************= ******************************/ >> @@ -189,9 +192,14 @@ int topology_to_devinfo(struct tp_system *sys, stru= ct device_maps *maps, KfdDevi >> devinfo->node_id =3D node->id; >> >> if (NODE_IS_GPU(node)) { >> - devinfo->gpu_id =3D maps_get_dest_gpu(maps, node->= gpu_id); >> - if (!devinfo->gpu_id) >> - continue; >> + if (maps) { >> + devinfo->gpu_id =3D maps_get_dest_gpu(maps= , node->gpu_id); >> + if (!devinfo->gpu_id) >> + continue; >> + } else { >> + devinfo->gpu_id =3D node->gpu_id; >> + } >> + >> >> devinfo->simd_count =3D node->simd_count; >> devinfo->mem_banks_count =3D node->mem_banks_coun= t; >> @@ -238,9 +246,13 @@ int topology_to_devinfo(struct tp_system *sys, stru= ct device_maps *maps, KfdDevi >> if (!iolink->valid) >> continue; >> >> - list_for_each_entry(node2, &sys->nodes, li= stm_system) >> - if (node2->id =3D=3D iolink->node_= to_id && maps_get_dest_gpu(maps, node2->gpu_id) !=3D 0) >> - link_to_present_node =3D t= rue; >> + if (maps) { >> + list_for_each_entry(node2, &sys->n= odes, listm_system) >> + if (node2->id =3D=3D iolin= k->node_to_id && maps_get_dest_gpu(maps, node2->gpu_id) !=3D 0) >> + link_to_present_no= de =3D true; >> + } else { >> + link_to_present_node =3D true; >> + } >> >> if (!link_to_present_node) >> continue; >> @@ -386,6 +398,11 @@ int amdgpu_plugin_init(int stage) >> maps_init(&checkpoint_maps); >> maps_init(&restore_maps); >> >> + if (stage =3D=3D CR_PLUGIN_STAGE__DUMP) { >> + kfd_dump_complete =3D false; >> + amdgpu_topology_dump_complete =3D false; >> + } >> + >> if (stage =3D=3D CR_PLUGIN_STAGE__RESTORE) { >> if (has_children(root_item)) { >> pr_info("Parallel restore disabled\n"); >> @@ -1552,6 +1569,7 @@ int amdgpu_plugin_dump_file(int fd, int id) >> if (ret) >> goto exit; >> >> + kfd_dump_complete =3D true; >> if (!plugin_added_to_inventory) { >> ret =3D add_inventory_plugin(CR_PLUGIN_DESC.name); >> if (ret) { >> @@ -1908,6 +1926,60 @@ int amdgpu_plugin_restore_file(int id, bool *retr= y_needed) >> >> pr_info("render node gpu_id =3D 0x%04x\n", rd->gpu_id); >> >> + if (list_empty(&restore_maps.cpu_maps) && list_empty(&rest= ore_maps.gpu_maps)) { >> + AmdgpuDevinfo *ad; >> + >> + pr_info("No restore maps found, making them from t= opology file\n"); >> + >> + img_fp =3D open_img_file(IMG_AMDGPU_TOPOLOGY_FILE,= false, &img_size, true); >> + if (!img_fp) { >> + pr_err("Failed to find either kfd or amdgp= u src topology information\n"); >> + ret =3D -EINVAL; >> + goto exit; >> + } >> + >> + buf =3D xmalloc(img_size); >> + if (!buf) { >> + pr_err("Failed to allocate memory\n"); >> + return -ENOMEM; >> + } >> + >> + ret =3D read_fp(img_fp, buf, img_size); >> + if (ret) { >> + pr_err("Unable to read from %s\n", IMG_AMD= GPU_TOPOLOGY_FILE); >> + ret =3D -EINVAL; >> + goto exit; >> + } >> + >> + ad =3D amdgpu_devinfo__unpack(NULL, img_size, buf)= ; >> + if (rd =3D=3D NULL) { >> + pr_perror("Unable to parse the amdgpu topo= logy message\n"); >> + fclose(img_fp); >> + ret =3D -EINVAL; >> + goto exit; >> + } >> + fclose(img_fp); >> + >> + ret =3D devinfo_to_topology(ad->device_entries, ad= ->num_of_devices, &src_topology); >> + if (ret) { >> + pr_err("Failed to convert amdgpu device in= formation to topology\n"); >> + ret =3D -EINVAL; >> + goto exit; >> + } >> + >> + ret =3D topology_parse(&dest_topology, "Local"); >> + if (ret) { >> + pr_err("Failed to parse local system topol= ogy\n"); >> + goto exit; >> + } >> + >> + ret =3D set_restore_gpu_maps(&src_topology, &dest_= topology, &restore_maps); >> + if (ret) { >> + pr_err("Failed to map GPUs\n"); >> + goto exit; >> + } >> + } >> + >> target_gpu_id =3D maps_get_dest_gpu(&restore_maps, rd->gp= u_id); >> if (!target_gpu_id) { >> fd =3D -ENODEV; >> diff --git a/plugins/amdgpu/amdgpu_plugin_drm.c b/plugins/amdgpu/amdgpu_= plugin_drm.c >> index c1dfb2dd4..a4c650753 100644 >> --- a/plugins/amdgpu/amdgpu_plugin_drm.c >> +++ b/plugins/amdgpu/amdgpu_plugin_drm.c >> @@ -467,11 +467,65 @@ int amdgpu_plugin_drm_dump_file(int fd, int id, st= ruct stat *drm) >> return -ENODEV; >> } >> >> - /* Get the GPU_ID of the DRM device */ >> - rd->gpu_id =3D maps_get_dest_gpu(&checkpoint_maps, tp_node->gpu_id= ); >> - if (!rd->gpu_id) { >> - pr_err("Failed to find valid gpu_id for the device =3D %d\= n", rd->gpu_id); >> - return -ENODEV; >> + if (kfd_dump_complete) { >> + /* Get the GPU_ID of the DRM device */ >> + rd->gpu_id =3D maps_get_dest_gpu(&checkpoint_maps, tp_node= ->gpu_id); >> + if (!rd->gpu_id) { >> + pr_err("Failed to find valid gpu_id for the device= =3D %d\n", rd->gpu_id); >> + return -ENODEV; >> + } >> + } else { >> + rd->gpu_id =3D tp_node->gpu_id; >> + >> + if (!amdgpu_topology_dump_complete) { >> + AmdgpuDevinfo *ad =3D NULL; >> + unsigned char *buf; >> + >> + ad =3D xmalloc(sizeof(*ad)); >> + amdgpu_devinfo__init(ad); >> + >> + ad->num_of_devices =3D src_topology.num_nodes; >> + >> + ad->device_entries =3D xmalloc(sizeof(KfdDeviceEnt= ry *) * ad->num_of_devices); >> + if (!ad->device_entries) { >> + pr_err("Failed to allocate device_entries\= n"); >> + return -ENOMEM; >> + } >> + >> + for (int i =3D 0; i < ad->num_of_devices; i++) { >> + KfdDeviceEntry *entry =3D xzalloc(sizeof(*= entry)); >> + >> + if (!entry) { >> + pr_err("Failed to allocate entry\n= "); >> + return -ENOMEM; >> + } >> + >> + kfd_device_entry__init(entry); >> + >> + ad->device_entries[i] =3D entry; >> + ad->n_device_entries++; >> + } >> + >> + topology_to_devinfo(&src_topology, NULL, ad->devic= e_entries); >> + >> + len =3D amdgpu_devinfo__get_packed_size(ad); >> + >> + buf =3D xmalloc(len); >> + if (!buf) { >> + pr_perror("Failed to allocate memory to st= ore protobuf"); >> + return -ENOMEM; >> + } >> + >> + amdgpu_devinfo__pack(ad, buf); >> + >> + ret =3D write_img_file(IMG_AMDGPU_TOPOLOGY_FILE, b= uf, len); >> + if (ret) { >> + pr_err("Failed to write image file %s\n", = IMG_AMDGPU_TOPOLOGY_FILE); >> + return -EINVAL; >> + } >> + >> + amdgpu_topology_dump_complete =3D true; >> + } >> } >> >> len =3D criu_render_node__get_packed_size(rd); >> diff --git a/plugins/amdgpu/amdgpu_plugin_util.h b/plugins/amdgpu/amdgpu= _plugin_util.h >> index 69b98a31c..ccfe30b49 100644 >> --- a/plugins/amdgpu/amdgpu_plugin_util.h >> +++ b/plugins/amdgpu/amdgpu_plugin_util.h >> @@ -2,6 +2,7 @@ >> #define __AMDGPU_PLUGIN_UTIL_H__ >> >> #include <libdrm/amdgpu.h> >> +#include "criu-amdgpu.pb-c.h" >> >> #ifndef _GNU_SOURCE >> #define _GNU_SOURCE 1 >> @@ -59,6 +60,9 @@ >> /* Name of file having serialized data of DRM device buffer objects (= BOs) */ >> #define IMG_DRM_PAGES_FILE "amdgpu-drm-pages-%d-%d-%04x.img" >> >> +/* Name of file containing the source device topology (generated only i= f IMG_KFD_FILE is not)*/ >> +#define IMG_AMDGPU_TOPOLOGY_FILE "amdgpu-topology.img" >> + >> /* Helper macros to Checkpoint and Restore a ROCm file */ >> #define HSAKMT_SHM_PATH "/dev/shm/hsakmt_shared_m= em" >> #define HSAKMT_SHM "/hsakmt_shared_mem" >> @@ -115,6 +119,9 @@ extern bool kfd_vram_size_check; >> extern bool kfd_numa_check; >> extern bool kfd_capability_check; >> >> +extern bool kfd_dump_complete; >> +extern bool amdgpu_topology_dump_complete; >> + >> int read_fp(FILE *fp, void *buf, const size_t buf_len); >> int write_fp(FILE *fp, const void *buf, const size_t buf_len); >> int read_file(const char *file_path, void *buf, const size_t buf_len)= ; >> @@ -142,4 +149,6 @@ int sdma_copy_bo(int shared_fd, uint64_t size, FILE = *storage_fp, >> >> int serve_out_dmabuf_fd(int handle, int fd); >> >> +int topology_to_devinfo(struct tp_system *sys, struct device_maps *maps= , KfdDeviceEntry **deviceEntries); >> + >> #endif /* __AMDGPU_PLUGIN_UTIL_H__ */ >> diff --git a/plugins/amdgpu/criu-amdgpu.proto b/plugins/amdgpu/criu-amdg= pu.proto >> index 7682a8f21..6e44e22aa 100644 >> --- a/plugins/amdgpu/criu-amdgpu.proto >> +++ b/plugins/amdgpu/criu-amdgpu.proto >> @@ -93,3 +93,8 @@ message criu_render_node { >> message criu_dmabuf_node { >> required uint32 gem_handle =3D 1; >> } >> + >> +message amdgpu_devinfo { >> + required uint32 num_of_devices =3D 1; >> + repeated kfd_device_entry device_entries =3D 2; >> +} >> \ No newline at end of file >