There is no need to use the fd libdrm duplicated from the original fd.
Just use the original one.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Simplify a bunch of error handling blocks by freeing the vm_info_entries
array as soon as we are done accessing it.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Instead of initializing and de-initializing libdrm for every buffer object
we dump (each of which consists of a few system calls), we can simply
initialize it once and by doing so simplify a bunch of the error handling
blocks.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
When saving buffer object content the current code constantly allocates
and frees aligned memory where the GPU will copy each buffer one by one.
We can optimise this path by keeping the buffer around and only grow it if
is too small for the current object.
While doing this we also fix a theoretically incorrect freeing of memory
allocated via posix_memalign with xfree.
An alternative solution could be to probe for the maximum size before hand
by adding a pre-scan loop.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
amdgpu_plugin_drm_dump_file() already defines a large temporary stack
buffer for the purpose of generating the file level protobuf image name.
Instead of the buffer object dumping loop defining a separate temporary
stack buffer for the buffer content protobuf images we can simply use the
top level one.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
We can save a few lines of code and end up with a potentially more
readable function if we replace the open-coded try-retry of getting the
list of buffer objects with a loop.
While refactoring we also remove the needless copy of the entry as the
dump loop iterates the handles, and also stop returning a false success
with a made up zero buffer objects saved in cases when the kernel does not
support the DRM_IOCTL_AMDGPU_GEM_LIST_HANDLES ioctl.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
The DRM minor local variable is shadowed between two blocks inside
amdgpu_plugin_drm_dump_file(). If at the top level we access the minor via
the copy stored in the protobuf image, we can simply drop this copy and
so avoid the buffer object dump loop shadowing it.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
If the protobuf image allocation fails we can simply return immediately
since there aren't any other things to clean up at this point.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Commit
1961c5e0de ("plugins/amdgpu: Cleanup env variable handling")
converted some error logs into warnings under the rationale of failures
not being fatal, but the execution continues with the default values.
Convert one more such error to a warning, as pointed out by Copilot that
makes things more consistent.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Copilot reports a possible false positive error on strtoul conversion due
errno not being reset on success. Fix it by clearing errnor prior to the
call.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Copilot reports %d is correct for boolean and %zx for size_t, fix it.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: 1961c5e0de ("plugins/amdgpu: Cleanup env variable handling")
A bunch of globals defined at the top of amdgpu_plugin.c only needs local
scope so make it so.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
It is a bit tidier to make the helpers return the value instead of passing
in the pointer, so lets make them take the default and return the overall
result, allowing the call sites to consolidate into a single line.
While at it we make the helpers static since they are not used outside the
file and demote the parsing errors to warning log messages since the
plugin does continue operating.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Mostly after calling libdrm functions pr_perror is wrong because calling
convention there is to return zero or negative error code - errno is not
guaranteed to be valid. So convert those to pr_err with manually calling
strerror to log user friendly messages. Same conversion for handling
errors from some plugin internal functions.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com> # v1
Cc: Radostin Stoyanov <rstoyanov@fedoraproject.org>
---
v2:
* Handle newly added pr_perror after amdgpu_device_initialize().
* Add newlines.
v3:
* Add some newlines to pr_err.
There is no need to initialize and de-initialize libdrm only to use the
duplicated fd for calls to drmPrimeFDToHandle(). From the kernel's
perspective it is exactly the same file so operation is identical to just
using the passed in file descriptor.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Consolidate KFD and DRM VMA handling path to the same (existing) helper.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Only try to restore the dmabuf fd if the render node restore failed due
image not present. Other errors means the file is a render node (image is
present) so we can return an error straight away.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Currently the code contains multiple places where callers expect the errno
to be valid (implied by calling pr_perror to log file IO errors) after
calling open_img_file(), read_fp() and write_fp() helpers. Problem there
is that those helpers can destroy the errno courtesy of themselves
emitting log messages via fprintf.
Furthermore, the callers then sometimes invent the errors to return back
to the caller, or even outside of the plugin.
On top of that there is no benefit to buffered file IO given the plugin
reads and writes in buffer object size chunks, so it it preferrable to
just go direct and avoid any possibility of extra copying to and from libc
temporary buffers.
So lets just convert it all in one swoop to POSIX IO and make sure correct
errnos are always propagated to the caller (including to CRIU core).
Hopefully this removes all instances of incorrect pr_perror log messages.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com> # v1
---
v2:
* Handle short reads and writes.
* Add O_TRUNC.
v3:
* Handle EINTR and EAGAIN.
* Set errno on len == 0.
* Use %zu for size_t printing.
v4:
* Drop shadowed ret.
* Drop needless casts.
Use the recently added image load helper in one more function which
contains the exactly same flow of open-alloc-read.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Given that dump and restore of KFD vs DRM forks into completely parallel
flows inside the respective plugin callbacks, lets split it out by
extracting into helpers. This de-clutters the code flow and makes it
obvious which part is done where.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Currently the plugin is added to the inventory only if there were amdgpu
mapped vmas in the target process. This makes restoring a simple process
with only an open DRM fd and some buffer objects not work.
Fix it by adding the plugin to the inventory when there were open DRM
files as well.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Check SDMA operation type once at the beginning of the copy helper and fix
one incorrect usage of pr_perror while at it.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Open_drm_render_device() is a little bit confused. It can log confusing
errors using errno which hasn't been set, or it has been overwritten. It
can also inspect errno after it had reset it creating effectively
unreachable code. And finally it sometimes returns the negative errno, and
sometimes a plain -1. Fix it all up and make it consistent, while at the
same time correcting two calls site incorrectly using pr_perror(), while
the function does not guarantee a valid errno.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Error logging for just a single error condition will eat the errno so lets
just remove it. All callers use pr_perror() and rely on the helper not
munching it up so no information is lost by removing the internal log.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
We remove some function entry and exit markers, some interim messages
which provide no real value, and only log virtual address allocation
failures on the failure path. For the last one we also fix incorrect
usage of pr_perror.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
The extra debug build log helper does not seem very useful and some of
them do not even compile. Lets just remove it and replace with the
standard pr_debug. In case of too much noise, we can later re-evaluate
to remove some of the not very useful log messages.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Instead of open coding the same ioctl twice we can put it in a loop from
which we break out once we have allocated enough space for all objects.
While at it we add error handling for the memory allocation.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
---
v2:
* s/for(;;)/while(1)/ (David)
Both CRIU and plugin are built with 64-bit off_t and most of the parallel
bo restore path uses uint64_t for storing offsets.
Lets consolidate by changing int to off_t, and by doing so remove any
possibility that the call to fseeko can overflow the return type.
While at it, add error checking to both seek operations.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com> # v1
---
v2:
- Correct offset type as well.
Shallower indentation makes a heavily indented function hopefully a bit
more readable.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Consolidate to one fclose() call with the extra bonus the inconsequential
file descriptor leak is eliminated.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Close the directory when probing images during restore init fails.
While at it, consolidate the identical code between the DRM and KFD images
into a common helper.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Currently the failure to save content of a buffer object is only logged,
while the checkpointing will continue unaware due the error code being
"eaten" inside sdma_copy_bo() cleanup path, together with the caller not
checking the return value to begin with.
Fix it by preserving and returning the original error code inside
sdma_copy_bo(), and abort the checkpointing process in case of a failure.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Currently failure to open files logs an unitialized stack string as the
failed path. Lets just log the basename and drop the unused stack buffer.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Size needs to be initialized before opening the file for write, otherwise
garbage is written into the file.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
The new_minor field in struct vma_metadata is effectively unused.
Remove it.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Remove unused current_pid global variable.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Instead of logging a zero failure code lets log the actual GPU id which
was not found.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-By: David Francis <David.Francis@amd.com>
Check that the image unpacking worked before deferencing the returned
pointer during plugin init in order to fail gracefully.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Allocate shared memory for the whole struct shared_handle_ids and not just
for the pointer to it, and by doing so stop relying on mmap rounding up to
page size.
While at it add error checking that the allocations actually succeeded.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Adjusted object sizes need to be used to unmap on the cleanup path so that
the unmap operations match with map for the case sizes were adjusted in
the function prologue.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
---
v2:
* Improve commit message and use adjusted size in error logs too.
The function was not closing the drm fd duplicated by the libdrm call,
could attempt to deinitialize the libdrm device which wasn't successfully
openened, and was using errno based log functions when libdrm functions
used return a negative errno. Fix it all.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Close the file when either memory allocation or a file read fails in
amdgpu_plugin_dmabuf_restore.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Docstring refers to size_t as 8-byte value while in reality this is
architecture dependent. Clarify it.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
In amdgpu_plugin_drm_restore_file(), dmabuf_fd can reach the
drmPrimeFDToHandle() call as -1 if fdstore_get() fails or if
boinfo->handle is -1 while boinfo->is_import is true. Passing
an invalid fd to libdrm causes a hard restore failure instead
of a retry. Check for -1 and set retry_needed so the restore
coordinator can retry later.
When retry_needed is set, some dmabufs entries are still invalid,
so skip record_completed_work() and restore_bo_contents_drm()
to avoid operating on incomplete state.
Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
Several error paths in amdgpu_plugin_drm_dump_file() leak
resources. The drmPrimeHandleToFD() and open_img_file() failures
use break instead of goto exit, which skips per-iteration cleanup
and lets the function continue dumping metadata with a stale ret
value. Replace break with proper resource cleanup and goto exit.
The vm_info_entries buffer is not freed when the ioctl or
allocate_vm_entries() calls fail after allocating it, and the
buffer from posix_memalign() is never freed after sdma_copy_bo(),
leaking memory on every loop iteration. Add the missing xfree()
calls.
Check the amdgpu_device_initialize() return value because h_dev
is undefined on failure. Move list_handles_entries cleanup into
the exit block because error paths that goto exit skip the
existing xfree() call after the loop. Add a NULL check before
free_e(rd) because rd allocation can fail before the struct is
initialized.
Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
Initialize gpu_ids, fds_write, and entries to NULL before
allocating them in init_parallel_restore_cmd() and
init_parallel_restore_cmd_by_head(). The caller declares the
struct on the stack without an initializer, so these members
hold undefined values. If a later xzalloc() fails and the
caller invokes free_parallel_restore_cmd(), it will call
xfree() on those undefined pointers.
Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
Call amdgpu_device_deinitialize() before continuing when
get_gem_handle() fails and before returning when a matching
handle is found. Both paths skip the existing deinitialize
call at the end of the loop body, leaking the device handle.
Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
open_img_file() can return NULL after a read_fp() or write_fp()
failure without setting errno. The openat() and fdopen() paths
set errno, but the read/write path does not, so callers using
-errno get zero or a stale value. Set errno to EIO on this path
so errno is always valid when the function returns NULL.
Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
Close the file descriptor when fdopen() fails, as fdopen does not
take ownership of the fd on failure.
Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
