Re: [RFC PATCH v3 11/11] drm, cgroup: Allow more aggressive memory reclaim
On Wed, Jun 26, 2019 at 06:52:50PM -0400, Kenny Ho wrote:
> Ok. I am not too familiar with shrinker but I will dig into it. Just
> so that I am looking into the right things, you are referring to
> things like struct shrinker and struct shrink_control?
Yeah. Reason I'm asking for this is this is how system memory is shrunk
right now, so at least having some conceptual similarities might be useful
here. And a lot of people have thought quite hard about system memory
shrinking and all that, so hopefully that gives us good design
inspiration.
-Daniel
>
> Regards,
> Kenny
>
> On Wed, Jun 26, 2019 at 12:44 PM Daniel Vetter wrote:
> >
> > On Wed, Jun 26, 2019 at 11:05:22AM -0400, Kenny Ho wrote:
> > > Allow DRM TTM memory manager to register a work_struct, such that, when
> > > a drmcgrp is under memory pressure, memory reclaiming can be triggered
> > > immediately.
> > >
> > > Change-Id: I25ac04e2db9c19ff12652b88ebff18b44b2706d8
> > > Signed-off-by: Kenny Ho
> > > ---
> > > drivers/gpu/drm/ttm/ttm_bo.c| 47 +
> > > include/drm/drm_cgroup.h| 14 ++
> > > include/drm/ttm/ttm_bo_driver.h | 2 ++
> > > kernel/cgroup/drm.c | 33 +++
> > > 4 files changed, 96 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> > > index 79c530f4a198..5fc3bc5bd4c5 100644
> > > --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > > @@ -1509,6 +1509,44 @@ int ttm_bo_evict_mm(struct ttm_bo_device *bdev,
> > > unsigned mem_type)
> > > }
> > > EXPORT_SYMBOL(ttm_bo_evict_mm);
> > >
> > > +static void ttm_bo_reclaim_wq(struct work_struct *work)
> > > +{
> >
> > I think a design a bit more inspired by memcg aware core shrinkers would
> > be nice, i.e. explicitly passing:
> > - which drm_cgroup needs to be shrunk
> > - which ttm_mem_reg (well the fancy new abstracted out stuff for tracking
> > special gpu memory resources like tt or vram or whatever)
> > - how much it needs to be shrunk
> >
> > I think with that a lot more the book-keeping could be pushed into the
> > drm_cgroup code, and the callback just needs to actually shrink enough as
> > requested.
> > -Daniel
> >
> > > + struct ttm_operation_ctx ctx = {
> > > + .interruptible = false,
> > > + .no_wait_gpu = false,
> > > + .flags = TTM_OPT_FLAG_FORCE_ALLOC
> > > + };
> > > + struct ttm_mem_type_manager *man =
> > > + container_of(work, struct ttm_mem_type_manager, reclaim_wq);
> > > + struct ttm_bo_device *bdev = man->bdev;
> > > + struct dma_fence *fence;
> > > + int mem_type;
> > > + int ret;
> > > +
> > > + for (mem_type = 0; mem_type < TTM_NUM_MEM_TYPES; mem_type++)
> > > + if (>man[mem_type] == man)
> > > + break;
> > > +
> > > + BUG_ON(mem_type >= TTM_NUM_MEM_TYPES);
> > > +
> > > + if (!drmcgrp_mem_pressure_scan(bdev, mem_type))
> > > + return;
> > > +
> > > + ret = ttm_mem_evict_first(bdev, mem_type, NULL, );
> > > + if (ret)
> > > + return;
> > > +
> > > + spin_lock(>move_lock);
> > > + fence = dma_fence_get(man->move);
> > > + spin_unlock(>move_lock);
> > > +
> > > + if (fence) {
> > > + ret = dma_fence_wait(fence, false);
> > > + dma_fence_put(fence);
> > > + }
> > > +
> > > +}
> > > +
> > > int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type,
> > > unsigned long p_size)
> > > {
> > > @@ -1543,6 +1581,13 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev,
> > > unsigned type,
> > > INIT_LIST_HEAD(>lru[i]);
> > > man->move = NULL;
> > >
> > > + pr_err("drmcgrp %p type %d\n", bdev->ddev, type);
> > > +
> > > + if (type <= TTM_PL_VRAM) {
> > > + INIT_WORK(>reclaim_wq, ttm_bo_reclaim_wq);
> > > + drmcgrp_register_device_mm(bdev->ddev, type,
> > > >reclaim_wq);
> > > + }
> > > +
> > > return 0;
> > > }
> > > EXPORT_SYMBOL(ttm_bo_init_mm);
> > > @@ -1620,6 +1665,8 @@ int ttm_bo_device_release(struct ttm_bo_device
> > > *bdev)
> > > man = >man[i];
> > > if (man->has_type) {
> > > man->use_type = false;
> > > + drmcgrp_unregister_device_mm(bdev->ddev, i);
> > > + cancel_work_sync(>reclaim_wq);
> > > if ((i != TTM_PL_SYSTEM) && ttm_bo_clean_mm(bdev,
> > > i)) {
> > > ret = -EBUSY;
> > > pr_err("DRM memory manager type %d is not
> > > clean\n",
> > > diff --git a/include/drm/drm_cgroup.h b/include/drm/drm_cgroup.h
> > > index 360c1e6c809f..134d6e5475f3 100644
> > > --- a/include/drm/drm_cgroup.h
> > > +++ b/include/drm/drm_cgroup.h
> > > @@ -5,6 +5,7 @@
> > > #define __DRM_CGROUP_H__
> > >
> > > #include
> > > +#include
> > > #include
> > > #include
Re: [RFC PATCH v3 11/11] drm, cgroup: Allow more aggressive memory reclaim
Ok. I am not too familiar with shrinker but I will dig into it. Just
so that I am looking into the right things, you are referring to
things like struct shrinker and struct shrink_control?
Regards,
Kenny
On Wed, Jun 26, 2019 at 12:44 PM Daniel Vetter wrote:
>
> On Wed, Jun 26, 2019 at 11:05:22AM -0400, Kenny Ho wrote:
> > Allow DRM TTM memory manager to register a work_struct, such that, when
> > a drmcgrp is under memory pressure, memory reclaiming can be triggered
> > immediately.
> >
> > Change-Id: I25ac04e2db9c19ff12652b88ebff18b44b2706d8
> > Signed-off-by: Kenny Ho
> > ---
> > drivers/gpu/drm/ttm/ttm_bo.c| 47 +
> > include/drm/drm_cgroup.h| 14 ++
> > include/drm/ttm/ttm_bo_driver.h | 2 ++
> > kernel/cgroup/drm.c | 33 +++
> > 4 files changed, 96 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> > index 79c530f4a198..5fc3bc5bd4c5 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > @@ -1509,6 +1509,44 @@ int ttm_bo_evict_mm(struct ttm_bo_device *bdev,
> > unsigned mem_type)
> > }
> > EXPORT_SYMBOL(ttm_bo_evict_mm);
> >
> > +static void ttm_bo_reclaim_wq(struct work_struct *work)
> > +{
>
> I think a design a bit more inspired by memcg aware core shrinkers would
> be nice, i.e. explicitly passing:
> - which drm_cgroup needs to be shrunk
> - which ttm_mem_reg (well the fancy new abstracted out stuff for tracking
> special gpu memory resources like tt or vram or whatever)
> - how much it needs to be shrunk
>
> I think with that a lot more the book-keeping could be pushed into the
> drm_cgroup code, and the callback just needs to actually shrink enough as
> requested.
> -Daniel
>
> > + struct ttm_operation_ctx ctx = {
> > + .interruptible = false,
> > + .no_wait_gpu = false,
> > + .flags = TTM_OPT_FLAG_FORCE_ALLOC
> > + };
> > + struct ttm_mem_type_manager *man =
> > + container_of(work, struct ttm_mem_type_manager, reclaim_wq);
> > + struct ttm_bo_device *bdev = man->bdev;
> > + struct dma_fence *fence;
> > + int mem_type;
> > + int ret;
> > +
> > + for (mem_type = 0; mem_type < TTM_NUM_MEM_TYPES; mem_type++)
> > + if (>man[mem_type] == man)
> > + break;
> > +
> > + BUG_ON(mem_type >= TTM_NUM_MEM_TYPES);
> > +
> > + if (!drmcgrp_mem_pressure_scan(bdev, mem_type))
> > + return;
> > +
> > + ret = ttm_mem_evict_first(bdev, mem_type, NULL, );
> > + if (ret)
> > + return;
> > +
> > + spin_lock(>move_lock);
> > + fence = dma_fence_get(man->move);
> > + spin_unlock(>move_lock);
> > +
> > + if (fence) {
> > + ret = dma_fence_wait(fence, false);
> > + dma_fence_put(fence);
> > + }
> > +
> > +}
> > +
> > int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type,
> > unsigned long p_size)
> > {
> > @@ -1543,6 +1581,13 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev,
> > unsigned type,
> > INIT_LIST_HEAD(>lru[i]);
> > man->move = NULL;
> >
> > + pr_err("drmcgrp %p type %d\n", bdev->ddev, type);
> > +
> > + if (type <= TTM_PL_VRAM) {
> > + INIT_WORK(>reclaim_wq, ttm_bo_reclaim_wq);
> > + drmcgrp_register_device_mm(bdev->ddev, type,
> > >reclaim_wq);
> > + }
> > +
> > return 0;
> > }
> > EXPORT_SYMBOL(ttm_bo_init_mm);
> > @@ -1620,6 +1665,8 @@ int ttm_bo_device_release(struct ttm_bo_device *bdev)
> > man = >man[i];
> > if (man->has_type) {
> > man->use_type = false;
> > + drmcgrp_unregister_device_mm(bdev->ddev, i);
> > + cancel_work_sync(>reclaim_wq);
> > if ((i != TTM_PL_SYSTEM) && ttm_bo_clean_mm(bdev, i))
> > {
> > ret = -EBUSY;
> > pr_err("DRM memory manager type %d is not
> > clean\n",
> > diff --git a/include/drm/drm_cgroup.h b/include/drm/drm_cgroup.h
> > index 360c1e6c809f..134d6e5475f3 100644
> > --- a/include/drm/drm_cgroup.h
> > +++ b/include/drm/drm_cgroup.h
> > @@ -5,6 +5,7 @@
> > #define __DRM_CGROUP_H__
> >
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -12,6 +13,9 @@
> >
> > int drmcgrp_register_device(struct drm_device *device);
> > int drmcgrp_unregister_device(struct drm_device *device);
> > +void drmcgrp_register_device_mm(struct drm_device *dev, unsigned type,
> > + struct work_struct *wq);
> > +void drmcgrp_unregister_device_mm(struct drm_device *dev, unsigned type);
> > bool drmcgrp_is_self_or_ancestor(struct drmcgrp *self,
> > struct drmcgrp *relative);
> > void drmcgrp_chg_bo_alloc(struct drmcgrp *drmcgrp, struct drm_device *dev,
> > @@ -40,6 +44,16 @@ static inline int drmcgrp_unregister_device(struct
> >
Re: [RFC PATCH v3 11/11] drm, cgroup: Allow more aggressive memory reclaim
On Wed, Jun 26, 2019 at 11:05:22AM -0400, Kenny Ho wrote:
> Allow DRM TTM memory manager to register a work_struct, such that, when
> a drmcgrp is under memory pressure, memory reclaiming can be triggered
> immediately.
>
> Change-Id: I25ac04e2db9c19ff12652b88ebff18b44b2706d8
> Signed-off-by: Kenny Ho
> ---
> drivers/gpu/drm/ttm/ttm_bo.c| 47 +
> include/drm/drm_cgroup.h| 14 ++
> include/drm/ttm/ttm_bo_driver.h | 2 ++
> kernel/cgroup/drm.c | 33 +++
> 4 files changed, 96 insertions(+)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 79c530f4a198..5fc3bc5bd4c5 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -1509,6 +1509,44 @@ int ttm_bo_evict_mm(struct ttm_bo_device *bdev,
> unsigned mem_type)
> }
> EXPORT_SYMBOL(ttm_bo_evict_mm);
>
> +static void ttm_bo_reclaim_wq(struct work_struct *work)
> +{
I think a design a bit more inspired by memcg aware core shrinkers would
be nice, i.e. explicitly passing:
- which drm_cgroup needs to be shrunk
- which ttm_mem_reg (well the fancy new abstracted out stuff for tracking
special gpu memory resources like tt or vram or whatever)
- how much it needs to be shrunk
I think with that a lot more the book-keeping could be pushed into the
drm_cgroup code, and the callback just needs to actually shrink enough as
requested.
-Daniel
> + struct ttm_operation_ctx ctx = {
> + .interruptible = false,
> + .no_wait_gpu = false,
> + .flags = TTM_OPT_FLAG_FORCE_ALLOC
> + };
> + struct ttm_mem_type_manager *man =
> + container_of(work, struct ttm_mem_type_manager, reclaim_wq);
> + struct ttm_bo_device *bdev = man->bdev;
> + struct dma_fence *fence;
> + int mem_type;
> + int ret;
> +
> + for (mem_type = 0; mem_type < TTM_NUM_MEM_TYPES; mem_type++)
> + if (>man[mem_type] == man)
> + break;
> +
> + BUG_ON(mem_type >= TTM_NUM_MEM_TYPES);
> +
> + if (!drmcgrp_mem_pressure_scan(bdev, mem_type))
> + return;
> +
> + ret = ttm_mem_evict_first(bdev, mem_type, NULL, );
> + if (ret)
> + return;
> +
> + spin_lock(>move_lock);
> + fence = dma_fence_get(man->move);
> + spin_unlock(>move_lock);
> +
> + if (fence) {
> + ret = dma_fence_wait(fence, false);
> + dma_fence_put(fence);
> + }
> +
> +}
> +
> int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type,
> unsigned long p_size)
> {
> @@ -1543,6 +1581,13 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev,
> unsigned type,
> INIT_LIST_HEAD(>lru[i]);
> man->move = NULL;
>
> + pr_err("drmcgrp %p type %d\n", bdev->ddev, type);
> +
> + if (type <= TTM_PL_VRAM) {
> + INIT_WORK(>reclaim_wq, ttm_bo_reclaim_wq);
> + drmcgrp_register_device_mm(bdev->ddev, type, >reclaim_wq);
> + }
> +
> return 0;
> }
> EXPORT_SYMBOL(ttm_bo_init_mm);
> @@ -1620,6 +1665,8 @@ int ttm_bo_device_release(struct ttm_bo_device *bdev)
> man = >man[i];
> if (man->has_type) {
> man->use_type = false;
> + drmcgrp_unregister_device_mm(bdev->ddev, i);
> + cancel_work_sync(>reclaim_wq);
> if ((i != TTM_PL_SYSTEM) && ttm_bo_clean_mm(bdev, i)) {
> ret = -EBUSY;
> pr_err("DRM memory manager type %d is not
> clean\n",
> diff --git a/include/drm/drm_cgroup.h b/include/drm/drm_cgroup.h
> index 360c1e6c809f..134d6e5475f3 100644
> --- a/include/drm/drm_cgroup.h
> +++ b/include/drm/drm_cgroup.h
> @@ -5,6 +5,7 @@
> #define __DRM_CGROUP_H__
>
> #include
> +#include
> #include
> #include
>
> @@ -12,6 +13,9 @@
>
> int drmcgrp_register_device(struct drm_device *device);
> int drmcgrp_unregister_device(struct drm_device *device);
> +void drmcgrp_register_device_mm(struct drm_device *dev, unsigned type,
> + struct work_struct *wq);
> +void drmcgrp_unregister_device_mm(struct drm_device *dev, unsigned type);
> bool drmcgrp_is_self_or_ancestor(struct drmcgrp *self,
> struct drmcgrp *relative);
> void drmcgrp_chg_bo_alloc(struct drmcgrp *drmcgrp, struct drm_device *dev,
> @@ -40,6 +44,16 @@ static inline int drmcgrp_unregister_device(struct
> drm_device *device)
> return 0;
> }
>
> +static inline void drmcgrp_register_device_mm(struct drm_device *dev,
> + unsigned type, struct work_struct *wq)
> +{
> +}
> +
> +static inline void drmcgrp_unregister_device_mm(struct drm_device *dev,
> + unsigned type)
> +{
> +}
> +
> static inline bool drmcgrp_is_self_or_ancestor(struct drmcgrp *self,
> struct drmcgrp *relative)
> {
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
>
