Re: [PATCH v2 0/3] drm/msm: Fix VM_BIND job, queue and entity lifetimes

[Jiwoo Kim]

On Sat, Nov 14, 2026 at 7:03 AM Rob Clark <rob.clark@xxxxxxxxxxxxxxxx> wrote:
> msm_gem_vm_close() waits on vm->last_fence which will be signaled
> after msm_vma_job_run() returns.

vm->last_fence only collects the fences of GEM_SUBMIT jobs, in
submit_attach_object_fences(). VM_BIND jobs don't add theirs, so
msm_gem_vm_close() waits for a VM_BIND job only if a submit depends
on it.

> My expectation is that if thread A is in midst of a VM_BIND ioctl,
> perhaps not having created/updated last_fence yet, while thread B is
> racing it closing the device fd, that file_operations::release will be
> called after thread A's ioctl call returns.  Is this not the case?

It is the case. (The vm_bind_closed check in patch 2 is only for
MSM_SUBMITQUEUE_CLOSE, which can race with a VM_BIND ioctl on the same
queue in another thread.)

The problem is a job that the ioctl has already pushed and that is
still waiting for its in-fence, e.g. one from another device or
drm_file, when the file is released:

- If the queue was closed with MSM_SUBMITQUEUE_CLOSE first,
  msm_submitqueue_close() no longer sees it, and nothing waits for the
  job. Otherwise drm_sched_entity_flush() waits at most
  MAX_WAIT_SCHED_ENTITY_Q_EMPTY for an exiting task, and kills the
  remaining jobs only on SIGKILL. Either way, the job's reference
  keeps the queue, and so the entity in it, alive.
- msm_gem_vm_close() then calls drm_sched_fini() on vm->sched, which
  frees the run queue that the entity still points to.
- When the in-fence signals, drm_sched_entity_wakeup(), which the
  scheduler registered on it, uses entity->rq->sched;
  drm_sched_entity_kill() leaves that callback in place. A killed job
  is freed then too, and the final put of the queue calls
  drm_sched_entity_destroy(), whose flush reads entity->rq->sched
  again.

Patch 2 destroys a VM_BIND queue's entity when the queue is closed,
by MSM_SUBMITQUEUE_CLOSE or on file close, so always before
msm_gem_vm_close() tears the scheduler down.

This comes from code review (see the note in the patches), and I have
not reproduced it.

v3 is at
https://lore.kernel.org/r/20261114141851.134290-1-arpe1618@xxxxxxxxx
and its patch 2 is unchanged from v2.

Separately, Sashiko's review of v3 found an ordering problem that
predates this series: the VM_BIND queues of a VM update the VM's VMAs
at ioctl time, but their pgtable updates are not ordered across
queues, so an unmap can reach the pgtables before the map it undoes.
That does not need a queue to be closed, but v3 patch 1 adds another
way to get there: the unmaps it applies when freeing a killed job are
not ordered against other queues either. (Its other point, that a
killed job's fence signals before those unmaps are applied, I'll
address in v4 as well.)

For v4 I would make a job that unmaps a VMA wait for the job that maps
it. The other way round, a map that overtakes an unmap of the same
range fails or gets undone, which breaks that VM but doesn't leave
freed pages mapped. Ordering all VM_BIND jobs of a VM would be
simpler, but turnip uses one VM_BIND queue per device plus one per
Vulkan queue. Would that be the right direction?

Thanks,
Jiwoo Kim