| Message ID | 20200707201229.472834-4-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | dma-fence annotations, round 3 | expand |
Hi, On Tue, 7 Jul 2020 at 21:13, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Comes up every few years, gets somewhat tedious to discuss, let's > write this down once and for all. Thanks for writing this up! I wonder if any of the notes from my reply to the previous-version thread would be helpful to more explicitly encode the carrot of dma-fence's positive guarantees, rather than just the stick of 'don't do this'. ;) Either way, this is: Acked-by: Daniel Stone <daniels@collabora.com> > What I'm not sure about is whether the text should be more explicit in > flat out mandating the amdkfd eviction fences for long running compute > workloads or workloads where userspace fencing is allowed. ... or whether we just say that you can never use dma-fence in conjunction with userptr. Cheers, Daniel
On Thu, Jul 09, 2020 at 08:36:43AM +0100, Daniel Stone wrote: > Hi, > > On Tue, 7 Jul 2020 at 21:13, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > Comes up every few years, gets somewhat tedious to discuss, let's > > write this down once and for all. > > Thanks for writing this up! I wonder if any of the notes from my reply > to the previous-version thread would be helpful to more explicitly > encode the carrot of dma-fence's positive guarantees, rather than just > the stick of 'don't do this'. ;) Either way, this is: I think the carrot should go into the intro section for dma-fence, this section here is very much just the "don't do this" part. The previous patches have an attempt at encoding this a bit, maybe see whether there's a place for your reply (or parts of it) to fit? > Acked-by: Daniel Stone <daniels@collabora.com> > > > What I'm not sure about is whether the text should be more explicit in > > flat out mandating the amdkfd eviction fences for long running compute > > workloads or workloads where userspace fencing is allowed. > > ... or whether we just say that you can never use dma-fence in > conjunction with userptr. Uh userptr is entirely different thing. That one is ok. It's userpsace fences or gpu futexes or future fences or whatever we want to call them. Or is there some other confusion here?. -Daniel > > Cheers, > Daniel
Am 07.07.20 um 22:12 schrieb Daniel Vetter: > Comes up every few years, gets somewhat tedious to discuss, let's > write this down once and for all. > > What I'm not sure about is whether the text should be more explicit in > flat out mandating the amdkfd eviction fences for long running compute > workloads or workloads where userspace fencing is allowed. > > v2: Now with dot graph! > > Cc: Jesse Natalie <jenatali@microsoft.com> > Cc: Steve Pronovost <spronovo@microsoft.com> > Cc: Jason Ekstrand <jason@jlekstrand.net> > Cc: Felix Kuehling <Felix.Kuehling@amd.com> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Cc: Thomas Hellstrom <thomas.hellstrom@intel.com> > Cc: linux-media@vger.kernel.org > Cc: linaro-mm-sig@lists.linaro.org > Cc: linux-rdma@vger.kernel.org > Cc: amd-gfx@lists.freedesktop.org > Cc: intel-gfx@lists.freedesktop.org > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Christian König <christian.koenig@amd.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Acked-by: Christian König <christian.koenig@amd.com> > --- > Documentation/driver-api/dma-buf.rst | 70 ++++++++++++++++++++++++ > drivers/gpu/drm/virtio/virtgpu_display.c | 20 ------- > 2 files changed, 70 insertions(+), 20 deletions(-) > > diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst > index f8f6decde359..037ba0078bb4 100644 > --- a/Documentation/driver-api/dma-buf.rst > +++ b/Documentation/driver-api/dma-buf.rst > @@ -178,3 +178,73 @@ DMA Fence uABI/Sync File > .. kernel-doc:: include/linux/sync_file.h > :internal: > > +Idefinite DMA Fences > +~~~~~~~~~~~~~~~~~~~~ > + > +At various times &dma_fence with an indefinite time until dma_fence_wait() > +finishes have been proposed. Examples include: > + > +* Future fences, used in HWC1 to signal when a buffer isn't used by the display > + any longer, and created with the screen update that makes the buffer visible. > + The time this fence completes is entirely under userspace's control. > + > +* Proxy fences, proposed to handle &drm_syncobj for which the fence has not yet > + been set. Used to asynchronously delay command submission. > + > +* Userspace fences or gpu futexes, fine-grained locking within a command buffer > + that userspace uses for synchronization across engines or with the CPU, which > + are then imported as a DMA fence for integration into existing winsys > + protocols. > + > +* Long-running compute command buffers, while still using traditional end of > + batch DMA fences for memory management instead of context preemption DMA > + fences which get reattached when the compute job is rescheduled. > + > +Common to all these schemes is that userspace controls the dependencies of these > +fences and controls when they fire. Mixing indefinite fences with normal > +in-kernel DMA fences does not work, even when a fallback timeout is included to > +protect against malicious userspace: > + > +* Only the kernel knows about all DMA fence dependencies, userspace is not aware > + of dependencies injected due to memory management or scheduler decisions. > + > +* Only userspace knows about all dependencies in indefinite fences and when > + exactly they will complete, the kernel has no visibility. > + > +Furthermore the kernel has to be able to hold up userspace command submission > +for memory management needs, which means we must support indefinite fences being > +dependent upon DMA fences. If the kernel also support indefinite fences in the > +kernel like a DMA fence, like any of the above proposal would, there is the > +potential for deadlocks. > + > +.. kernel-render:: DOT > + :alt: Indefinite Fencing Dependency Cycle > + :caption: Indefinite Fencing Dependency Cycle > + > + digraph "Fencing Cycle" { > + node [shape=box bgcolor=grey style=filled] > + kernel [label="Kernel DMA Fences"] > + userspace [label="userspace controlled fences"] > + kernel -> userspace [label="memory management"] > + userspace -> kernel [label="Future fence, fence proxy, ..."] > + > + { rank=same; kernel userspace } > + } > + > +This means that the kernel might accidentally create deadlocks > +through memory management dependencies which userspace is unaware of, which > +randomly hangs workloads until the timeout kicks in. Workloads, which from > +userspace's perspective, do not contain a deadlock. In such a mixed fencing > +architecture there is no single entity with knowledge of all dependencies. > +Thefore preventing such deadlocks from within the kernel is not possible. > + > +The only solution to avoid dependencies loops is by not allowing indefinite > +fences in the kernel. This means: > + > +* No future fences, proxy fences or userspace fences imported as DMA fences, > + with or without a timeout. > + > +* No DMA fences that signal end of batchbuffer for command submission where > + userspace is allowed to use userspace fencing or long running compute > + workloads. This also means no implicit fencing for shared buffers in these > + cases. > diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c > index f3ce49c5a34c..af55b334be2f 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_display.c > +++ b/drivers/gpu/drm/virtio/virtgpu_display.c > @@ -314,25 +314,6 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev, > return &virtio_gpu_fb->base; > } > > -static void vgdev_atomic_commit_tail(struct drm_atomic_state *state) > -{ > - struct drm_device *dev = state->dev; > - > - drm_atomic_helper_commit_modeset_disables(dev, state); > - drm_atomic_helper_commit_modeset_enables(dev, state); > - drm_atomic_helper_commit_planes(dev, state, 0); > - > - drm_atomic_helper_fake_vblank(state); > - drm_atomic_helper_commit_hw_done(state); > - > - drm_atomic_helper_wait_for_vblanks(dev, state); > - drm_atomic_helper_cleanup_planes(dev, state); > -} > - > -static const struct drm_mode_config_helper_funcs virtio_mode_config_helpers = { > - .atomic_commit_tail = vgdev_atomic_commit_tail, > -}; > - > static const struct drm_mode_config_funcs virtio_gpu_mode_funcs = { > .fb_create = virtio_gpu_user_framebuffer_create, > .atomic_check = drm_atomic_helper_check, > @@ -346,7 +327,6 @@ void virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev) > drm_mode_config_init(vgdev->ddev); > vgdev->ddev->mode_config.quirk_addfb_prefer_host_byte_order = true; > vgdev->ddev->mode_config.funcs = &virtio_gpu_mode_funcs; > - vgdev->ddev->mode_config.helper_private = &virtio_mode_config_helpers; > > /* modes will be validated against the framebuffer size */ > vgdev->ddev->mode_config.min_width = XRES_MIN;
On Thu, 9 Jul 2020 at 09:05, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Jul 09, 2020 at 08:36:43AM +0100, Daniel Stone wrote: > > On Tue, 7 Jul 2020 at 21:13, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > Comes up every few years, gets somewhat tedious to discuss, let's > > > write this down once and for all. > > > > Thanks for writing this up! I wonder if any of the notes from my reply > > to the previous-version thread would be helpful to more explicitly > > encode the carrot of dma-fence's positive guarantees, rather than just > > the stick of 'don't do this'. ;) Either way, this is: > > I think the carrot should go into the intro section for dma-fence, this > section here is very much just the "don't do this" part. The previous > patches have an attempt at encoding this a bit, maybe see whether there's > a place for your reply (or parts of it) to fit? Sounds good to me. > > Acked-by: Daniel Stone <daniels@collabora.com> > > > > > What I'm not sure about is whether the text should be more explicit in > > > flat out mandating the amdkfd eviction fences for long running compute > > > workloads or workloads where userspace fencing is allowed. > > > > ... or whether we just say that you can never use dma-fence in > > conjunction with userptr. > > Uh userptr is entirely different thing. That one is ok. It's userpsace > fences or gpu futexes or future fences or whatever we want to call them. > Or is there some other confusion here?. I mean generating a dma_fence from a batch which will try to page in userptr. Given that userptr could be backed by absolutely anything at all, it doesn't seem smart to allow fences to rely on a pointer to an mmap'ed NFS file. So it seems like batches should be mutually exclusive between arbitrary SVM userptr and generating a dma-fence? Speaking of entirely different things ... the virtio-gpu bit really doesn't belong in this patch. Cheers, Daniel
On Thu, Jul 9, 2020 at 2:11 PM Daniel Stone <daniel@fooishbar.org> wrote: > > On Thu, 9 Jul 2020 at 09:05, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, Jul 09, 2020 at 08:36:43AM +0100, Daniel Stone wrote: > > > On Tue, 7 Jul 2020 at 21:13, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > Comes up every few years, gets somewhat tedious to discuss, let's > > > > write this down once and for all. > > > > > > Thanks for writing this up! I wonder if any of the notes from my reply > > > to the previous-version thread would be helpful to more explicitly > > > encode the carrot of dma-fence's positive guarantees, rather than just > > > the stick of 'don't do this'. ;) Either way, this is: > > > > I think the carrot should go into the intro section for dma-fence, this > > section here is very much just the "don't do this" part. The previous > > patches have an attempt at encoding this a bit, maybe see whether there's > > a place for your reply (or parts of it) to fit? > > Sounds good to me. > > > > Acked-by: Daniel Stone <daniels@collabora.com> > > > > > > > What I'm not sure about is whether the text should be more explicit in > > > > flat out mandating the amdkfd eviction fences for long running compute > > > > workloads or workloads where userspace fencing is allowed. > > > > > > ... or whether we just say that you can never use dma-fence in > > > conjunction with userptr. > > > > Uh userptr is entirely different thing. That one is ok. It's userpsace > > fences or gpu futexes or future fences or whatever we want to call them. > > Or is there some other confusion here?. > > I mean generating a dma_fence from a batch which will try to page in > userptr. Given that userptr could be backed by absolutely anything at > all, it doesn't seem smart to allow fences to rely on a pointer to an > mmap'ed NFS file. So it seems like batches should be mutually > exclusive between arbitrary SVM userptr and generating a dma-fence? Locking is Tricky (tm) but essentially what at least amdgpu does is pull in the backing storage before we publish any dma-fence. And then some serious locking magic to make sure that doesn't race with a core mm invalidation event. So for your case here the cs ioctl just blocks until the nfs pages are pulled in. Once we've committed for the dma-fence it's only the other way round, i.e. core mm will stall on the dma-fence if it wants to throw out these pages again. More or less at least. That way we never have a dma-fence depending upon any core mm operations. The only pain here is that this severely limits what you can do in the critical path towards signalling a dma-fence, because the tldr is "no interacting with core mm at all allowed". > Speaking of entirely different things ... the virtio-gpu bit really > doesn't belong in this patch. Oops, dunno where I lost that as a sparate patch. Will split out again :-( -Daniel > > Cheers, > Daniel
Am 09.07.20 um 14:31 schrieb Daniel Vetter: > On Thu, Jul 9, 2020 at 2:11 PM Daniel Stone <daniel@fooishbar.org> wrote: >> On Thu, 9 Jul 2020 at 09:05, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Thu, Jul 09, 2020 at 08:36:43AM +0100, Daniel Stone wrote: >>>> On Tue, 7 Jul 2020 at 21:13, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >>>>> Comes up every few years, gets somewhat tedious to discuss, let's >>>>> write this down once and for all. >>>> Thanks for writing this up! I wonder if any of the notes from my reply >>>> to the previous-version thread would be helpful to more explicitly >>>> encode the carrot of dma-fence's positive guarantees, rather than just >>>> the stick of 'don't do this'. ;) Either way, this is: >>> I think the carrot should go into the intro section for dma-fence, this >>> section here is very much just the "don't do this" part. The previous >>> patches have an attempt at encoding this a bit, maybe see whether there's >>> a place for your reply (or parts of it) to fit? >> Sounds good to me. >> >>>> Acked-by: Daniel Stone <daniels@collabora.com> >>>> >>>>> What I'm not sure about is whether the text should be more explicit in >>>>> flat out mandating the amdkfd eviction fences for long running compute >>>>> workloads or workloads where userspace fencing is allowed. >>>> ... or whether we just say that you can never use dma-fence in >>>> conjunction with userptr. >>> Uh userptr is entirely different thing. That one is ok. It's userpsace >>> fences or gpu futexes or future fences or whatever we want to call them. >>> Or is there some other confusion here?. >> I mean generating a dma_fence from a batch which will try to page in >> userptr. Given that userptr could be backed by absolutely anything at >> all, it doesn't seem smart to allow fences to rely on a pointer to an >> mmap'ed NFS file. So it seems like batches should be mutually >> exclusive between arbitrary SVM userptr and generating a dma-fence? > Locking is Tricky (tm) but essentially what at least amdgpu does is > pull in the backing storage before we publish any dma-fence. And then > some serious locking magic to make sure that doesn't race with a core > mm invalidation event. So for your case here the cs ioctl just blocks > until the nfs pages are pulled in. Yeah, we had some iterations until all was settled. Basic idea is the following: 1. Have a sequence counter increased whenever a change to the page tables happens. 2. During CS grab the current value of this counter. 3. Get all the pages you need in an array. 4. Prepare CS, grab the low level lock the MM notifier waits for and double check the counter. 5. If the counter is still the same all is well and the DMA-fence pushed to the hardware. 6. If the counter has changed repeat. Can result in a nice live lock when you constantly page things in/out, but that is expected behavior. Christian. > > Once we've committed for the dma-fence it's only the other way round, > i.e. core mm will stall on the dma-fence if it wants to throw out > these pages again. More or less at least. That way we never have a > dma-fence depending upon any core mm operations. The only pain here is > that this severely limits what you can do in the critical path towards > signalling a dma-fence, because the tldr is "no interacting with core > mm at all allowed". > >> Speaking of entirely different things ... the virtio-gpu bit really >> doesn't belong in this patch. > Oops, dunno where I lost that as a sparate patch. Will split out again :-( > -Daniel > >> Cheers, >> Daniel > >
diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst index f8f6decde359..037ba0078bb4 100644 --- a/Documentation/driver-api/dma-buf.rst +++ b/Documentation/driver-api/dma-buf.rst @@ -178,3 +178,73 @@ DMA Fence uABI/Sync File .. kernel-doc:: include/linux/sync_file.h :internal: +Idefinite DMA Fences +~~~~~~~~~~~~~~~~~~~~ + +At various times &dma_fence with an indefinite time until dma_fence_wait() +finishes have been proposed. Examples include: + +* Future fences, used in HWC1 to signal when a buffer isn't used by the display + any longer, and created with the screen update that makes the buffer visible. + The time this fence completes is entirely under userspace's control. + +* Proxy fences, proposed to handle &drm_syncobj for which the fence has not yet + been set. Used to asynchronously delay command submission. + +* Userspace fences or gpu futexes, fine-grained locking within a command buffer + that userspace uses for synchronization across engines or with the CPU, which + are then imported as a DMA fence for integration into existing winsys + protocols. + +* Long-running compute command buffers, while still using traditional end of + batch DMA fences for memory management instead of context preemption DMA + fences which get reattached when the compute job is rescheduled. + +Common to all these schemes is that userspace controls the dependencies of these +fences and controls when they fire. Mixing indefinite fences with normal +in-kernel DMA fences does not work, even when a fallback timeout is included to +protect against malicious userspace: + +* Only the kernel knows about all DMA fence dependencies, userspace is not aware + of dependencies injected due to memory management or scheduler decisions. + +* Only userspace knows about all dependencies in indefinite fences and when + exactly they will complete, the kernel has no visibility. + +Furthermore the kernel has to be able to hold up userspace command submission +for memory management needs, which means we must support indefinite fences being +dependent upon DMA fences. If the kernel also support indefinite fences in the +kernel like a DMA fence, like any of the above proposal would, there is the +potential for deadlocks. + +.. kernel-render:: DOT + :alt: Indefinite Fencing Dependency Cycle + :caption: Indefinite Fencing Dependency Cycle + + digraph "Fencing Cycle" { + node [shape=box bgcolor=grey style=filled] + kernel [label="Kernel DMA Fences"] + userspace [label="userspace controlled fences"] + kernel -> userspace [label="memory management"] + userspace -> kernel [label="Future fence, fence proxy, ..."] + + { rank=same; kernel userspace } + } + +This means that the kernel might accidentally create deadlocks +through memory management dependencies which userspace is unaware of, which +randomly hangs workloads until the timeout kicks in. Workloads, which from +userspace's perspective, do not contain a deadlock. In such a mixed fencing +architecture there is no single entity with knowledge of all dependencies. +Thefore preventing such deadlocks from within the kernel is not possible. + +The only solution to avoid dependencies loops is by not allowing indefinite +fences in the kernel. This means: + +* No future fences, proxy fences or userspace fences imported as DMA fences, + with or without a timeout. + +* No DMA fences that signal end of batchbuffer for command submission where + userspace is allowed to use userspace fencing or long running compute + workloads. This also means no implicit fencing for shared buffers in these + cases. diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index f3ce49c5a34c..af55b334be2f 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -314,25 +314,6 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev, return &virtio_gpu_fb->base; } -static void vgdev_atomic_commit_tail(struct drm_atomic_state *state) -{ - struct drm_device *dev = state->dev; - - drm_atomic_helper_commit_modeset_disables(dev, state); - drm_atomic_helper_commit_modeset_enables(dev, state); - drm_atomic_helper_commit_planes(dev, state, 0); - - drm_atomic_helper_fake_vblank(state); - drm_atomic_helper_commit_hw_done(state); - - drm_atomic_helper_wait_for_vblanks(dev, state); - drm_atomic_helper_cleanup_planes(dev, state); -} - -static const struct drm_mode_config_helper_funcs virtio_mode_config_helpers = { - .atomic_commit_tail = vgdev_atomic_commit_tail, -}; - static const struct drm_mode_config_funcs virtio_gpu_mode_funcs = { .fb_create = virtio_gpu_user_framebuffer_create, .atomic_check = drm_atomic_helper_check, @@ -346,7 +327,6 @@ void virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev) drm_mode_config_init(vgdev->ddev); vgdev->ddev->mode_config.quirk_addfb_prefer_host_byte_order = true; vgdev->ddev->mode_config.funcs = &virtio_gpu_mode_funcs; - vgdev->ddev->mode_config.helper_private = &virtio_mode_config_helpers; /* modes will be validated against the framebuffer size */ vgdev->ddev->mode_config.min_width = XRES_MIN;
Comes up every few years, gets somewhat tedious to discuss, let's write this down once and for all. What I'm not sure about is whether the text should be more explicit in flat out mandating the amdkfd eviction fences for long running compute workloads or workloads where userspace fencing is allowed. v2: Now with dot graph! Cc: Jesse Natalie <jenatali@microsoft.com> Cc: Steve Pronovost <spronovo@microsoft.com> Cc: Jason Ekstrand <jason@jlekstrand.net> Cc: Felix Kuehling <Felix.Kuehling@amd.com> Cc: Mika Kuoppala <mika.kuoppala@intel.com> Cc: Thomas Hellstrom <thomas.hellstrom@intel.com> Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org Cc: linux-rdma@vger.kernel.org Cc: amd-gfx@lists.freedesktop.org Cc: intel-gfx@lists.freedesktop.org Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Christian König <christian.koenig@amd.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- Documentation/driver-api/dma-buf.rst | 70 ++++++++++++++++++++++++ drivers/gpu/drm/virtio/virtgpu_display.c | 20 ------- 2 files changed, 70 insertions(+), 20 deletions(-)