Hi Dave, On Mon, 2025-09-01 at 14:56 +1000, Dave Airlie wrote: > From: Dave Airlie <[email protected]> > > While discussing cgroups we noticed a problem where you could export > a BO to a dma-buf without having it ever being backed or accounted > for. > > This meant in low memory situations or eventually with cgroups, a > lower privledged process might cause the compositor to try and > allocate > a lot of memory on it's behalf and this could fail. At least make > sure the exporter has managed to allocate the RAM at least once > before exporting the object.
With a shmem analogy, let's say process A creates a shmem object and doesn't populate it. It's then shared with process B which faults in the pages or otherwise causes it to be populated. Will this patch ensure we behave the same WRT memory usage accounting? Also some concerns below. > > This only applies currently to TTM_PL_SYSTEM objects, because > GTT objects get populated on first validate, and VRAM doesn't > use TT. > > TODO: > other drivers? > split this into two? > > Cc: Christian Koenig <[email protected]> > Cc: Thomas Hellström <[email protected]> > Cc: Simona Vetter <[email protected]> > Signed-off-by: Dave Airlie <[email protected]> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 5 ++++ > drivers/gpu/drm/ttm/ttm_bo.c | 28 > +++++++++++++++++++++ > include/drm/ttm/ttm_bo.h | 1 + > 3 files changed, 34 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > index ce27cb5bb05e..d0f578d0ae6c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > @@ -343,11 +343,16 @@ struct dma_buf *amdgpu_gem_prime_export(struct > drm_gem_object *gobj, > { > struct amdgpu_bo *bo = gem_to_amdgpu_bo(gobj); > struct dma_buf *buf; > + int ret; > > if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) || > bo->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID) > return ERR_PTR(-EPERM); > > + ret = ttm_bo_setup_export(&bo->tbo); > + if (ret) > + return ERR_PTR(ret); > + > buf = drm_gem_prime_export(gobj, flags); > if (!IS_ERR(buf)) > buf->ops = &amdgpu_dmabuf_ops; > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c > b/drivers/gpu/drm/ttm/ttm_bo.c > index 273757974b9f..bf98e28a8bda 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -1284,3 +1284,31 @@ int ttm_bo_populate(struct ttm_buffer_object > *bo, > return 0; > } > EXPORT_SYMBOL(ttm_bo_populate); > + > +int ttm_bo_setup_export(struct ttm_buffer_object *bo) > +{ > + struct ttm_operation_ctx ctx = { > + .interruptible = false, > + .no_wait_gpu = true, > + /* We opt to avoid OOM on system pages allocations > */ > + .gfp_retry_mayfail = true, > + .allow_res_evict = false, > + }; I think we'd want to have the caller (driver) provide the ttm_operation_ctx. The driver may want to subclass or call interruptible. > + > + int ret; > + > + if (!bo->ttm) > + return 0; > + > + if (bo->ttm && ttm_tt_is_populated(bo->ttm)) > + return 0; > + bo->ttm is not safe to reference without the bo lock. Nor is the ttm_tt_is_populated stable without the bo lock since you may race with a swapout / shrink. Thanks, Thomas > + ret = ttm_bo_reserve(bo, false, false, NULL); > + if (ret != 0) > + return ret; > + > + ret = ttm_bo_populate(bo, bo->resource->placement & > TTM_PL_FLAG_MEMCG, &ctx); > + ttm_bo_unreserve(bo); > + return ret; > +} > +EXPORT_SYMBOL(ttm_bo_setup_export); > diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h > index c33b3667ae76..5cdd89da9ef5 100644 > --- a/include/drm/ttm/ttm_bo.h > +++ b/include/drm/ttm/ttm_bo.h > @@ -473,6 +473,7 @@ void ttm_bo_tt_destroy(struct ttm_buffer_object > *bo); > int ttm_bo_populate(struct ttm_buffer_object *bo, > bool memcg_account, > struct ttm_operation_ctx *ctx); > +int ttm_bo_setup_export(struct ttm_buffer_object *bo); > > /* Driver LRU walk helpers initially targeted for shrinking. */ >
