Repository navigation
Let a device empty a copy somebody else still points at - #813
Conversation
a1680a4 to
fb994d8
Compare
| parsec_warning("GPU[%d:%s] asked to release its memory while it is still being managed." | ||
| " The copies may be in use, so nothing is flushed.", | ||
| device->device_index, device->name); | ||
| return PARSEC_ERR_DEVICE; |
There was a problem hiding this comment.
Fixed, in two halves.
parsec_device_memory_release() only runs from device fini, where a live manager means we are tearing the device down under a thread still using its copies. Nothing up that call chain can recover from that, and the caller is already committed to destroying the streams next, so it is now a parsec_fatal rather than a silent teardown.
Separately, parsec_devices_release_memory() now reports the refusal to its caller. It still asks every device, since whether one can give its memory back says nothing about the others, and returns PARSEC_ERR_DEVICE if any was busy. A refusal is non-destructive: parsec_device_flush_lru() returns before touching a list, so the caller can simply ask again once the work it is running has drained.
| #if !defined(PARSEC_GPU_ALLOC_PER_TILE) | ||
| if( (NULL != gpu_elem) && parsec_data_copy_is_placeholder(gpu_elem) ) { | ||
| /* This device emptied its copy of that data to make room for another | ||
| * one. Give it memory back and let the stage in refill it: it is | ||
| * already attached and already invalid, so memory is all it misses. |
There was a problem hiding this comment.
Right: the decision to leave a placeholder was compiled unconditionally while everything that makes one work sits behind !PARSEC_GPU_ALLOC_PER_TILE. In that mode the copy stayed attached to its old data and was then attached to the new one as well.
Rather than build a per-tile refill path, placeholders are now impossible there: allow_placeholders starts at -1, so the recoverability check is never consulted, keep_as_placeholder can never be set, and the eviction behaves exactly as it did before this PR. For what it is worth, that mode does not compile on master either: parsec_device_memory_release_list() calls memory_free() with one argument where the member takes two.
| if( (gpu_device->super.device_index == task->data[i].data_in->device_index) && | ||
| (gtask->flow_info[i].source == task->data[i].data_in) ) continue; |
There was a problem hiding this comment.
Good catch. The in-place branch of parsec_device_data_stage_in() returns without recording a source, and it was master's unconditional skip of device-local inputs that kept that field from ever being read.
It now assigns flow_info[].source to the copy itself, so source == data_in means "this flow has no transfer of its own to close out" and only the refilled placeholder falls through. Setting it beats a NULL check: the field is not reset between push attempts, so the value to guard against could be stale rather than NULL.
| int nb_transferred = task->locals[0].value; | ||
|
|
||
| parsec_thread_mempool_free(es->context_mempool, task); | ||
| PARSEC_OBJ_RELEASE(gpu_task); /* no need to call release_device_task, just release the task */ | ||
| gpu_device->data_avail_epoch++; | ||
| return 0; | ||
| return nb_transferred; |
There was a problem hiding this comment.
Fixed. The counter is incremented only in the branch that puts the copy back on the clean LRU, and it is named nb_reclaimable to match the parameter of parsec_device_wake_starved_tasks().
| parsec_list_t gpu_mem_starved; /**< Tasks that could not reserve the device memory | ||
| * they need. They remain counted in mutex, and | ||
| * only a write-back that turns dirty blocks into | ||
| * reclaimable ones hands them back to the manager. | ||
| */ |
There was a problem hiding this comment.
The naming is slightly confusing: gpu_mem_lru and gpu_mem_owned_lru are holding data copies while gpu_mem_starved holds tasks. Suggest renaming to starved (mirroring pending) or gpu_task_starved (more explicit).
There was a problem hiding this comment.
Renamed to starved_tasks. It sits between two lists of data copies, so a name that says "tasks" earns its place.
06810c1 to
4cda92b
Compare
The merge-base changed after approval.
A device that runs out of memory looks for a copy in its read LRU whose memory it can take. It passes over every copy whose reference count is above one, on the grounds that the extra reference belongs to a task completion that has yet to run and will drop it shortly. That is no longer the only thing such a reference can be. A copy handed to a successor is retained by the data repository that carries the dependency, and a reshape promise parked there retains both the copy it was given and the one it produced. Those references last as long as the dependency does, which can be until the end of the taskpool, and they are not waiting on anything this device can do. With little enough device memory, every copy in the LRU is referenced that way and the search comes back empty. The device has memory, all of it spoken for by copies nobody is reading, and the tasks waiting for a block wait for a release that is never coming. Let the search take those copies. The copy object has to survive, since the holders point at it, but its memory does not: emptied, it stays attached to its data, keeps the version its content had, and is marked invalid with nothing in flight. Anybody who needs the value again gives it memory back and stages it in from the host mirror, which is where the value went. Emptying a copy is only admissible when the value can be brought back, so examine the candidate first. A shared copy whose host mirror carries the same version can be emptied as it is. A copy holding no value at all is free to empty, which is what a reservation that had to be given up leaves behind. A copy the device owns, or one that is shared but ahead of the mirror, holds the only current value; move it to the owned list, where the write-back hands the value and the version to the host and returns it shared, which is precisely the state that makes it recoverable. Only start doing any of this once a full sweep has shown there is no copy left that this device owns outright. Three places assumed a copy always has memory behind it. A transfer looking for a source could pick an emptied peer, and hand the copy routine a source pointer of nil; it now passes those over, and a flow that also writes, which never looked for an alternate source at all, falls back on the host mirror. A source that is invalid was taken to mean a transfer is on its way in and is worth coming back for, which is not true of one that was emptied, so that now keys off the transfer being in flight. And the push completion skipped any flow whose input already sits on this device, on the grounds that no transfer was needed to bring it in; a copy being refilled is both the input and the output of its flow, so it was skipped and left under transfer for good, which is a state the eviction will never take memory back from.
4cda92b to
ab0f371
Compare
|
This has been approved before, but as a stacked PR I had to rebase once #812 got merged. |
When every copy on a device is still pointed at from outside it, the victim search used to find nothing and the task stayed starved. A device can now take the memory of such a copy and leave the copy object behind as a placeholder: it stays attached to its data, marked invalid, and whoever still holds it recovers the value from the host mirror on the next stage in.
Stacked on #812; only the last commit is new here.