| Message ID | 20210721152358.2893314-7-jason@jlekstrand.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix the debugfs splat from mock selftests (v3) | expand |
On 21/07/2021 16:23, Jason Ekstrand wrote: > There's no reason that I can tell why this should be per-i915_buddy_mm > and doing so causes KMEM_CACHE to throw dmesg warnings because it tries > to create a debugfs entry with the name i915_buddy_block multiple times. > We could handle this by carefully giving each slab its own name but that > brings its own pain because then we have to store that string somewhere > and manage the lifetimes of the different slabs. The most likely > outcome would be a global atomic which we increment to get a new name or > something like that. > > The much easier solution is to use the i915_globals system like we do > for every other slab in i915. This ensures that we have exactly one of > them for each i915 driver load and it gets neatly created on module load > and destroyed on module unload. Using the globals system also means > that its now tied into the shrink handler so we can properly respond to > low-memory situations. > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > Fixes: 88be9a0a06b7 ("drm/i915/ttm: add ttm_buddy_man") > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Christian König <christian.koenig@amd.com> It was intentionally ripped it out with the idea that we would be moving the buddy stuff into ttm, and so part of that was trying to get rid of the some of the i915 specifics, like this globals thing. Reviewed-by: Matthew Auld <matthew.auld@intel.com> > --- > drivers/gpu/drm/i915/i915_buddy.c | 44 ++++++++++++++++++++++------- > drivers/gpu/drm/i915/i915_buddy.h | 3 +- > drivers/gpu/drm/i915/i915_globals.c | 2 ++ > 3 files changed, 38 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_buddy.c b/drivers/gpu/drm/i915/i915_buddy.c > index 29dd7d0310c1f..911feedad4513 100644 > --- a/drivers/gpu/drm/i915/i915_buddy.c > +++ b/drivers/gpu/drm/i915/i915_buddy.c > @@ -8,8 +8,14 @@ > #include "i915_buddy.h" > > #include "i915_gem.h" > +#include "i915_globals.h" > #include "i915_utils.h" > > +static struct i915_global_buddy { > + struct i915_global base; > + struct kmem_cache *slab_blocks; > +} global; > + > static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, > struct i915_buddy_block *parent, > unsigned int order, > @@ -19,7 +25,7 @@ static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, > > GEM_BUG_ON(order > I915_BUDDY_MAX_ORDER); > > - block = kmem_cache_zalloc(mm->slab_blocks, GFP_KERNEL); > + block = kmem_cache_zalloc(global.slab_blocks, GFP_KERNEL); > if (!block) > return NULL; > > @@ -34,7 +40,7 @@ static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, > static void i915_block_free(struct i915_buddy_mm *mm, > struct i915_buddy_block *block) > { > - kmem_cache_free(mm->slab_blocks, block); > + kmem_cache_free(global.slab_blocks, block); > } > > static void mark_allocated(struct i915_buddy_block *block) > @@ -85,15 +91,11 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64 size, u64 chunk_size) > > GEM_BUG_ON(mm->max_order > I915_BUDDY_MAX_ORDER); > > - mm->slab_blocks = KMEM_CACHE(i915_buddy_block, SLAB_HWCACHE_ALIGN); > - if (!mm->slab_blocks) > - return -ENOMEM; > - > mm->free_list = kmalloc_array(mm->max_order + 1, > sizeof(struct list_head), > GFP_KERNEL); > if (!mm->free_list) > - goto out_destroy_slab; > + return -ENOMEM; > > for (i = 0; i <= mm->max_order; ++i) > INIT_LIST_HEAD(&mm->free_list[i]); > @@ -145,8 +147,6 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64 size, u64 chunk_size) > kfree(mm->roots); > out_free_list: > kfree(mm->free_list); > -out_destroy_slab: > - kmem_cache_destroy(mm->slab_blocks); > return -ENOMEM; > } > > @@ -161,7 +161,6 @@ void i915_buddy_fini(struct i915_buddy_mm *mm) > > kfree(mm->roots); > kfree(mm->free_list); > - kmem_cache_destroy(mm->slab_blocks); > } > > static int split_block(struct i915_buddy_mm *mm, > @@ -410,3 +409,28 @@ int i915_buddy_alloc_range(struct i915_buddy_mm *mm, > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > #include "selftests/i915_buddy.c" > #endif > + > +static void i915_global_buddy_shrink(void) > +{ > + kmem_cache_shrink(global.slab_blocks); > +} > + > +static void i915_global_buddy_exit(void) > +{ > + kmem_cache_destroy(global.slab_blocks); > +} > + > +static struct i915_global_buddy global = { { > + .shrink = i915_global_buddy_shrink, > + .exit = i915_global_buddy_exit, > +} }; > + > +int __init i915_global_buddy_init(void) > +{ > + global.slab_blocks = KMEM_CACHE(i915_buddy_block, 0); > + if (!global.slab_blocks) > + return -ENOMEM; > + > + i915_global_register(&global.base); > + return 0; > +} > diff --git a/drivers/gpu/drm/i915/i915_buddy.h b/drivers/gpu/drm/i915/i915_buddy.h > index 37f8c42071d12..d8f26706de52f 100644 > --- a/drivers/gpu/drm/i915/i915_buddy.h > +++ b/drivers/gpu/drm/i915/i915_buddy.h > @@ -47,7 +47,6 @@ struct i915_buddy_block { > * i915_buddy_alloc* and i915_buddy_free* should suffice. > */ > struct i915_buddy_mm { > - struct kmem_cache *slab_blocks; > /* Maintain a free list for each order. */ > struct list_head *free_list; > > @@ -130,4 +129,6 @@ void i915_buddy_free(struct i915_buddy_mm *mm, struct i915_buddy_block *block); > > void i915_buddy_free_list(struct i915_buddy_mm *mm, struct list_head *objects); > > +int i915_global_buddy_init(void); > + > #endif > diff --git a/drivers/gpu/drm/i915/i915_globals.c b/drivers/gpu/drm/i915/i915_globals.c > index 87267e1d2ad92..e57102a4c8d16 100644 > --- a/drivers/gpu/drm/i915/i915_globals.c > +++ b/drivers/gpu/drm/i915/i915_globals.c > @@ -8,6 +8,7 @@ > #include <linux/workqueue.h> > > #include "i915_active.h" > +#include "i915_buddy.h" > #include "gem/i915_gem_context.h" > #include "gem/i915_gem_object.h" > #include "i915_globals.h" > @@ -87,6 +88,7 @@ static void __i915_globals_cleanup(void) > > static __initconst int (* const initfn[])(void) = { > i915_global_active_init, > + i915_global_buddy_init, > i915_global_context_init, > i915_global_gem_context_init, > i915_global_objects_init, >
On Wed, Jul 21, 2021 at 05:25:41PM +0100, Matthew Auld wrote: > On 21/07/2021 16:23, Jason Ekstrand wrote: > > There's no reason that I can tell why this should be per-i915_buddy_mm > > and doing so causes KMEM_CACHE to throw dmesg warnings because it tries > > to create a debugfs entry with the name i915_buddy_block multiple times. > > We could handle this by carefully giving each slab its own name but that > > brings its own pain because then we have to store that string somewhere > > and manage the lifetimes of the different slabs. The most likely > > outcome would be a global atomic which we increment to get a new name or > > something like that. > > > > The much easier solution is to use the i915_globals system like we do > > for every other slab in i915. This ensures that we have exactly one of > > them for each i915 driver load and it gets neatly created on module load > > and destroyed on module unload. Using the globals system also means > > that its now tied into the shrink handler so we can properly respond to > > low-memory situations. > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > Fixes: 88be9a0a06b7 ("drm/i915/ttm: add ttm_buddy_man") > > Cc: Matthew Auld <matthew.auld@intel.com> > > Cc: Christian König <christian.koenig@amd.com> > > It was intentionally ripped it out with the idea that we would be moving the > buddy stuff into ttm, and so part of that was trying to get rid of the some > of the i915 specifics, like this globals thing. > > Reviewed-by: Matthew Auld <matthew.auld@intel.com> I just sent out a patch to put i915_globals on a diet, so maybe we can hold this patch here a bit when there's other reasons for why this is special? Or at least no make this use the i915_globals stuff and instead just link up the init/exit function calls directly into Jason's new table, so that we don't have a merge conflict here? -Daniel > > > --- > > drivers/gpu/drm/i915/i915_buddy.c | 44 ++++++++++++++++++++++------- > > drivers/gpu/drm/i915/i915_buddy.h | 3 +- > > drivers/gpu/drm/i915/i915_globals.c | 2 ++ > > 3 files changed, 38 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_buddy.c b/drivers/gpu/drm/i915/i915_buddy.c > > index 29dd7d0310c1f..911feedad4513 100644 > > --- a/drivers/gpu/drm/i915/i915_buddy.c > > +++ b/drivers/gpu/drm/i915/i915_buddy.c > > @@ -8,8 +8,14 @@ > > #include "i915_buddy.h" > > #include "i915_gem.h" > > +#include "i915_globals.h" > > #include "i915_utils.h" > > +static struct i915_global_buddy { > > + struct i915_global base; > > + struct kmem_cache *slab_blocks; > > +} global; > > + > > static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, > > struct i915_buddy_block *parent, > > unsigned int order, > > @@ -19,7 +25,7 @@ static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, > > GEM_BUG_ON(order > I915_BUDDY_MAX_ORDER); > > - block = kmem_cache_zalloc(mm->slab_blocks, GFP_KERNEL); > > + block = kmem_cache_zalloc(global.slab_blocks, GFP_KERNEL); > > if (!block) > > return NULL; > > @@ -34,7 +40,7 @@ static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, > > static void i915_block_free(struct i915_buddy_mm *mm, > > struct i915_buddy_block *block) > > { > > - kmem_cache_free(mm->slab_blocks, block); > > + kmem_cache_free(global.slab_blocks, block); > > } > > static void mark_allocated(struct i915_buddy_block *block) > > @@ -85,15 +91,11 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64 size, u64 chunk_size) > > GEM_BUG_ON(mm->max_order > I915_BUDDY_MAX_ORDER); > > - mm->slab_blocks = KMEM_CACHE(i915_buddy_block, SLAB_HWCACHE_ALIGN); > > - if (!mm->slab_blocks) > > - return -ENOMEM; > > - > > mm->free_list = kmalloc_array(mm->max_order + 1, > > sizeof(struct list_head), > > GFP_KERNEL); > > if (!mm->free_list) > > - goto out_destroy_slab; > > + return -ENOMEM; > > for (i = 0; i <= mm->max_order; ++i) > > INIT_LIST_HEAD(&mm->free_list[i]); > > @@ -145,8 +147,6 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64 size, u64 chunk_size) > > kfree(mm->roots); > > out_free_list: > > kfree(mm->free_list); > > -out_destroy_slab: > > - kmem_cache_destroy(mm->slab_blocks); > > return -ENOMEM; > > } > > @@ -161,7 +161,6 @@ void i915_buddy_fini(struct i915_buddy_mm *mm) > > kfree(mm->roots); > > kfree(mm->free_list); > > - kmem_cache_destroy(mm->slab_blocks); > > } > > static int split_block(struct i915_buddy_mm *mm, > > @@ -410,3 +409,28 @@ int i915_buddy_alloc_range(struct i915_buddy_mm *mm, > > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > > #include "selftests/i915_buddy.c" > > #endif > > + > > +static void i915_global_buddy_shrink(void) > > +{ > > + kmem_cache_shrink(global.slab_blocks); > > +} > > + > > +static void i915_global_buddy_exit(void) > > +{ > > + kmem_cache_destroy(global.slab_blocks); > > +} > > + > > +static struct i915_global_buddy global = { { > > + .shrink = i915_global_buddy_shrink, > > + .exit = i915_global_buddy_exit, > > +} }; > > + > > +int __init i915_global_buddy_init(void) > > +{ > > + global.slab_blocks = KMEM_CACHE(i915_buddy_block, 0); > > + if (!global.slab_blocks) > > + return -ENOMEM; > > + > > + i915_global_register(&global.base); > > + return 0; > > +} > > diff --git a/drivers/gpu/drm/i915/i915_buddy.h b/drivers/gpu/drm/i915/i915_buddy.h > > index 37f8c42071d12..d8f26706de52f 100644 > > --- a/drivers/gpu/drm/i915/i915_buddy.h > > +++ b/drivers/gpu/drm/i915/i915_buddy.h > > @@ -47,7 +47,6 @@ struct i915_buddy_block { > > * i915_buddy_alloc* and i915_buddy_free* should suffice. > > */ > > struct i915_buddy_mm { > > - struct kmem_cache *slab_blocks; > > /* Maintain a free list for each order. */ > > struct list_head *free_list; > > @@ -130,4 +129,6 @@ void i915_buddy_free(struct i915_buddy_mm *mm, struct i915_buddy_block *block); > > void i915_buddy_free_list(struct i915_buddy_mm *mm, struct list_head *objects); > > +int i915_global_buddy_init(void); > > + > > #endif > > diff --git a/drivers/gpu/drm/i915/i915_globals.c b/drivers/gpu/drm/i915/i915_globals.c > > index 87267e1d2ad92..e57102a4c8d16 100644 > > --- a/drivers/gpu/drm/i915/i915_globals.c > > +++ b/drivers/gpu/drm/i915/i915_globals.c > > @@ -8,6 +8,7 @@ > > #include <linux/workqueue.h> > > #include "i915_active.h" > > +#include "i915_buddy.h" > > #include "gem/i915_gem_context.h" > > #include "gem/i915_gem_object.h" > > #include "i915_globals.h" > > @@ -87,6 +88,7 @@ static void __i915_globals_cleanup(void) > > static __initconst int (* const initfn[])(void) = { > > i915_global_active_init, > > + i915_global_buddy_init, > > i915_global_context_init, > > i915_global_gem_context_init, > > i915_global_objects_init, > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jul 21, 2021 at 1:56 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Jul 21, 2021 at 05:25:41PM +0100, Matthew Auld wrote: > > On 21/07/2021 16:23, Jason Ekstrand wrote: > > > There's no reason that I can tell why this should be per-i915_buddy_mm > > > and doing so causes KMEM_CACHE to throw dmesg warnings because it tries > > > to create a debugfs entry with the name i915_buddy_block multiple times. > > > We could handle this by carefully giving each slab its own name but that > > > brings its own pain because then we have to store that string somewhere > > > and manage the lifetimes of the different slabs. The most likely > > > outcome would be a global atomic which we increment to get a new name or > > > something like that. > > > > > > The much easier solution is to use the i915_globals system like we do > > > for every other slab in i915. This ensures that we have exactly one of > > > them for each i915 driver load and it gets neatly created on module load > > > and destroyed on module unload. Using the globals system also means > > > that its now tied into the shrink handler so we can properly respond to > > > low-memory situations. > > > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > > Fixes: 88be9a0a06b7 ("drm/i915/ttm: add ttm_buddy_man") > > > Cc: Matthew Auld <matthew.auld@intel.com> > > > Cc: Christian König <christian.koenig@amd.com> > > > > It was intentionally ripped it out with the idea that we would be moving the > > buddy stuff into ttm, and so part of that was trying to get rid of the some > > of the i915 specifics, like this globals thing. > > > > Reviewed-by: Matthew Auld <matthew.auld@intel.com> > > I just sent out a patch to put i915_globals on a diet, so maybe we can > hold this patch here a bit when there's other reasons for why this is > special? This is required to get rid of the dmesg warnings. > Or at least no make this use the i915_globals stuff and instead just link > up the init/exit function calls directly into Jason's new table, so that > we don't have a merge conflict here? I'm happy to deal with merge conflicts however they land. --Jason > -Daniel > > > > > > --- > > > drivers/gpu/drm/i915/i915_buddy.c | 44 ++++++++++++++++++++++------- > > > drivers/gpu/drm/i915/i915_buddy.h | 3 +- > > > drivers/gpu/drm/i915/i915_globals.c | 2 ++ > > > 3 files changed, 38 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_buddy.c b/drivers/gpu/drm/i915/i915_buddy.c > > > index 29dd7d0310c1f..911feedad4513 100644 > > > --- a/drivers/gpu/drm/i915/i915_buddy.c > > > +++ b/drivers/gpu/drm/i915/i915_buddy.c > > > @@ -8,8 +8,14 @@ > > > #include "i915_buddy.h" > > > #include "i915_gem.h" > > > +#include "i915_globals.h" > > > #include "i915_utils.h" > > > +static struct i915_global_buddy { > > > + struct i915_global base; > > > + struct kmem_cache *slab_blocks; > > > +} global; > > > + > > > static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, > > > struct i915_buddy_block *parent, > > > unsigned int order, > > > @@ -19,7 +25,7 @@ static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, > > > GEM_BUG_ON(order > I915_BUDDY_MAX_ORDER); > > > - block = kmem_cache_zalloc(mm->slab_blocks, GFP_KERNEL); > > > + block = kmem_cache_zalloc(global.slab_blocks, GFP_KERNEL); > > > if (!block) > > > return NULL; > > > @@ -34,7 +40,7 @@ static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, > > > static void i915_block_free(struct i915_buddy_mm *mm, > > > struct i915_buddy_block *block) > > > { > > > - kmem_cache_free(mm->slab_blocks, block); > > > + kmem_cache_free(global.slab_blocks, block); > > > } > > > static void mark_allocated(struct i915_buddy_block *block) > > > @@ -85,15 +91,11 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64 size, u64 chunk_size) > > > GEM_BUG_ON(mm->max_order > I915_BUDDY_MAX_ORDER); > > > - mm->slab_blocks = KMEM_CACHE(i915_buddy_block, SLAB_HWCACHE_ALIGN); > > > - if (!mm->slab_blocks) > > > - return -ENOMEM; > > > - > > > mm->free_list = kmalloc_array(mm->max_order + 1, > > > sizeof(struct list_head), > > > GFP_KERNEL); > > > if (!mm->free_list) > > > - goto out_destroy_slab; > > > + return -ENOMEM; > > > for (i = 0; i <= mm->max_order; ++i) > > > INIT_LIST_HEAD(&mm->free_list[i]); > > > @@ -145,8 +147,6 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64 size, u64 chunk_size) > > > kfree(mm->roots); > > > out_free_list: > > > kfree(mm->free_list); > > > -out_destroy_slab: > > > - kmem_cache_destroy(mm->slab_blocks); > > > return -ENOMEM; > > > } > > > @@ -161,7 +161,6 @@ void i915_buddy_fini(struct i915_buddy_mm *mm) > > > kfree(mm->roots); > > > kfree(mm->free_list); > > > - kmem_cache_destroy(mm->slab_blocks); > > > } > > > static int split_block(struct i915_buddy_mm *mm, > > > @@ -410,3 +409,28 @@ int i915_buddy_alloc_range(struct i915_buddy_mm *mm, > > > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > > > #include "selftests/i915_buddy.c" > > > #endif > > > + > > > +static void i915_global_buddy_shrink(void) > > > +{ > > > + kmem_cache_shrink(global.slab_blocks); > > > +} > > > + > > > +static void i915_global_buddy_exit(void) > > > +{ > > > + kmem_cache_destroy(global.slab_blocks); > > > +} > > > + > > > +static struct i915_global_buddy global = { { > > > + .shrink = i915_global_buddy_shrink, > > > + .exit = i915_global_buddy_exit, > > > +} }; > > > + > > > +int __init i915_global_buddy_init(void) > > > +{ > > > + global.slab_blocks = KMEM_CACHE(i915_buddy_block, 0); > > > + if (!global.slab_blocks) > > > + return -ENOMEM; > > > + > > > + i915_global_register(&global.base); > > > + return 0; > > > +} > > > diff --git a/drivers/gpu/drm/i915/i915_buddy.h b/drivers/gpu/drm/i915/i915_buddy.h > > > index 37f8c42071d12..d8f26706de52f 100644 > > > --- a/drivers/gpu/drm/i915/i915_buddy.h > > > +++ b/drivers/gpu/drm/i915/i915_buddy.h > > > @@ -47,7 +47,6 @@ struct i915_buddy_block { > > > * i915_buddy_alloc* and i915_buddy_free* should suffice. > > > */ > > > struct i915_buddy_mm { > > > - struct kmem_cache *slab_blocks; > > > /* Maintain a free list for each order. */ > > > struct list_head *free_list; > > > @@ -130,4 +129,6 @@ void i915_buddy_free(struct i915_buddy_mm *mm, struct i915_buddy_block *block); > > > void i915_buddy_free_list(struct i915_buddy_mm *mm, struct list_head *objects); > > > +int i915_global_buddy_init(void); > > > + > > > #endif > > > diff --git a/drivers/gpu/drm/i915/i915_globals.c b/drivers/gpu/drm/i915/i915_globals.c > > > index 87267e1d2ad92..e57102a4c8d16 100644 > > > --- a/drivers/gpu/drm/i915/i915_globals.c > > > +++ b/drivers/gpu/drm/i915/i915_globals.c > > > @@ -8,6 +8,7 @@ > > > #include <linux/workqueue.h> > > > #include "i915_active.h" > > > +#include "i915_buddy.h" > > > #include "gem/i915_gem_context.h" > > > #include "gem/i915_gem_object.h" > > > #include "i915_globals.h" > > > @@ -87,6 +88,7 @@ static void __i915_globals_cleanup(void) > > > static __initconst int (* const initfn[])(void) = { > > > i915_global_active_init, > > > + i915_global_buddy_init, > > > i915_global_context_init, > > > i915_global_gem_context_init, > > > i915_global_objects_init, > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Wed, Jul 21, 2021 at 03:15:09PM -0500, Jason Ekstrand wrote: > On Wed, Jul 21, 2021 at 1:56 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Wed, Jul 21, 2021 at 05:25:41PM +0100, Matthew Auld wrote: > > > On 21/07/2021 16:23, Jason Ekstrand wrote: > > > > There's no reason that I can tell why this should be per-i915_buddy_mm > > > > and doing so causes KMEM_CACHE to throw dmesg warnings because it tries > > > > to create a debugfs entry with the name i915_buddy_block multiple times. > > > > We could handle this by carefully giving each slab its own name but that > > > > brings its own pain because then we have to store that string somewhere > > > > and manage the lifetimes of the different slabs. The most likely > > > > outcome would be a global atomic which we increment to get a new name or > > > > something like that. > > > > > > > > The much easier solution is to use the i915_globals system like we do > > > > for every other slab in i915. This ensures that we have exactly one of > > > > them for each i915 driver load and it gets neatly created on module load > > > > and destroyed on module unload. Using the globals system also means > > > > that its now tied into the shrink handler so we can properly respond to > > > > low-memory situations. > > > > > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > > > Fixes: 88be9a0a06b7 ("drm/i915/ttm: add ttm_buddy_man") > > > > Cc: Matthew Auld <matthew.auld@intel.com> > > > > Cc: Christian König <christian.koenig@amd.com> > > > > > > It was intentionally ripped it out with the idea that we would be moving the > > > buddy stuff into ttm, and so part of that was trying to get rid of the some > > > of the i915 specifics, like this globals thing. > > > > > > Reviewed-by: Matthew Auld <matthew.auld@intel.com> > > > > I just sent out a patch to put i915_globals on a diet, so maybe we can > > hold this patch here a bit when there's other reasons for why this is > > special? > > This is required to get rid of the dmesg warnings. > > > Or at least no make this use the i915_globals stuff and instead just link > > up the init/exit function calls directly into Jason's new table, so that > > we don't have a merge conflict here? > > I'm happy to deal with merge conflicts however they land. I went hitshooting and rebased while applying :-) -Daniel > > --Jason > > > -Daniel > > > > > > > > > --- > > > > drivers/gpu/drm/i915/i915_buddy.c | 44 ++++++++++++++++++++++------- > > > > drivers/gpu/drm/i915/i915_buddy.h | 3 +- > > > > drivers/gpu/drm/i915/i915_globals.c | 2 ++ > > > > 3 files changed, 38 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_buddy.c b/drivers/gpu/drm/i915/i915_buddy.c > > > > index 29dd7d0310c1f..911feedad4513 100644 > > > > --- a/drivers/gpu/drm/i915/i915_buddy.c > > > > +++ b/drivers/gpu/drm/i915/i915_buddy.c > > > > @@ -8,8 +8,14 @@ > > > > #include "i915_buddy.h" > > > > #include "i915_gem.h" > > > > +#include "i915_globals.h" > > > > #include "i915_utils.h" > > > > +static struct i915_global_buddy { > > > > + struct i915_global base; > > > > + struct kmem_cache *slab_blocks; > > > > +} global; > > > > + > > > > static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, > > > > struct i915_buddy_block *parent, > > > > unsigned int order, > > > > @@ -19,7 +25,7 @@ static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, > > > > GEM_BUG_ON(order > I915_BUDDY_MAX_ORDER); > > > > - block = kmem_cache_zalloc(mm->slab_blocks, GFP_KERNEL); > > > > + block = kmem_cache_zalloc(global.slab_blocks, GFP_KERNEL); > > > > if (!block) > > > > return NULL; > > > > @@ -34,7 +40,7 @@ static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, > > > > static void i915_block_free(struct i915_buddy_mm *mm, > > > > struct i915_buddy_block *block) > > > > { > > > > - kmem_cache_free(mm->slab_blocks, block); > > > > + kmem_cache_free(global.slab_blocks, block); > > > > } > > > > static void mark_allocated(struct i915_buddy_block *block) > > > > @@ -85,15 +91,11 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64 size, u64 chunk_size) > > > > GEM_BUG_ON(mm->max_order > I915_BUDDY_MAX_ORDER); > > > > - mm->slab_blocks = KMEM_CACHE(i915_buddy_block, SLAB_HWCACHE_ALIGN); > > > > - if (!mm->slab_blocks) > > > > - return -ENOMEM; > > > > - > > > > mm->free_list = kmalloc_array(mm->max_order + 1, > > > > sizeof(struct list_head), > > > > GFP_KERNEL); > > > > if (!mm->free_list) > > > > - goto out_destroy_slab; > > > > + return -ENOMEM; > > > > for (i = 0; i <= mm->max_order; ++i) > > > > INIT_LIST_HEAD(&mm->free_list[i]); > > > > @@ -145,8 +147,6 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64 size, u64 chunk_size) > > > > kfree(mm->roots); > > > > out_free_list: > > > > kfree(mm->free_list); > > > > -out_destroy_slab: > > > > - kmem_cache_destroy(mm->slab_blocks); > > > > return -ENOMEM; > > > > } > > > > @@ -161,7 +161,6 @@ void i915_buddy_fini(struct i915_buddy_mm *mm) > > > > kfree(mm->roots); > > > > kfree(mm->free_list); > > > > - kmem_cache_destroy(mm->slab_blocks); > > > > } > > > > static int split_block(struct i915_buddy_mm *mm, > > > > @@ -410,3 +409,28 @@ int i915_buddy_alloc_range(struct i915_buddy_mm *mm, > > > > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > > > > #include "selftests/i915_buddy.c" > > > > #endif > > > > + > > > > +static void i915_global_buddy_shrink(void) > > > > +{ > > > > + kmem_cache_shrink(global.slab_blocks); > > > > +} > > > > + > > > > +static void i915_global_buddy_exit(void) > > > > +{ > > > > + kmem_cache_destroy(global.slab_blocks); > > > > +} > > > > + > > > > +static struct i915_global_buddy global = { { > > > > + .shrink = i915_global_buddy_shrink, > > > > + .exit = i915_global_buddy_exit, > > > > +} }; > > > > + > > > > +int __init i915_global_buddy_init(void) > > > > +{ > > > > + global.slab_blocks = KMEM_CACHE(i915_buddy_block, 0); > > > > + if (!global.slab_blocks) > > > > + return -ENOMEM; > > > > + > > > > + i915_global_register(&global.base); > > > > + return 0; > > > > +} > > > > diff --git a/drivers/gpu/drm/i915/i915_buddy.h b/drivers/gpu/drm/i915/i915_buddy.h > > > > index 37f8c42071d12..d8f26706de52f 100644 > > > > --- a/drivers/gpu/drm/i915/i915_buddy.h > > > > +++ b/drivers/gpu/drm/i915/i915_buddy.h > > > > @@ -47,7 +47,6 @@ struct i915_buddy_block { > > > > * i915_buddy_alloc* and i915_buddy_free* should suffice. > > > > */ > > > > struct i915_buddy_mm { > > > > - struct kmem_cache *slab_blocks; > > > > /* Maintain a free list for each order. */ > > > > struct list_head *free_list; > > > > @@ -130,4 +129,6 @@ void i915_buddy_free(struct i915_buddy_mm *mm, struct i915_buddy_block *block); > > > > void i915_buddy_free_list(struct i915_buddy_mm *mm, struct list_head *objects); > > > > +int i915_global_buddy_init(void); > > > > + > > > > #endif > > > > diff --git a/drivers/gpu/drm/i915/i915_globals.c b/drivers/gpu/drm/i915/i915_globals.c > > > > index 87267e1d2ad92..e57102a4c8d16 100644 > > > > --- a/drivers/gpu/drm/i915/i915_globals.c > > > > +++ b/drivers/gpu/drm/i915/i915_globals.c > > > > @@ -8,6 +8,7 @@ > > > > #include <linux/workqueue.h> > > > > #include "i915_active.h" > > > > +#include "i915_buddy.h" > > > > #include "gem/i915_gem_context.h" > > > > #include "gem/i915_gem_object.h" > > > > #include "i915_globals.h" > > > > @@ -87,6 +88,7 @@ static void __i915_globals_cleanup(void) > > > > static __initconst int (* const initfn[])(void) = { > > > > i915_global_active_init, > > > > + i915_global_buddy_init, > > > > i915_global_context_init, > > > > i915_global_gem_context_init, > > > > i915_global_objects_init, > > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/i915_buddy.c b/drivers/gpu/drm/i915/i915_buddy.c index 29dd7d0310c1f..911feedad4513 100644 --- a/drivers/gpu/drm/i915/i915_buddy.c +++ b/drivers/gpu/drm/i915/i915_buddy.c @@ -8,8 +8,14 @@ #include "i915_buddy.h" #include "i915_gem.h" +#include "i915_globals.h" #include "i915_utils.h" +static struct i915_global_buddy { + struct i915_global base; + struct kmem_cache *slab_blocks; +} global; + static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, struct i915_buddy_block *parent, unsigned int order, @@ -19,7 +25,7 @@ static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, GEM_BUG_ON(order > I915_BUDDY_MAX_ORDER); - block = kmem_cache_zalloc(mm->slab_blocks, GFP_KERNEL); + block = kmem_cache_zalloc(global.slab_blocks, GFP_KERNEL); if (!block) return NULL; @@ -34,7 +40,7 @@ static struct i915_buddy_block *i915_block_alloc(struct i915_buddy_mm *mm, static void i915_block_free(struct i915_buddy_mm *mm, struct i915_buddy_block *block) { - kmem_cache_free(mm->slab_blocks, block); + kmem_cache_free(global.slab_blocks, block); } static void mark_allocated(struct i915_buddy_block *block) @@ -85,15 +91,11 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64 size, u64 chunk_size) GEM_BUG_ON(mm->max_order > I915_BUDDY_MAX_ORDER); - mm->slab_blocks = KMEM_CACHE(i915_buddy_block, SLAB_HWCACHE_ALIGN); - if (!mm->slab_blocks) - return -ENOMEM; - mm->free_list = kmalloc_array(mm->max_order + 1, sizeof(struct list_head), GFP_KERNEL); if (!mm->free_list) - goto out_destroy_slab; + return -ENOMEM; for (i = 0; i <= mm->max_order; ++i) INIT_LIST_HEAD(&mm->free_list[i]); @@ -145,8 +147,6 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64 size, u64 chunk_size) kfree(mm->roots); out_free_list: kfree(mm->free_list); -out_destroy_slab: - kmem_cache_destroy(mm->slab_blocks); return -ENOMEM; } @@ -161,7 +161,6 @@ void i915_buddy_fini(struct i915_buddy_mm *mm) kfree(mm->roots); kfree(mm->free_list); - kmem_cache_destroy(mm->slab_blocks); } static int split_block(struct i915_buddy_mm *mm, @@ -410,3 +409,28 @@ int i915_buddy_alloc_range(struct i915_buddy_mm *mm, #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) #include "selftests/i915_buddy.c" #endif + +static void i915_global_buddy_shrink(void) +{ + kmem_cache_shrink(global.slab_blocks); +} + +static void i915_global_buddy_exit(void) +{ + kmem_cache_destroy(global.slab_blocks); +} + +static struct i915_global_buddy global = { { + .shrink = i915_global_buddy_shrink, + .exit = i915_global_buddy_exit, +} }; + +int __init i915_global_buddy_init(void) +{ + global.slab_blocks = KMEM_CACHE(i915_buddy_block, 0); + if (!global.slab_blocks) + return -ENOMEM; + + i915_global_register(&global.base); + return 0; +} diff --git a/drivers/gpu/drm/i915/i915_buddy.h b/drivers/gpu/drm/i915/i915_buddy.h index 37f8c42071d12..d8f26706de52f 100644 --- a/drivers/gpu/drm/i915/i915_buddy.h +++ b/drivers/gpu/drm/i915/i915_buddy.h @@ -47,7 +47,6 @@ struct i915_buddy_block { * i915_buddy_alloc* and i915_buddy_free* should suffice. */ struct i915_buddy_mm { - struct kmem_cache *slab_blocks; /* Maintain a free list for each order. */ struct list_head *free_list; @@ -130,4 +129,6 @@ void i915_buddy_free(struct i915_buddy_mm *mm, struct i915_buddy_block *block); void i915_buddy_free_list(struct i915_buddy_mm *mm, struct list_head *objects); +int i915_global_buddy_init(void); + #endif diff --git a/drivers/gpu/drm/i915/i915_globals.c b/drivers/gpu/drm/i915/i915_globals.c index 87267e1d2ad92..e57102a4c8d16 100644 --- a/drivers/gpu/drm/i915/i915_globals.c +++ b/drivers/gpu/drm/i915/i915_globals.c @@ -8,6 +8,7 @@ #include <linux/workqueue.h> #include "i915_active.h" +#include "i915_buddy.h" #include "gem/i915_gem_context.h" #include "gem/i915_gem_object.h" #include "i915_globals.h" @@ -87,6 +88,7 @@ static void __i915_globals_cleanup(void) static __initconst int (* const initfn[])(void) = { i915_global_active_init, + i915_global_buddy_init, i915_global_context_init, i915_global_gem_context_init, i915_global_objects_init,
There's no reason that I can tell why this should be per-i915_buddy_mm and doing so causes KMEM_CACHE to throw dmesg warnings because it tries to create a debugfs entry with the name i915_buddy_block multiple times. We could handle this by carefully giving each slab its own name but that brings its own pain because then we have to store that string somewhere and manage the lifetimes of the different slabs. The most likely outcome would be a global atomic which we increment to get a new name or something like that. The much easier solution is to use the i915_globals system like we do for every other slab in i915. This ensures that we have exactly one of them for each i915 driver load and it gets neatly created on module load and destroyed on module unload. Using the globals system also means that its now tied into the shrink handler so we can properly respond to low-memory situations. Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> Fixes: 88be9a0a06b7 ("drm/i915/ttm: add ttm_buddy_man") Cc: Matthew Auld <matthew.auld@intel.com> Cc: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/i915_buddy.c | 44 ++++++++++++++++++++++------- drivers/gpu/drm/i915/i915_buddy.h | 3 +- drivers/gpu/drm/i915/i915_globals.c | 2 ++ 3 files changed, 38 insertions(+), 11 deletions(-)