Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

14 participants
3243 discussions

Re: [Linaro-mm-sig] [PATCH v3 6/8] drm/shmem-helper: Provide a vmap function for short-term mappings

by Daniel Vetter

On Wed, Dec 09, 2020 at 03:25:25PM +0100, Thomas Zimmermann wrote: > Implementations of the vmap/vunmap GEM callbacks may perform pinning > of the BO and may acquire the associated reservation object's lock. > Callers that only require a mapping of the contained memory can thus > interfere with other tasks that require exact pinning, such as scanout. > This is less of an issue with private SHMEM buffers, but may happen > with imported ones. > > Therefore provide the new interfaces drm_gem_shmem_vmap_local() and > drm_gem_shmem_vunmap_local(), which only perform the vmap/vunmap > operations. Callers have to hold the reservation lock while the mapping > persists. > > The affected callers are display updates in cirrus, gm12u320, mgag200 > and udl. All are being changed to the new interface. > > This patch also connects GEM SHMEM helpers to GEM object functions with > equivalent functionality. > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 71 +++++++++++++++++++++++++- > drivers/gpu/drm/mgag200/mgag200_mode.c | 16 ++++-- > drivers/gpu/drm/tiny/cirrus.c | 10 +++- > drivers/gpu/drm/tiny/gm12u320.c | 14 +++-- > drivers/gpu/drm/udl/udl_modeset.c | 18 ++++--- I think that's a bit much driver conversion. I'd say in general split these out from the patch that adds the new functionality. More importantly, since vmap_local is optional, you can only start using it once all drivers have it. Especially with udl this will break use-cases otherwise, but also probably with gm12u320. > include/drm/drm_gem_shmem_helper.h | 2 + > 6 files changed, 115 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index 9825c378dfa6..41663f48d46a 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -32,6 +32,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = { > .get_sg_table = drm_gem_shmem_get_sg_table, > .vmap = drm_gem_shmem_vmap, > .vunmap = drm_gem_shmem_vunmap, > + .vmap_local = drm_gem_shmem_vmap_local, > + .vunmap_local = drm_gem_shmem_vunmap_local, > .mmap = drm_gem_shmem_mmap, > }; > > @@ -313,7 +315,7 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct > return ret; > } > > -/* > +/** > * drm_gem_shmem_vmap - Create a virtual mapping for a shmem GEM object > * @shmem: shmem GEM object > * @map: Returns the kernel virtual address of the SHMEM GEM object's backing > @@ -346,6 +348,44 @@ int drm_gem_shmem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map) > } > EXPORT_SYMBOL(drm_gem_shmem_vmap); > > +/** > + * drm_gem_shmem_vmap_local - Create a virtual mapping for a shmem GEM object > + * @shmem: shmem GEM object > + * @map: Returns the kernel virtual address of the SHMEM GEM object's backing > + * store. > + * > + * This function makes sure that a contiguous kernel virtual address mapping > + * exists for the buffer backing the shmem GEM object. > + * > + * The function is called with the BO's reservation object locked. Callers must > + * hold the lock until after unmapping the buffer. > + * > + * This function can be used to implement &drm_gem_object_funcs.vmap_local. But > + * it can also be called by drivers directly, in which case it will hide the > + * differences between dma-buf imported and natively allocated objects. > + * > + * Acquired mappings should be cleaned up by calling drm_gem_shmem_vunmap_local(). > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_shmem_vmap_local(struct drm_gem_object *obj, struct dma_buf_map *map) > +{ > + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > + int ret; > + > + dma_resv_assert_held(obj->resv); > + > + ret = mutex_lock_interruptible(&shmem->vmap_lock); This bites. You need to check for shmem->import_attach and call dma_buf_vmap_local directly here before you take any shmem helper locks. Real fix would be to replace both vmap_lock and pages_lock with dma_resv lock, but that's more work. Same for vunmap_local With that fixed on the helper part of this patch: Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > + if (ret) > + return ret; > + ret = drm_gem_shmem_vmap_locked(shmem, map); > + mutex_unlock(&shmem->vmap_lock); > + > + return ret; > +} > +EXPORT_SYMBOL(drm_gem_shmem_vmap_local); > + > static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, > struct dma_buf_map *map) > { > @@ -366,7 +406,7 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, > drm_gem_shmem_put_pages(shmem); > } > > -/* > +/** > * drm_gem_shmem_vunmap - Unmap a virtual mapping fo a shmem GEM object > * @shmem: shmem GEM object > * @map: Kernel virtual address where the SHMEM GEM object was mapped > @@ -389,6 +429,33 @@ void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map) > } > EXPORT_SYMBOL(drm_gem_shmem_vunmap); > > +/** > + * drm_gem_shmem_vunmap_local - Unmap a virtual mapping fo a shmem GEM object > + * @shmem: shmem GEM object > + * @map: Kernel virtual address where the SHMEM GEM object was mapped > + * > + * This function cleans up a kernel virtual address mapping acquired by > + * drm_gem_shmem_vmap_local(). The mapping is only removed when the use count > + * drops to zero. > + * > + * The function is called with the BO's reservation object locked. > + * > + * This function can be used to implement &drm_gem_object_funcs.vmap_local. > + * But it can also be called by drivers directly, in which case it will hide > + * the differences between dma-buf imported and natively allocated objects. > + */ > +void drm_gem_shmem_vunmap_local(struct drm_gem_object *obj, struct dma_buf_map *map) > +{ > + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > + > + dma_resv_assert_held(obj->resv); > + > + mutex_lock(&shmem->vmap_lock); > + drm_gem_shmem_vunmap_locked(shmem, map); > + mutex_unlock(&shmem->vmap_lock); > +} > +EXPORT_SYMBOL(drm_gem_shmem_vunmap_local); > + > struct drm_gem_shmem_object * > drm_gem_shmem_create_with_handle(struct drm_file *file_priv, > struct drm_device *dev, size_t size, > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c > index 1dfc42170059..a33e28d4c5e9 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_mode.c > +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c > @@ -1552,22 +1552,32 @@ mgag200_handle_damage(struct mga_device *mdev, struct drm_framebuffer *fb, > struct drm_rect *clip) > { > struct drm_device *dev = &mdev->base; > + struct drm_gem_object *obj = fb->obj[0]; > struct dma_buf_map map; > void *vmap; > int ret; > > - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > + ret = dma_resv_lock(obj->resv, NULL); > if (drm_WARN_ON(dev, ret)) > - return; /* BUG: SHMEM BO should always be vmapped */ > + return; > + ret = drm_gem_shmem_vmap_local(obj, &map); > + if (drm_WARN_ON(dev, ret)) > + goto err_dma_resv_unlock; /* BUG: SHMEM BO should always be vmapped */ > vmap = map.vaddr; /* TODO: Use mapping abstraction properly */ > > drm_fb_memcpy_dstclip(mdev->vram, vmap, fb, clip); > > - drm_gem_shmem_vunmap(fb->obj[0], &map); > + drm_gem_shmem_vunmap_local(obj, &map); > + dma_resv_unlock(obj->resv); > > /* Always scanout image at VRAM offset 0 */ > mgag200_set_startadd(mdev, (u32)0); > mgag200_set_offset(mdev, fb); > + > + return; > + > +err_dma_resv_unlock: > + dma_resv_unlock(obj->resv); > } > > static void > diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c > index 561c49d8657a..58c694964148 100644 > --- a/drivers/gpu/drm/tiny/cirrus.c > +++ b/drivers/gpu/drm/tiny/cirrus.c > @@ -315,6 +315,7 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb, > struct drm_rect *rect) > { > struct cirrus_device *cirrus = to_cirrus(fb->dev); > + struct drm_gem_object *obj = fb->obj[0]; > struct dma_buf_map map; > void *vmap; > int idx, ret; > @@ -323,9 +324,12 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb, > if (!drm_dev_enter(&cirrus->dev, &idx)) > goto out; > > - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > + ret = dma_resv_lock(obj->resv, NULL); > if (ret) > goto out_dev_exit; > + ret = drm_gem_shmem_vmap_local(fb->obj[0], &map); > + if (ret) > + goto out_dma_resv_unlock; > vmap = map.vaddr; /* TODO: Use mapping abstraction properly */ > > if (cirrus->cpp == fb->format->cpp[0]) > @@ -345,9 +349,11 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb, > else > WARN_ON_ONCE("cpp mismatch"); > > - drm_gem_shmem_vunmap(fb->obj[0], &map); > ret = 0; > > + drm_gem_shmem_vunmap_local(obj, &map); > +out_dma_resv_unlock: > + dma_resv_unlock(obj->resv); > out_dev_exit: > drm_dev_exit(idx); > out: > diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c > index 33f65f4626e5..b0c6e350f2b3 100644 > --- a/drivers/gpu/drm/tiny/gm12u320.c > +++ b/drivers/gpu/drm/tiny/gm12u320.c > @@ -265,11 +265,16 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320) > y1 = gm12u320->fb_update.rect.y1; > y2 = gm12u320->fb_update.rect.y2; > > - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > + ret = dma_resv_lock(fb->obj[0]->resv, NULL); > if (ret) { > - GM12U320_ERR("failed to vmap fb: %d\n", ret); > + GM12U320_ERR("failed to reserve fb: %d\n", ret); > goto put_fb; > } > + ret = drm_gem_shmem_vmap_local(fb->obj[0], &map); > + if (ret) { > + GM12U320_ERR("failed to vmap fb: %d\n", ret); > + goto unlock_resv; > + } > vaddr = map.vaddr; /* TODO: Use mapping abstraction properly */ > > if (fb->obj[0]->import_attach) { > @@ -321,8 +326,11 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320) > if (ret) > GM12U320_ERR("dma_buf_end_cpu_access err: %d\n", ret); > } > + > +unlock_resv: > + dma_resv_unlock(fb->obj[0]->resv); > vunmap: > - drm_gem_shmem_vunmap(fb->obj[0], &map); > + drm_gem_shmem_vunmap_local(fb->obj[0], &map); > put_fb: > drm_framebuffer_put(fb); > gm12u320->fb_update.fb = NULL; > diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c > index 9d34ec9d03f6..46b55b4d03c2 100644 > --- a/drivers/gpu/drm/udl/udl_modeset.c > +++ b/drivers/gpu/drm/udl/udl_modeset.c > @@ -290,14 +290,18 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > else if ((clip.x2 > fb->width) || (clip.y2 > fb->height)) > return -EINVAL; > > + ret = dma_resv_lock(fb->obj[0]->resv, NULL); > + if (ret) > + return ret; > + > if (import_attach) { > ret = dma_buf_begin_cpu_access(import_attach->dmabuf, > DMA_FROM_DEVICE); > if (ret) > - return ret; > + goto out_dma_resv_unlock; > } > > - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > + ret = drm_gem_shmem_vmap_local(fb->obj[0], &map); > if (ret) { > DRM_ERROR("failed to vmap fb\n"); > goto out_dma_buf_end_cpu_access; > @@ -307,7 +311,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > urb = udl_get_urb(dev); > if (!urb) { > ret = -ENOMEM; > - goto out_drm_gem_shmem_vunmap; > + goto out_drm_gem_shmem_vunmap_local; > } > cmd = urb->transfer_buffer; > > @@ -320,7 +324,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > &cmd, byte_offset, dev_byte_offset, > byte_width); > if (ret) > - goto out_drm_gem_shmem_vunmap; > + goto out_drm_gem_shmem_vunmap_local; > } > > if (cmd > (char *)urb->transfer_buffer) { > @@ -336,8 +340,8 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > > ret = 0; > > -out_drm_gem_shmem_vunmap: > - drm_gem_shmem_vunmap(fb->obj[0], &map); > +out_drm_gem_shmem_vunmap_local: > + drm_gem_shmem_vunmap_local(fb->obj[0], &map); > out_dma_buf_end_cpu_access: > if (import_attach) { > tmp_ret = dma_buf_end_cpu_access(import_attach->dmabuf, > @@ -345,6 +349,8 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > if (tmp_ret && !ret) > ret = tmp_ret; /* only update ret if not set yet */ > } > +out_dma_resv_unlock: > + dma_resv_unlock(fb->obj[0]->resv); > > return ret; > } > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h > index 434328d8a0d9..3f59bdf749aa 100644 > --- a/include/drm/drm_gem_shmem_helper.h > +++ b/include/drm/drm_gem_shmem_helper.h > @@ -114,7 +114,9 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem); > int drm_gem_shmem_pin(struct drm_gem_object *obj); > void drm_gem_shmem_unpin(struct drm_gem_object *obj); > int drm_gem_shmem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map); > +int drm_gem_shmem_vmap_local(struct drm_gem_object *obj, struct dma_buf_map *map); > void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map); > +void drm_gem_shmem_vunmap_local(struct drm_gem_object *obj, struct dma_buf_map *map); > > int drm_gem_shmem_madvise(struct drm_gem_object *obj, int madv); > > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

Re: [Linaro-mm-sig] [PATCH v3 4/8] drm/gem: Create infrastructure for GEM vmap_local

by Daniel Vetter

On Wed, Dec 09, 2020 at 03:25:23PM +0100, Thomas Zimmermann wrote: > This patch adds vmap_local and vunmap_local to struct drm_gem_object_funcs; > including the PRIME helpers to connect with dma-buf's related interfaces. > > Besides the generic DRM core, this will become relevant for fbdev emulation > with virtio, so we update it as well. > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> > --- > drivers/gpu/drm/drm_gem.c | 28 ++++++++++++++++++ > drivers/gpu/drm/drm_internal.h | 2 ++ > drivers/gpu/drm/drm_prime.c | 39 ++++++++++++++++++++++++++ > drivers/gpu/drm/virtio/virtgpu_prime.c | 2 ++ > include/drm/drm_gem.h | 20 +++++++++++++ > include/drm/drm_prime.h | 2 ++ > 6 files changed, 93 insertions(+) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 92f89cee213e..6e131d9bb7bd 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -1234,6 +1234,34 @@ void drm_gem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map) > dma_buf_map_clear(map); > } > > +int drm_gem_vmap_local(struct drm_gem_object *obj, struct dma_buf_map *map) > +{ > + int ret; > + > + if (!obj->funcs->vmap_local) > + return -EOPNOTSUPP; > + > + ret = obj->funcs->vmap_local(obj, map); > + if (ret) > + return ret; > + else if (dma_buf_map_is_null(map)) > + return -ENOMEM; > + > + return 0; > +} > + > +void drm_gem_vunmap_local(struct drm_gem_object *obj, struct dma_buf_map *map) > +{ > + if (dma_buf_map_is_null(map)) > + return; > + > + if (obj->funcs->vunmap_local) > + obj->funcs->vunmap_local(obj, map); > + > + /* Always set the mapping to NULL. Callers may rely on this. */ > + dma_buf_map_clear(map); > +} > + > /** > * drm_gem_lock_reservations - Sets up the ww context and acquires > * the lock on an array of GEM objects. > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > index 81d386b5b92a..b0bf6aba763a 100644 > --- a/drivers/gpu/drm/drm_internal.h > +++ b/drivers/gpu/drm/drm_internal.h > @@ -190,6 +190,8 @@ int drm_gem_pin(struct drm_gem_object *obj); > void drm_gem_unpin(struct drm_gem_object *obj); > int drm_gem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map); > void drm_gem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map); > +int drm_gem_vmap_local(struct drm_gem_object *obj, struct dma_buf_map *map); > +void drm_gem_vunmap_local(struct drm_gem_object *obj, struct dma_buf_map *map); > > /* drm_debugfs.c drm_debugfs_crc.c */ > #if defined(CONFIG_DEBUG_FS) > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 683aa29ecd3b..633edea76985 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -695,6 +695,43 @@ void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, struct dma_buf_map *map) > } > EXPORT_SYMBOL(drm_gem_dmabuf_vunmap); > > +/** > + * drm_gem_dmabuf_vmap_local - dma_buf vmap_local implementation for GEM > + * @dma_buf: buffer to be mapped > + * @map: the virtual address of the buffer > + * > + * Sets up a kernel virtual mapping. This can be used as the &dma_buf_ops.vmap_local > + * callback. Calls into &drm_gem_object_funcs.vmap_local for device specific handling. > + * The kernel virtual address is returned in map. > + * > + * Returns: > + * 0 on success or a negative errno code otherwise. > + */ > +int drm_gem_dmabuf_vmap_local(struct dma_buf *dma_buf, struct dma_buf_map *map) > +{ > + struct drm_gem_object *obj = dma_buf->priv; > + > + return drm_gem_vmap_local(obj, map); > +} > +EXPORT_SYMBOL(drm_gem_dmabuf_vmap_local); > + > +/** > + * drm_gem_dmabuf_vunmap_local - dma_buf vunmap_local implementation for GEM > + * @dma_buf: buffer to be unmapped > + * @map: the virtual address of the buffer > + * > + * Releases a kernel virtual mapping. This can be used as the > + * &dma_buf_ops.vunmap_local callback. Calls into &drm_gem_object_funcs.vunmap_local > + * for device specific handling. > + */ > +void drm_gem_dmabuf_vunmap_local(struct dma_buf *dma_buf, struct dma_buf_map *map) > +{ > + struct drm_gem_object *obj = dma_buf->priv; > + > + drm_gem_vunmap_local(obj, map); > +} > +EXPORT_SYMBOL(drm_gem_dmabuf_vunmap_local); > + > /** > * drm_gem_prime_mmap - PRIME mmap function for GEM drivers > * @obj: GEM object > @@ -787,6 +824,8 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = { > .mmap = drm_gem_dmabuf_mmap, > .vmap = drm_gem_dmabuf_vmap, > .vunmap = drm_gem_dmabuf_vunmap, > + .vmap_local = drm_gem_dmabuf_vmap_local, > + .vunmap_local = drm_gem_dmabuf_vunmap_local, > }; > > /** > diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c > index 807a27a16365..fea11a53d8fc 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_prime.c > +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c > @@ -54,6 +54,8 @@ static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops = { > .mmap = drm_gem_dmabuf_mmap, > .vmap = drm_gem_dmabuf_vmap, > .vunmap = drm_gem_dmabuf_vunmap, > + .vmap = drm_gem_dmabuf_vmap_local, > + .vunmap = drm_gem_dmabuf_vunmap_local, > }, > .device_attach = drm_gem_map_attach, > .get_uuid = virtgpu_virtio_get_uuid, > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > index 5e6daa1c982f..1281f26de494 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -151,6 +151,26 @@ struct drm_gem_object_funcs { > */ > void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map *map); > > + /** > + * @vmap_local: > + * > + * Returns a virtual address for the buffer. Used by the > + * drm_gem_dmabuf_vmap_local() helper. I'd add "Callers will hold &drm_gem_object.resv already and only release it after @vunmap is called" (and pls check I typed this correctly for the linking). Either way: Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > + * > + * This callback is optional. > + */ > + int (*vmap_local)(struct drm_gem_object *obj, struct dma_buf_map *map); > + > + /** > + * @vunmap_local: > + * > + * Releases the address previously returned by @vmap. Used by the > + * drm_gem_dmabuf_vunmap_local() helper. > + * > + * This callback is optional. > + */ > + void (*vunmap_local)(struct drm_gem_object *obj, struct dma_buf_map *map); > + > /** > * @mmap: > * > diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h > index 54f2c58305d2..fd2aef6966ef 100644 > --- a/include/drm/drm_prime.h > +++ b/include/drm/drm_prime.h > @@ -85,6 +85,8 @@ void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach, > enum dma_data_direction dir); > int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map *map); > void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, struct dma_buf_map *map); > +int drm_gem_dmabuf_vmap_local(struct dma_buf *dma_buf, struct dma_buf_map *map); > +void drm_gem_dmabuf_vunmap_local(struct dma_buf *dma_buf, struct dma_buf_map *map); > > int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma); > int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma); > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

Re: [Linaro-mm-sig] [PATCH v3 3/8] dma-buf: Add vmap_local and vnumap_local operations

by Daniel Vetter

On Wed, Dec 09, 2020 at 03:25:22PM +0100, Thomas Zimmermann wrote: > The existing dma-buf calls dma_buf_vmap() and dma_buf_vunmap() are > allowed to pin the buffer or acquire the buffer's reservation object > lock. > > This is a problem for callers that only require a short-term mapping > of the buffer without the pinning, or callers that have special locking > requirements. These may suffer from unnecessary overhead or interfere > with regular pin operations. > > The new interfaces dma_buf_vmap_local(), dma_buf_vunmapo_local(), and > their rsp callbacks in struct dma_buf_ops provide an alternative without > pinning or reservation locking. Callers are responsible for these > operations. > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> > --- > drivers/dma-buf/dma-buf.c | 80 +++++++++++++++++++++++++++++++++++++++ > include/linux/dma-buf.h | 34 +++++++++++++++++ > 2 files changed, 114 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index e63684d4cd90..be9f80190a66 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1265,6 +1265,86 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map) > } > EXPORT_SYMBOL_GPL(dma_buf_vunmap); > > +/** > + * dma_buf_vmap_local - Create virtual mapping for the buffer object into kernel > + * address space. > + * @dmabuf: [in] buffer to vmap > + * @map: [out] returns the vmap pointer > + * > + * This call may fail due to lack of virtual mapping address space. > + * These calls are optional in drivers. The intended use for them > + * is for mapping objects linear in kernel space for high use objects. > + * Please attempt to use kmap/kunmap before thinking about these interfaces. Kmap is gone, so the entire 2 sentences here are no longer needed. Maybe mention something like "Unlike dma_buf_vmap() this is a short term mapping and will not pin the buffer. The struct dma_resv for the @dmabuf must be locked until dma_buf_vunmap_local() is called." > + * > + * Returns: > + * 0 on success, or a negative errno code otherwise. > + */ > +int dma_buf_vmap_local(struct dma_buf *dmabuf, struct dma_buf_map *map) > +{ > + struct dma_buf_map ptr; > + int ret = 0; > + > + dma_buf_map_clear(map); > + > + if (WARN_ON(!dmabuf)) > + return -EINVAL; > + > + dma_resv_assert_held(dmabuf->resv); > + > + if (!dmabuf->ops->vmap_local) > + return -EINVAL; > + > + mutex_lock(&dmabuf->lock); > + if (dmabuf->vmapping_counter) { > + dmabuf->vmapping_counter++; > + BUG_ON(dma_buf_map_is_null(&dmabuf->vmap_ptr)); > + *map = dmabuf->vmap_ptr; > + goto out_unlock; > + } > + > + BUG_ON(dma_buf_map_is_set(&dmabuf->vmap_ptr)); > + > + ret = dmabuf->ops->vmap_local(dmabuf, &ptr); > + if (WARN_ON_ONCE(ret)) > + goto out_unlock; > + > + dmabuf->vmap_ptr = ptr; > + dmabuf->vmapping_counter = 1; > + > + *map = dmabuf->vmap_ptr; > + > +out_unlock: > + mutex_unlock(&dmabuf->lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(dma_buf_vmap_local); > + > +/** > + * dma_buf_vunmap_local - Unmap a vmap obtained by dma_buf_vmap_local. > + * @dmabuf: [in] buffer to vunmap > + * @map: [in] vmap pointer to vunmap Maybe for hyperlinking add "Release a mapping established with dma_buf_vmap_local()." > + */ > +void dma_buf_vunmap_local(struct dma_buf *dmabuf, struct dma_buf_map *map) > +{ > + if (WARN_ON(!dmabuf)) > + return; > + > + dma_resv_assert_held(dmabuf->resv); > + > + BUG_ON(dma_buf_map_is_null(&dmabuf->vmap_ptr)); > + BUG_ON(dmabuf->vmapping_counter == 0); > + BUG_ON(!dma_buf_map_is_equal(&dmabuf->vmap_ptr, map)); > + > + mutex_lock(&dmabuf->lock); > + if (--dmabuf->vmapping_counter == 0) { > + if (dmabuf->ops->vunmap_local) > + dmabuf->ops->vunmap_local(dmabuf, map); > + dma_buf_map_clear(&dmabuf->vmap_ptr); > + } > + mutex_unlock(&dmabuf->lock); > +} > +EXPORT_SYMBOL_GPL(dma_buf_vunmap_local); > + > #ifdef CONFIG_DEBUG_FS > static int dma_buf_debug_show(struct seq_file *s, void *unused) > { > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index cf72699cb2bc..f66580d23a9b 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -269,6 +269,38 @@ struct dma_buf_ops { > > int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > + > + /** > + * @vmap_local: > + * > + * Creates a virtual mapping for the buffer into kernel address space. > + * > + * This callback establishes short-term mappings for situations where > + * callers only use the buffer for a bounded amount of time; such as > + * updates to the framebuffer or reading back contained information. > + * In contrast to the regular @vmap callback, vmap_local does never pin > + * the buffer to a specific domain or acquire the buffer's reservation > + * lock. > + * > + * This is called with the dmabuf->resv object locked. Callers must hold ^^Not the right kerneldoc, I think it should be &dma_buf.resv to get the hyperlink. > + * the lock until after removing the mapping with @vunmap_local. > + * > + * This callback is optional. > + * > + * Returns: > + * > + * 0 on success or a negative error code on failure. > + */ > + int (*vmap_local)(struct dma_buf *dmabuf, struct dma_buf_map *map); > + > + /** > + * @vunmap_local: > + * > + * Removes a virtual mapping that wa sestablished by @vmap_local. ^^established > + * > + * This callback is optional. > + */ > + void (*vunmap_local)(struct dma_buf *dmabuf, struct dma_buf_map *map); > }; > > /** > @@ -506,4 +538,6 @@ int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *, > unsigned long); > int dma_buf_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map); > void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map); > +int dma_buf_vmap_local(struct dma_buf *dmabuf, struct dma_buf_map *map); > +void dma_buf_vunmap_local(struct dma_buf *dmabuf, struct dma_buf_map *map); > #endif /* __DMA_BUF_H__ */ With the doc nits addressed: Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

[PATCH 0/4] Chunk Heap Support on DMA-HEAP

by Minchan Kim

This patchset introduces a new dma heap, chunk heap that makes it easy to perform the bulk allocation of high order pages. It has been created to help optimize the 4K/8K HDR video playback with secure DRM HW to protect contents on memory. The HW needs physically contiguous memory chunks up to several hundred MB memory. The chunk heap is registered by device tree with alignment and memory node of Contiguous Memory Allocator(CMA). Alignment defines chunk page size. For example, alignment 0x1_0000 means chunk page size is 64KB. The phandle to memory node indicates contiguous memory allocator(CMA). If device node doesn't have cma, the registration of chunk heap fails. This patchset is against on next-20201110. The patchset includes the following: - cma_alloc_bulk API - export dma-heap API to register kernel module dma heap. - add chunk heap implementation. - devicetree Hyesoo Yu (3): dma-buf: add export symbol for dma-heap dma-buf: heaps: add chunk heap to dmabuf heaps dma-heap: Devicetree binding for chunk heap Minchan Kim (1): mm: introduce cma_alloc_bulk API .../bindings/dma-buf/chunk_heap.yaml | 52 ++ drivers/dma-buf/dma-heap.c | 2 + drivers/dma-buf/heaps/Kconfig | 9 + drivers/dma-buf/heaps/Makefile | 1 + drivers/dma-buf/heaps/chunk_heap.c | 458 ++++++++++++++++++ include/linux/cma.h | 5 + include/linux/page-isolation.h | 1 + mm/cma.c | 129 ++++- mm/page_alloc.c | 19 +- mm/page_isolation.c | 3 +- 10 files changed, 666 insertions(+), 13 deletions(-) create mode 100644 Documentation/devicetree/bindings/dma-buf/chunk_heap.yaml create mode 100644 drivers/dma-buf/heaps/chunk_heap.c -- 2.29.2.299.gdc1121823c-goog

5 years

Re: [Linaro-mm-sig] [PATCH v2 7/7] dma-buf: Write down some rules for vmap usage

by Daniel Vetter

On Wed, Dec 9, 2020 at 10:32 AM Thomas Zimmermann <tzimmermann(a)suse.de> wrote: > > Hi > > Am 09.12.20 um 01:13 schrieb Daniel Vetter: > > On Fri, Dec 04, 2020 at 09:47:08AM +0100, Christian König wrote: > >> Am 04.12.20 um 09:32 schrieb Thomas Zimmermann: > >>> Hi > >>> > >>> Am 03.12.20 um 21:41 schrieb Daniel Vetter: > >>>> On Thu, Dec 03, 2020 at 07:59:04PM +0100, Thomas Zimmermann wrote: > >>>>> Hi > >>>>> > >>>>> Am 03.12.20 um 16:26 schrieb Daniel Vetter: > >>>>>> On Thu, Dec 03, 2020 at 03:02:59PM +0100, Thomas Zimmermann wrote: > >>>>>>> Dma-buf's vmap and vunmap callbacks are undocumented and various > >>>>>>> exporters currently have slightly different semantics for them. Add > >>>>>>> documentation on how to implement and use these interfaces correctly. > >>>>>>> > >>>>>>> v2: > >>>>>>> * document vmap semantics in struct dma_buf_ops > >>>>>>> * add TODO item for reviewing and maybe fixing dma-buf exporters > >>>>>>> > >>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> > >>>>>> > >>>>>> I still don't think this works, we're breaking dma_buf_vmap > >>>>>> for everyone > >>>>>> else here. > >>>>> > >>>>> I removed the text on the importer. These notes for callers in > >>>>> the docs are > >>>>> more or less a consequence of the exporter semantics. > >>>> > >>>> Callers are importers, so I'm not seeing how that fixes anything. > >>>> > >>>>> I thought we at least agreed on the exporter semantics in the > >>>>> other thread, > >>>>> didn't we? > >>>>> > >>>>> What I'm trying to do is to write dome some rules for exporters, > >>>>> even if not > >>>>> all exporters follow them. > >>>> > >>>> This is a standard interface, everyone needs to follow the same > >>>> rules. And > >>>> if they change, we need to make sure nothing breaks and we're not > >>>> creating > >>>> issues. > >>>> > >>>> In the past the rule was the dma_buf_vmap was allowed to take the > >>>> dma_resv_lock, and that the buffer should be pinned. Now some ttm > >>>> drivers > >>>> didn't ever bother with the pinning, and mostly got away with that > >>>> because > >>>> drm_prime helpers do the pinning by default at attach time, and most > >>>> users > >>>> do call dma_buf_attach. > >>>> > >>>> But if you look through dma-buf docs nothing ever said you have to do a > >>>> dummy attachment before you call dma_buf_vmap, that's just slightly > >>>> crappy > >>>> implementations that didn't blow up yet. > >>> > >>> I had a patch for adding pin to radeon's implementation of vmap. [1] > >>> Christian told me to not do this; instead just get the lock in the fbdev > >>> code. His advise almost seems the opposite of what you're telling me > >>> here. > >> > >> I think what Daniel suggests here is that we need to smoothly transition the > >> code from making assumptions to having a straight interface where importers > >> explicitly say when stuff is locked and when stuff is pinned. > >> > >> I've started this with the attach interface by adding a new dynamic approach > >> to that, but you probably need to carry on here with that for vmap as well. > >> > >> When that is done we can migrate every exporter over to the new dynamic > >> approach. > >> > >>> > >>> For the GEM VRAM helpers, that implicit pin in vmap gave me headaches. > >>> Because scanouts can only be done from VRAM, which is badly suited for > >>> exporting. So I ended up with an implicit pin that pins the buffer to > >>> whatever domain it currently is. I got away with it because GEM VRAM BOs > >>> are not sharable among devices; fbdev is the only user of that > >>> functionality and only pins for short periods of time. > >>> > >>> I suspect that fixing TTM-based drivers by adding an implicit pin would > >>> result in a similar situation. Whatever domain it ends up pinning, some > >>> functionality might not be compatible with that. > >> > >> Correct, exactly that's the problem. > >> > >>> > >>>> > >>>>> Given the circumstances, we should leave out this patch from the > >>>>> patchset. > >>>> > >>>> So the defacto rules are already a big mess, but that's not a good > >>>> excuse > >>>> to make it worse. > >>>> > >>>> What I had in mind is that we split dma_buf_vmap up into two variants: > >>>> > >>>> - The current one, which should guarantee that the buffer is pinned. > >>>> Because that's what all current callers wanted, before the fbdev code > >>>> started allowing non-pinned buffers. > >>> > >>> Can we add an explicit pin operation to dma_buf_vmap() to enforce the > >>> semantics? > >> > >> At least I would be fine with that. For now amdgpu is the only exporter who > >> implements the explicit pin/unpin semantics anyway. > > > > Yup, I think that makes sense (if it works). Maybe we could use something > > like: > > > > a) dma_buf pin exists, driver is dynamic. This means dma_buf_vmap needs to > > first pin, then call ->vmap. dma_buf_vmap_local otoh can directly call > > ->vmap since the exporter relies on either a pin or dma_resv_lock. > > > > b) dma_buf pin not implement, driver is a legacy pile. dma_buf_vmap will > > pin (somewhere at least, or rely on some implicit pin), dma_buf_vmap_local > > doesn't work and should fail. > > I think I read in the dma-buf documentation that pin is supposed to put > the BO in a domain that is suitable for scanout. Now I don't really > trust this to work. Amdgpu, radeon and nouveau put it into the GTT > region. Qxl appears to put it wherever it is. Uh that sounds wrong, or at least not the full complexity. ->pin's main use right now is to freeze the dma-buf into system memory when there's a non-dynamic attachement for a dynamic exporter. There's also a dma_buf_pin call in amdgpu, and I think that only works for scanout on integrated gpu. Pinning to vram would break sharing for all existing dma-buf users. Christian can you perhaps explain when amdgpu uses dma_buf_pin()? My take is the ->pin callback should clarify that it should pin into system memory, for legacy (non-dynamic) dma-buf users. And dma_buf_pin() should gain some kerneldoc which then states that "This should only be used in limited use cases like scanout and not for temporary pin operations." I think the problem with this kerneldoc is simply that it tries to document the exporter and importer side of the contract in one place and makes a mess of it, plus leaves the actual importer side function undocumented. I think the kerneldoc also predates the final patch version, and we didn't update it fully. > > I think for less transition work fbdev helpers could first try > > dma_resv_lock + dma_buf_vmap_local, if that fails, drop the dma_resv_lock > > and do the pinning dma_buf_vmap. That way we don't have to convert shmem > > helpers over to dma_resv locking, which should help. > > I have meanwhile made a patchset that updates helpers for cma, shmem and > vram with vmap_local; and converts fbdev emulation as well. It needs a > bit more testing before being posted. Nice, even better! -Daniel > Best regards > Thomas > > > > > And ttm drivers would do the new clean interface, so at least everyone > > using dma_resv today is all fine. Intel's conversion to dma_resv lock is > > in-flight, but that needs a conversion to the dynamic interface anyway, > > the current code splats. And dynamic brings means explicit pin/unpin > > callbacks, so should be good too. > > -Daniel > > > >> > >> Regards, > >> Christian. > >> > >>> > >>> Best regards > >>> Thomas > >>> > >>> [1] https://patchwork.freedesktop.org/patch/400054/?series=83765&rev=1 > >>> > >>>> > >>>> - The new one, which allows vmapping with just dma_resv locked, and > >>>> should > >>>> have some caching in exporters. > >>>> > >>>> Breaking code and then adding todos about that is kinda not so cool > >>>> approach here imo. > >>>> > >>>> Also I guess ttm_bo_vmap should have a check that either the buffer is > >>>> pinned, or dma_resv_lock is held. > >>>> > >>>> Cheers, Daniel > >>>> > >>>> > >>>> > >>>>> > >>>>> Best regards > >>>>> Thomas > >>>>> > >>>>>> > >>>>>>> --- > >>>>>>> Documentation/gpu/todo.rst | 15 +++++++++++++ > >>>>>>> include/drm/drm_gem.h | 4 ++++ > >>>>>>> include/linux/dma-buf.h | 45 > >>>>>>> ++++++++++++++++++++++++++++++++++++++ > >>>>>>> 3 files changed, 64 insertions(+) > >>>>>>> > >>>>>>> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > >>>>>>> index 009d8e6c7e3c..32bb797a84fc 100644 > >>>>>>> --- a/Documentation/gpu/todo.rst > >>>>>>> +++ b/Documentation/gpu/todo.rst > >>>>>>> @@ -505,6 +505,21 @@ Contact: Thomas Zimmermann > >>>>>>> <tzimmermann(a)suse.de>, Christian König, Daniel Vette > >>>>>>> Level: Intermediate > >>>>>>> +Enforce rules for dma-buf vmap and pin ops > >>>>>>> +------------------------------------------ > >>>>>>> + > >>>>>>> +Exporter implementations of vmap and pin in struct > >>>>>>> dma_buf_ops (and consequently > >>>>>>> +struct drm_gem_object_funcs) use a variety of locking > >>>>>>> semantics. Some rely on > >>>>>>> +the caller holding the dma-buf's reservation lock, some > >>>>>>> do their own locking, > >>>>>>> +some don't require any locking. VRAM helpers even used > >>>>>>> to pin as part of vmap. > >>>>>>> + > >>>>>>> +We need to review each exporter and enforce the documented rules. > >>>>>>> + > >>>>>>> +Contact: Christian König, Daniel Vetter, Thomas > >>>>>>> Zimmermann <tzimmermann(a)suse.de> > >>>>>>> + > >>>>>>> +Level: Advanced > >>>>>>> + > >>>>>>> + > >>>>>>> Core refactorings > >>>>>>> ================= > >>>>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > >>>>>>> index 5e6daa1c982f..1864c6a721b1 100644 > >>>>>>> --- a/include/drm/drm_gem.h > >>>>>>> +++ b/include/drm/drm_gem.h > >>>>>>> @@ -138,6 +138,8 @@ struct drm_gem_object_funcs { > >>>>>>> * drm_gem_dmabuf_vmap() helper. > >>>>>>> * > >>>>>>> * This callback is optional. > >>>>>>> + * > >>>>>>> + * See also struct dma_buf_ops.vmap > >>>>>>> */ > >>>>>>> int (*vmap)(struct drm_gem_object *obj, struct > >>>>>>> dma_buf_map *map); > >>>>>>> @@ -148,6 +150,8 @@ struct drm_gem_object_funcs { > >>>>>>> * drm_gem_dmabuf_vunmap() helper. > >>>>>>> * > >>>>>>> * This callback is optional. > >>>>>>> + * > >>>>>>> + * See also struct dma_buf_ops.vunmap > >>>>>>> */ > >>>>>>> void (*vunmap)(struct drm_gem_object *obj, struct > >>>>>>> dma_buf_map *map); > >>>>>>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > >>>>>>> index cf72699cb2bc..dc81fdc01dda 100644 > >>>>>>> --- a/include/linux/dma-buf.h > >>>>>>> +++ b/include/linux/dma-buf.h > >>>>>>> @@ -267,7 +267,52 @@ struct dma_buf_ops { > >>>>>>> */ > >>>>>>> int (*mmap)(struct dma_buf *, struct vm_area_struct *vma); > >>>>>>> + /** > >>>>>>> + * @vmap: > >>>>>> > >>>>>> There's already a @vmap and @vunamp kerneldoc at the top comment, that > >>>>>> needs to be removed. > >>>>>> -Daniel > >>>>>> > >>>>>>> + * > >>>>>>> + * Returns a virtual address for the buffer. > >>>>>>> + * > >>>>>>> + * Notes to callers: > >>>>>>> + * > >>>>>>> + * - Callers must hold the struct dma_buf.resv lock > >>>>>>> before calling > >>>>>>> + * this interface. > >>>>>>> + * > >>>>>>> + * - Callers must provide means to prevent the > >>>>>>> mappings from going > >>>>>>> + * stale, such as holding the reservation lock or providing a > >>>>>>> + * move-notify callback to the exporter. > >>>>>>> + * > >>>>>>> + * Notes to implementors: > >>>>>>> + * > >>>>>>> + * - Implementations must expect pairs of @vmap and > >>>>>>> @vunmap to be > >>>>>>> + * called frequently and should optimize for this case. > >>>>>>> + * > >>>>>>> + * - Implementations should avoid additional operations, such as > >>>>>>> + * pinning. > >>>>>>> + * > >>>>>>> + * - Implementations may expect the caller to hold the dma-buf's > >>>>>>> + * reservation lock to protect against concurrent calls and > >>>>>>> + * relocation. > >>>>>>> + * > >>>>>>> + * - Implementations may provide additional > >>>>>>> guarantees, such as working > >>>>>>> + * without holding the reservation lock. > >>>>>>> + * > >>>>>>> + * This callback is optional. > >>>>>>> + * > >>>>>>> + * Returns: > >>>>>>> + * > >>>>>>> + * 0 on success or a negative error code on failure. > >>>>>>> + */ > >>>>>>> int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > >>>>>>> + > >>>>>>> + /** > >>>>>>> + * @vunmap: > >>>>>>> + * > >>>>>>> + * Releases the address previously returned by @vmap. > >>>>>>> + * > >>>>>>> + * This callback is optional. > >>>>>>> + * > >>>>>>> + * See also @vmap() > >>>>>>> + */ > >>>>>>> void (*vunmap)(struct dma_buf *dmabuf, struct > >>>>>>> dma_buf_map *map); > >>>>>>> }; > >>>>>>> -- > >>>>>>> 2.29.2 > >>>>>>> > >>>>>> > >>>>> > >>>>> -- > >>>>> Thomas Zimmermann > >>>>> Graphics Driver Developer > >>>>> SUSE Software Solutions Germany GmbH > >>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany > >>>>> (HRB 36809, AG Nürnberg) > >>>>> Geschäftsführer: Felix Imendörffer > >>>>> > >>>> > >>>> > >>>> > >>>> > >>> > >> > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

Re: [Linaro-mm-sig] [PATCH v2 7/7] dma-buf: Write down some rules for vmap usage

by Christian König

Am 04.12.20 um 09:32 schrieb Thomas Zimmermann: > Hi > > Am 03.12.20 um 21:41 schrieb Daniel Vetter: >> On Thu, Dec 03, 2020 at 07:59:04PM +0100, Thomas Zimmermann wrote: >>> Hi >>> >>> Am 03.12.20 um 16:26 schrieb Daniel Vetter: >>>> On Thu, Dec 03, 2020 at 03:02:59PM +0100, Thomas Zimmermann wrote: >>>>> Dma-buf's vmap and vunmap callbacks are undocumented and various >>>>> exporters currently have slightly different semantics for them. Add >>>>> documentation on how to implement and use these interfaces correctly. >>>>> >>>>> v2: >>>>> * document vmap semantics in struct dma_buf_ops >>>>> * add TODO item for reviewing and maybe fixing dma-buf exporters >>>>> >>>>> Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> >>>> >>>> I still don't think this works, we're breaking dma_buf_vmap for >>>> everyone >>>> else here. >>> >>> I removed the text on the importer. These notes for callers in the >>> docs are >>> more or less a consequence of the exporter semantics. >> >> Callers are importers, so I'm not seeing how that fixes anything. >> >>> I thought we at least agreed on the exporter semantics in the other >>> thread, >>> didn't we? >>> >>> What I'm trying to do is to write dome some rules for exporters, >>> even if not >>> all exporters follow them. >> >> This is a standard interface, everyone needs to follow the same >> rules. And >> if they change, we need to make sure nothing breaks and we're not >> creating >> issues. >> >> In the past the rule was the dma_buf_vmap was allowed to take the >> dma_resv_lock, and that the buffer should be pinned. Now some ttm >> drivers >> didn't ever bother with the pinning, and mostly got away with that >> because >> drm_prime helpers do the pinning by default at attach time, and most >> users >> do call dma_buf_attach. >> >> But if you look through dma-buf docs nothing ever said you have to do a >> dummy attachment before you call dma_buf_vmap, that's just slightly >> crappy >> implementations that didn't blow up yet. > > I had a patch for adding pin to radeon's implementation of vmap. [1] > Christian told me to not do this; instead just get the lock in the > fbdev code. His advise almost seems the opposite of what you're > telling me here. I think what Daniel suggests here is that we need to smoothly transition the code from making assumptions to having a straight interface where importers explicitly say when stuff is locked and when stuff is pinned. I've started this with the attach interface by adding a new dynamic approach to that, but you probably need to carry on here with that for vmap as well. When that is done we can migrate every exporter over to the new dynamic approach. > > For the GEM VRAM helpers, that implicit pin in vmap gave me headaches. > Because scanouts can only be done from VRAM, which is badly suited for > exporting. So I ended up with an implicit pin that pins the buffer to > whatever domain it currently is. I got away with it because GEM VRAM > BOs are not sharable among devices; fbdev is the only user of that > functionality and only pins for short periods of time. > > I suspect that fixing TTM-based drivers by adding an implicit pin > would result in a similar situation. Whatever domain it ends up > pinning, some functionality might not be compatible with that. Correct, exactly that's the problem. > >> >>> Given the circumstances, we should leave out this patch from the >>> patchset. >> >> So the defacto rules are already a big mess, but that's not a good >> excuse >> to make it worse. >> >> What I had in mind is that we split dma_buf_vmap up into two variants: >> >> - The current one, which should guarantee that the buffer is pinned. >> Because that's what all current callers wanted, before the fbdev code >> started allowing non-pinned buffers. > > Can we add an explicit pin operation to dma_buf_vmap() to enforce the > semantics? At least I would be fine with that. For now amdgpu is the only exporter who implements the explicit pin/unpin semantics anyway. Regards, Christian. > > Best regards > Thomas > > [1] https://patchwork.freedesktop.org/patch/400054/?series=83765&rev=1 > >> >> - The new one, which allows vmapping with just dma_resv locked, and >> should >> have some caching in exporters. >> >> Breaking code and then adding todos about that is kinda not so cool >> approach here imo. >> >> Also I guess ttm_bo_vmap should have a check that either the buffer is >> pinned, or dma_resv_lock is held. >> >> Cheers, Daniel >> >> >> >>> >>> Best regards >>> Thomas >>> >>>> >>>>> --- >>>>> Documentation/gpu/todo.rst | 15 +++++++++++++ >>>>> include/drm/drm_gem.h | 4 ++++ >>>>> include/linux/dma-buf.h | 45 >>>>> ++++++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 64 insertions(+) >>>>> >>>>> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst >>>>> index 009d8e6c7e3c..32bb797a84fc 100644 >>>>> --- a/Documentation/gpu/todo.rst >>>>> +++ b/Documentation/gpu/todo.rst >>>>> @@ -505,6 +505,21 @@ Contact: Thomas Zimmermann >>>>> <tzimmermann(a)suse.de>, Christian König, Daniel Vette >>>>> Level: Intermediate >>>>> +Enforce rules for dma-buf vmap and pin ops >>>>> +------------------------------------------ >>>>> + >>>>> +Exporter implementations of vmap and pin in struct dma_buf_ops >>>>> (and consequently >>>>> +struct drm_gem_object_funcs) use a variety of locking semantics. >>>>> Some rely on >>>>> +the caller holding the dma-buf's reservation lock, some do their >>>>> own locking, >>>>> +some don't require any locking. VRAM helpers even used to pin as >>>>> part of vmap. >>>>> + >>>>> +We need to review each exporter and enforce the documented rules. >>>>> + >>>>> +Contact: Christian König, Daniel Vetter, Thomas Zimmermann >>>>> <tzimmermann(a)suse.de> >>>>> + >>>>> +Level: Advanced >>>>> + >>>>> + >>>>> Core refactorings >>>>> ================= >>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h >>>>> index 5e6daa1c982f..1864c6a721b1 100644 >>>>> --- a/include/drm/drm_gem.h >>>>> +++ b/include/drm/drm_gem.h >>>>> @@ -138,6 +138,8 @@ struct drm_gem_object_funcs { >>>>> * drm_gem_dmabuf_vmap() helper. >>>>> * >>>>> * This callback is optional. >>>>> + * >>>>> + * See also struct dma_buf_ops.vmap >>>>> */ >>>>> int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map >>>>> *map); >>>>> @@ -148,6 +150,8 @@ struct drm_gem_object_funcs { >>>>> * drm_gem_dmabuf_vunmap() helper. >>>>> * >>>>> * This callback is optional. >>>>> + * >>>>> + * See also struct dma_buf_ops.vunmap >>>>> */ >>>>> void (*vunmap)(struct drm_gem_object *obj, struct >>>>> dma_buf_map *map); >>>>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h >>>>> index cf72699cb2bc..dc81fdc01dda 100644 >>>>> --- a/include/linux/dma-buf.h >>>>> +++ b/include/linux/dma-buf.h >>>>> @@ -267,7 +267,52 @@ struct dma_buf_ops { >>>>> */ >>>>> int (*mmap)(struct dma_buf *, struct vm_area_struct *vma); >>>>> + /** >>>>> + * @vmap: >>>> >>>> There's already a @vmap and @vunamp kerneldoc at the top comment, that >>>> needs to be removed. >>>> -Daniel >>>> >>>>> + * >>>>> + * Returns a virtual address for the buffer. >>>>> + * >>>>> + * Notes to callers: >>>>> + * >>>>> + * - Callers must hold the struct dma_buf.resv lock before >>>>> calling >>>>> + * this interface. >>>>> + * >>>>> + * - Callers must provide means to prevent the mappings from >>>>> going >>>>> + * stale, such as holding the reservation lock or providing a >>>>> + * move-notify callback to the exporter. >>>>> + * >>>>> + * Notes to implementors: >>>>> + * >>>>> + * - Implementations must expect pairs of @vmap and @vunmap >>>>> to be >>>>> + * called frequently and should optimize for this case. >>>>> + * >>>>> + * - Implementations should avoid additional operations, such as >>>>> + * pinning. >>>>> + * >>>>> + * - Implementations may expect the caller to hold the dma-buf's >>>>> + * reservation lock to protect against concurrent calls and >>>>> + * relocation. >>>>> + * >>>>> + * - Implementations may provide additional guarantees, such >>>>> as working >>>>> + * without holding the reservation lock. >>>>> + * >>>>> + * This callback is optional. >>>>> + * >>>>> + * Returns: >>>>> + * >>>>> + * 0 on success or a negative error code on failure. >>>>> + */ >>>>> int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); >>>>> + >>>>> + /** >>>>> + * @vunmap: >>>>> + * >>>>> + * Releases the address previously returned by @vmap. >>>>> + * >>>>> + * This callback is optional. >>>>> + * >>>>> + * See also @vmap() >>>>> + */ >>>>> void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map >>>>> *map); >>>>> }; >>>>> -- >>>>> 2.29.2 >>>>> >>>> >>> >>> -- >>> Thomas Zimmermann >>> Graphics Driver Developer >>> SUSE Software Solutions Germany GmbH >>> Maxfeldstr. 5, 90409 Nürnberg, Germany >>> (HRB 36809, AG Nürnberg) >>> Geschäftsführer: Felix Imendörffer >>> >> >> >> >> >

5 years

Re: [Linaro-mm-sig] [PATCH v2 7/7] dma-buf: Write down some rules for vmap usage

by Daniel Vetter

On Thu, Dec 03, 2020 at 07:59:04PM +0100, Thomas Zimmermann wrote: > Hi > > Am 03.12.20 um 16:26 schrieb Daniel Vetter: > > On Thu, Dec 03, 2020 at 03:02:59PM +0100, Thomas Zimmermann wrote: > > > Dma-buf's vmap and vunmap callbacks are undocumented and various > > > exporters currently have slightly different semantics for them. Add > > > documentation on how to implement and use these interfaces correctly. > > > > > > v2: > > > * document vmap semantics in struct dma_buf_ops > > > * add TODO item for reviewing and maybe fixing dma-buf exporters > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> > > > > I still don't think this works, we're breaking dma_buf_vmap for everyone > > else here. > > I removed the text on the importer. These notes for callers in the docs are > more or less a consequence of the exporter semantics. Callers are importers, so I'm not seeing how that fixes anything. > I thought we at least agreed on the exporter semantics in the other thread, > didn't we? > > What I'm trying to do is to write dome some rules for exporters, even if not > all exporters follow them. This is a standard interface, everyone needs to follow the same rules. And if they change, we need to make sure nothing breaks and we're not creating issues. In the past the rule was the dma_buf_vmap was allowed to take the dma_resv_lock, and that the buffer should be pinned. Now some ttm drivers didn't ever bother with the pinning, and mostly got away with that because drm_prime helpers do the pinning by default at attach time, and most users do call dma_buf_attach. But if you look through dma-buf docs nothing ever said you have to do a dummy attachment before you call dma_buf_vmap, that's just slightly crappy implementations that didn't blow up yet. > Given the circumstances, we should leave out this patch from the patchset. So the defacto rules are already a big mess, but that's not a good excuse to make it worse. What I had in mind is that we split dma_buf_vmap up into two variants: - The current one, which should guarantee that the buffer is pinned. Because that's what all current callers wanted, before the fbdev code started allowing non-pinned buffers. - The new one, which allows vmapping with just dma_resv locked, and should have some caching in exporters. Breaking code and then adding todos about that is kinda not so cool approach here imo. Also I guess ttm_bo_vmap should have a check that either the buffer is pinned, or dma_resv_lock is held. Cheers, Daniel > > Best regards > Thomas > > > > > > --- > > > Documentation/gpu/todo.rst | 15 +++++++++++++ > > > include/drm/drm_gem.h | 4 ++++ > > > include/linux/dma-buf.h | 45 ++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 64 insertions(+) > > > > > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > > > index 009d8e6c7e3c..32bb797a84fc 100644 > > > --- a/Documentation/gpu/todo.rst > > > +++ b/Documentation/gpu/todo.rst > > > @@ -505,6 +505,21 @@ Contact: Thomas Zimmermann <tzimmermann(a)suse.de>, Christian König, Daniel Vette > > > Level: Intermediate > > > +Enforce rules for dma-buf vmap and pin ops > > > +------------------------------------------ > > > + > > > +Exporter implementations of vmap and pin in struct dma_buf_ops (and consequently > > > +struct drm_gem_object_funcs) use a variety of locking semantics. Some rely on > > > +the caller holding the dma-buf's reservation lock, some do their own locking, > > > +some don't require any locking. VRAM helpers even used to pin as part of vmap. > > > + > > > +We need to review each exporter and enforce the documented rules. > > > + > > > +Contact: Christian König, Daniel Vetter, Thomas Zimmermann <tzimmermann(a)suse.de> > > > + > > > +Level: Advanced > > > + > > > + > > > Core refactorings > > > ================= > > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > > > index 5e6daa1c982f..1864c6a721b1 100644 > > > --- a/include/drm/drm_gem.h > > > +++ b/include/drm/drm_gem.h > > > @@ -138,6 +138,8 @@ struct drm_gem_object_funcs { > > > * drm_gem_dmabuf_vmap() helper. > > > * > > > * This callback is optional. > > > + * > > > + * See also struct dma_buf_ops.vmap > > > */ > > > int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map); > > > @@ -148,6 +150,8 @@ struct drm_gem_object_funcs { > > > * drm_gem_dmabuf_vunmap() helper. > > > * > > > * This callback is optional. > > > + * > > > + * See also struct dma_buf_ops.vunmap > > > */ > > > void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map *map); > > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > > > index cf72699cb2bc..dc81fdc01dda 100644 > > > --- a/include/linux/dma-buf.h > > > +++ b/include/linux/dma-buf.h > > > @@ -267,7 +267,52 @@ struct dma_buf_ops { > > > */ > > > int (*mmap)(struct dma_buf *, struct vm_area_struct *vma); > > > + /** > > > + * @vmap: > > > > There's already a @vmap and @vunamp kerneldoc at the top comment, that > > needs to be removed. > > -Daniel > > > > > + * > > > + * Returns a virtual address for the buffer. > > > + * > > > + * Notes to callers: > > > + * > > > + * - Callers must hold the struct dma_buf.resv lock before calling > > > + * this interface. > > > + * > > > + * - Callers must provide means to prevent the mappings from going > > > + * stale, such as holding the reservation lock or providing a > > > + * move-notify callback to the exporter. > > > + * > > > + * Notes to implementors: > > > + * > > > + * - Implementations must expect pairs of @vmap and @vunmap to be > > > + * called frequently and should optimize for this case. > > > + * > > > + * - Implementations should avoid additional operations, such as > > > + * pinning. > > > + * > > > + * - Implementations may expect the caller to hold the dma-buf's > > > + * reservation lock to protect against concurrent calls and > > > + * relocation. > > > + * > > > + * - Implementations may provide additional guarantees, such as working > > > + * without holding the reservation lock. > > > + * > > > + * This callback is optional. > > > + * > > > + * Returns: > > > + * > > > + * 0 on success or a negative error code on failure. > > > + */ > > > int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > > > + > > > + /** > > > + * @vunmap: > > > + * > > > + * Releases the address previously returned by @vmap. > > > + * > > > + * This callback is optional. > > > + * > > > + * See also @vmap() > > > + */ > > > void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > > > }; > > > -- > > > 2.29.2 > > > > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

Re: [Linaro-mm-sig] [PATCH v2 7/7] dma-buf: Write down some rules for vmap usage

by Daniel Vetter

On Thu, Dec 03, 2020 at 03:02:59PM +0100, Thomas Zimmermann wrote: > Dma-buf's vmap and vunmap callbacks are undocumented and various > exporters currently have slightly different semantics for them. Add > documentation on how to implement and use these interfaces correctly. > > v2: > * document vmap semantics in struct dma_buf_ops > * add TODO item for reviewing and maybe fixing dma-buf exporters > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> I still don't think this works, we're breaking dma_buf_vmap for everyone else here. > --- > Documentation/gpu/todo.rst | 15 +++++++++++++ > include/drm/drm_gem.h | 4 ++++ > include/linux/dma-buf.h | 45 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 64 insertions(+) > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > index 009d8e6c7e3c..32bb797a84fc 100644 > --- a/Documentation/gpu/todo.rst > +++ b/Documentation/gpu/todo.rst > @@ -505,6 +505,21 @@ Contact: Thomas Zimmermann <tzimmermann(a)suse.de>, Christian König, Daniel Vette > Level: Intermediate > > > +Enforce rules for dma-buf vmap and pin ops > +------------------------------------------ > + > +Exporter implementations of vmap and pin in struct dma_buf_ops (and consequently > +struct drm_gem_object_funcs) use a variety of locking semantics. Some rely on > +the caller holding the dma-buf's reservation lock, some do their own locking, > +some don't require any locking. VRAM helpers even used to pin as part of vmap. > + > +We need to review each exporter and enforce the documented rules. > + > +Contact: Christian König, Daniel Vetter, Thomas Zimmermann <tzimmermann(a)suse.de> > + > +Level: Advanced > + > + > Core refactorings > ================= > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > index 5e6daa1c982f..1864c6a721b1 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -138,6 +138,8 @@ struct drm_gem_object_funcs { > * drm_gem_dmabuf_vmap() helper. > * > * This callback is optional. > + * > + * See also struct dma_buf_ops.vmap > */ > int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map); > > @@ -148,6 +150,8 @@ struct drm_gem_object_funcs { > * drm_gem_dmabuf_vunmap() helper. > * > * This callback is optional. > + * > + * See also struct dma_buf_ops.vunmap > */ > void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map *map); > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index cf72699cb2bc..dc81fdc01dda 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -267,7 +267,52 @@ struct dma_buf_ops { > */ > int (*mmap)(struct dma_buf *, struct vm_area_struct *vma); > > + /** > + * @vmap: There's already a @vmap and @vunamp kerneldoc at the top comment, that needs to be removed. -Daniel > + * > + * Returns a virtual address for the buffer. > + * > + * Notes to callers: > + * > + * - Callers must hold the struct dma_buf.resv lock before calling > + * this interface. > + * > + * - Callers must provide means to prevent the mappings from going > + * stale, such as holding the reservation lock or providing a > + * move-notify callback to the exporter. > + * > + * Notes to implementors: > + * > + * - Implementations must expect pairs of @vmap and @vunmap to be > + * called frequently and should optimize for this case. > + * > + * - Implementations should avoid additional operations, such as > + * pinning. > + * > + * - Implementations may expect the caller to hold the dma-buf's > + * reservation lock to protect against concurrent calls and > + * relocation. > + * > + * - Implementations may provide additional guarantees, such as working > + * without holding the reservation lock. > + * > + * This callback is optional. > + * > + * Returns: > + * > + * 0 on success or a negative error code on failure. > + */ > int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > + > + /** > + * @vunmap: > + * > + * Releases the address previously returned by @vmap. > + * > + * This callback is optional. > + * > + * See also @vmap() > + */ > void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > }; > > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

Re: [Linaro-mm-sig] [PATCH v2 2/4] mm: introduce cma_alloc_bulk API

by David Hildenbrand

On 03.12.20 12:47, Michal Hocko wrote: > On Thu 03-12-20 10:47:02, David Hildenbrand wrote: >> On 03.12.20 09:28, Michal Hocko wrote: > [...] >>> I think we should aim at easy and very highlevel behavior: >>> - GFP_NOWAIT - unsupported currently IIRC but something that something >>> that should be possible to implement. Isolation is non blocking, >>> migration could be skipped >>> - GFP_KERNEL - default behavior whatever that means >>> - GFP_NORETRY - opportunistic allocation as lightweight as we can get. >>> Failures to be expected also for transient reasons. >>> - GFP_RETRY_MAYFAIL - try hard but not as hard as to trigger disruption >>> (e.g. via oom killer). >> >> I think we currently see demand for 3 modes for alloc_contig_range() >> >> a) normal >> >> As is. Try, but don't try too hard. E.g., drain LRU, drain PCP, retry a >> couple of times. Failures in some cases (short-term pinning, PCP races) >> are still possible and acceptable. >> >> GFP_RETRY_MAYFAIL ? > > normal shouldn't really require anybody to think about gfp flags hard. > That to most people really means GFP_KERNEL. > >> E.g., "Allocations with this flag may fail, but only when there is >> genuinely little unused memory." - current description does not match at >> all. When allocating ranges things behave completely different. >> >> >> b) fast >> >> Try, but fail fast. Leave optimizations that can improve the result to >> the caller. E.g., don't drain LRU, don't drain PCP, don't retry. >> Frequent failures are expected and acceptable. >> >> __GFP_NORETRY ? >> >> E.g., "The VM implementation will try only very lightweight memory >> direct reclaim to get some memory under memory pressure" - again, I >> think current description does not really match. > > Agreed. As mentioned above this would be an opportunistic allocation > mode. > > >> c) hard >> >> Try hard, E.g., temporarily disabling the PCP. Certainly not >> __GFP_NOFAIL, that would be highly dangerous. So no flags / GFP_KERNEL? > > NOFAIL semantic is out of question. Should we have a mode to try harder > than the default? I dunno. Do we have users? I think RETRY_MAYFAIL is a > middle ground between the default and NORETRY which is just too easy to > fail. This is the case for the allocator as well. And from what I have > seen people are already using MAYFAIL in order to prevent oom killer so > this is a generally recognized pattern. virtio-mem might be one user. It might first try in normal mode to get as much memory out as possible, but switch to hard mode when it might make sense. > >>> - __GFP_THIS_NODE - stick to a node without fallback >>> - we can support zone modifiers although there is no existing user. >>> - __GFP_NOWARN - obvious >>> >>> And that is it. Or maybe I am seeing that oversimplified. >>> >> >> Again, I think most flags make sense for the migration target allocation >> path and mainly deal with OOM situations and reclaim. For the migration >> path - which is specific to the alloc_contig_range() allocater - they >> don't really apply and create more confusion than they actually help - IMHO. > > Migration is really an implementation detail of this interface. You > shouldn't be even thinking that there is a migration underneath not even > mention to actually trying to control it. CMA? I tend to agree. alloc_contig_range? I disagree. -- Thanks, David / dhildenb

5 years

Re: [Linaro-mm-sig] [PATCH v2 2/4] mm: introduce cma_alloc_bulk API

by David Hildenbrand

On 03.12.20 09:28, Michal Hocko wrote: > On Wed 02-12-20 21:22:36, David Hildenbrand wrote: >> On 02.12.20 20:26, Minchan Kim wrote: >>> On Wed, Dec 02, 2020 at 07:51:07PM +0100, Michal Hocko wrote: > [...] >>>> I am still not sure a specific flag is a good interface. Really can this >>>> be gfp_mask instead? >>> >>> I am not strong(even, I did it with GFP_NORETRY) but David wanted to >>> have special mode and I agreed when he mentioned ALLOC_CONTIG_HARD as >>> one of options in future(it would be hard to indicate that mode with >>> gfp flags). >> >> I can't tell regarding the CMA interface, but for the alloc_contig() >> interface I think modes make sense. Yes, it's different to other >> allocaters, but the contig range allocater is different already. E.g., >> the CMA allocater mostly hides "which exact PFNs you try to allocate". > > Yes, alloc_contig_range is a low level API but it already has a gfp_mask > parameter. Adding yet another allocation mode sounds like API > convolution to me. Well, if another parameter is a concern, we can introduce alloc_contig_range_fast() instead. > >> In the contig range allocater, gfp flags are currently used to express >> how to allocate pages used as migration targets. I don't think mangling >> in other gfp flags (or even overloading them) makes things a lot >> clearer. E.g., GFP_NORETRY: don't retry to allocate migration targets? >> don't retry to migrate pages? both? >> >> As I said, other aspects might be harder to model (e.g., don't drain >> LRU) and hiding them behind generic gfp flags (e.g., GFP_NORETRY) feels >> wrong. >> >> With the mode, we're expressing details for the necessary page >> migration. Suggestions on how to model that are welcome. > > The question is whether the caller should really have such an intimate > knowledge and control of the alloc_contig_range implementation. This all > are implementation details. Should really anybody think about how many > times migration retries or control LRU draining? Those can change in the The question is not "how many times", rather "if at all". I can understand the possible performance improvements by letting the caller handle things (lru draining, pcp draining) like that when issuing gazillions of alloc_contig_range() calls. > future and I do not think we really want to go over all users grown over > that time and try to deduce what was the intention behind. That's why I think we need a clear mechanism to express the expected behavior - something we can properly document and users can actually understand to optimize for - and we can fix them up when the documented behavior changes. Mangling this into somewhat-fitting gfp flags makes the interface harder to use and more error-prone IMHO. > > I think we should aim at easy and very highlevel behavior: > - GFP_NOWAIT - unsupported currently IIRC but something that something > that should be possible to implement. Isolation is non blocking, > migration could be skipped > - GFP_KERNEL - default behavior whatever that means > - GFP_NORETRY - opportunistic allocation as lightweight as we can get. > Failures to be expected also for transient reasons. > - GFP_RETRY_MAYFAIL - try hard but not as hard as to trigger disruption > (e.g. via oom killer). I think we currently see demand for 3 modes for alloc_contig_range() a) normal As is. Try, but don't try too hard. E.g., drain LRU, drain PCP, retry a couple of times. Failures in some cases (short-term pinning, PCP races) are still possible and acceptable. GFP_RETRY_MAYFAIL ? E.g., "Allocations with this flag may fail, but only when there is genuinely little unused memory." - current description does not match at all. When allocating ranges things behave completely different. b) fast Try, but fail fast. Leave optimizations that can improve the result to the caller. E.g., don't drain LRU, don't drain PCP, don't retry. Frequent failures are expected and acceptable. __GFP_NORETRY ? E.g., "The VM implementation will try only very lightweight memory direct reclaim to get some memory under memory pressure" - again, I think current description does not really match. c) hard Try hard, E.g., temporarily disabling the PCP. Certainly not __GFP_NOFAIL, that would be highly dangerous. So no flags / GFP_KERNEL? > > - __GFP_THIS_NODE - stick to a node without fallback > - we can support zone modifiers although there is no existing user. > - __GFP_NOWARN - obvious > > And that is it. Or maybe I am seeing that oversimplified. > Again, I think most flags make sense for the migration target allocation path and mainly deal with OOM situations and reclaim. For the migration path - which is specific to the alloc_contig_range() allocater - they don't really apply and create more confusion than they actually help - IMHO. -- Thanks, David / dhildenb

5 years

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig