| Message ID | 20190304092908.57382-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/selftests/mm: Switch to bitmap_zalloc() | expand |
Quoting Andy Shevchenko (2019-03-04 09:29:08) > Switch to bitmap_zalloc() to show clearly what we are allocating. > Besides that it returns pointer of bitmap type instead of opaque void *. Which is confusing; since we explicitly want unsigned longs, not some amorphous bitmap type. > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 2 +- > drivers/gpu/drm/i915/i915_gem_fence_reg.c | 3 +-- > drivers/gpu/drm/i915/i915_gem_tiling.c | 6 +++--- > drivers/gpu/drm/i915/selftests/intel_uncore.c | 5 ++--- > 4 files changed, 7 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 6728ea5c71d4..0d96520cfdb3 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4410,7 +4410,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915, > drm_gem_object_release(&obj->base); > i915_gem_info_remove_obj(i915, obj->base.size); > > - kfree(obj->bit_17); > + bitmap_free(obj->bit_17); > i915_gem_object_free(obj); > > GEM_BUG_ON(!atomic_read(&i915->mm.free_count)); > diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c > index e037e94792f3..1f880e5b79b0 100644 > --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c > +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c > @@ -790,8 +790,7 @@ i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj, > int i; > > if (obj->bit_17 == NULL) { > - obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count), > - sizeof(long), GFP_KERNEL); > + obj->bit_17 = bitmap_zalloc(page_count, GFP_KERNEL); That feels a bit more of an overreach, as we just use bitops and never actually use the bitmap iface. Simply because it kills BITS_TO_LONGS(), even though I do not see why the bitmap_[z]alloc and bitmap_free are not inlines... And for this is not the overflow protection of kcalloc silly? We start with a large value, factorise it, then check that the two factors do not overflow? If it were to overflow, it would overflow in the BITS_TO_LONGS() itself. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Mon, Mar 04, 2019 at 09:41:34AM +0000, Chris Wilson wrote: > Quoting Andy Shevchenko (2019-03-04 09:29:08) > > Switch to bitmap_zalloc() to show clearly what we are allocating. > > Besides that it returns pointer of bitmap type instead of opaque void *. > > Which is confusing; since we explicitly want unsigned longs, not some > amorphous bitmap type. Why? You use it as a bitmap anyway since you are telling below you are using bit ops like set/clear_bit. > > if (obj->bit_17 == NULL) { > > - obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count), > > - sizeof(long), GFP_KERNEL); > > + obj->bit_17 = bitmap_zalloc(page_count, GFP_KERNEL); > > That feels a bit more of an overreach, as we just use bitops and never > actually use the bitmap iface. bitops are _luckily_ part of bitmap iface. bitmap iface has been evolved specifically the way the existing ops will work on it w/o any change. > Simply because it kills BITS_TO_LONGS(), even though I do not see why > the bitmap_[z]alloc and bitmap_free are not inlines... Because of circular dependencies (hell) in the headers. > And for this is not the overflow protection of kcalloc silly? We start > with a large value, factorise it, then check that the two factors do not > overflow? If it were to overflow, it would overflow in the > BITS_TO_LONGS() itself. This just a simple API change w/o functional changes. > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Thank you.
Quoting Andy Shevchenko (2019-03-04 09:54:46) > On Mon, Mar 04, 2019 at 09:41:34AM +0000, Chris Wilson wrote: > > Quoting Andy Shevchenko (2019-03-04 09:29:08) > > > Switch to bitmap_zalloc() to show clearly what we are allocating. > > > Besides that it returns pointer of bitmap type instead of opaque void *. > > > > Which is confusing; since we explicitly want unsigned longs, not some > > amorphous bitmap type. > > Why? You use it as a bitmap anyway since you are telling below you are using > bit ops like set/clear_bit. I consider that bitmap sits on on top of the bitops iface and so we should be using the types as defined by bitops. The allusion of "return pointer of bitmap type" is that it may become an abstract type, ill suited for the actual use. Just concerns over inferred language. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 6728ea5c71d4..0d96520cfdb3 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4410,7 +4410,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915, drm_gem_object_release(&obj->base); i915_gem_info_remove_obj(i915, obj->base.size); - kfree(obj->bit_17); + bitmap_free(obj->bit_17); i915_gem_object_free(obj); GEM_BUG_ON(!atomic_read(&i915->mm.free_count)); diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c index e037e94792f3..1f880e5b79b0 100644 --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c @@ -790,8 +790,7 @@ i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj, int i; if (obj->bit_17 == NULL) { - obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count), - sizeof(long), GFP_KERNEL); + obj->bit_17 = bitmap_zalloc(page_count, GFP_KERNEL); if (obj->bit_17 == NULL) { DRM_ERROR("Failed to allocate memory for bit 17 " "record\n"); diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c index 16cc9ddbce34..a9b5329dae3b 100644 --- a/drivers/gpu/drm/i915/i915_gem_tiling.c +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c @@ -301,11 +301,11 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object *obj, /* Try to preallocate memory required to save swizzling on put-pages */ if (i915_gem_object_needs_bit17_swizzle(obj)) { if (!obj->bit_17) { - obj->bit_17 = kcalloc(BITS_TO_LONGS(obj->base.size >> PAGE_SHIFT), - sizeof(long), GFP_KERNEL); + obj->bit_17 = bitmap_zalloc(obj->base.size >> PAGE_SHIFT, + GFP_KERNEL); } } else { - kfree(obj->bit_17); + bitmap_free(obj->bit_17); obj->bit_17 = NULL; } diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c index 81d9d31042a9..600167ebb303 100644 --- a/drivers/gpu/drm/i915/selftests/intel_uncore.c +++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c @@ -137,8 +137,7 @@ static int intel_uncore_check_forcewake_domains(struct drm_i915_private *dev_pri if (!IS_ENABLED(CONFIG_DRM_I915_SELFTEST_BROKEN)) return 0; - valid = kcalloc(BITS_TO_LONGS(FW_RANGE), sizeof(*valid), - GFP_KERNEL); + valid = bitmap_zalloc(FW_RANGE, GFP_KERNEL); if (!valid) return -ENOMEM; @@ -173,7 +172,7 @@ static int intel_uncore_check_forcewake_domains(struct drm_i915_private *dev_pri } } - kfree(valid); + bitmap_free(valid); return err; }
Switch to bitmap_zalloc() to show clearly what we are allocating. Besides that it returns pointer of bitmap type instead of opaque void *. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 2 +- drivers/gpu/drm/i915/i915_gem_fence_reg.c | 3 +-- drivers/gpu/drm/i915/i915_gem_tiling.c | 6 +++--- drivers/gpu/drm/i915/selftests/intel_uncore.c | 5 ++--- 4 files changed, 7 insertions(+), 9 deletions(-)