Re: [RFCv2 2/3] staging: ion: debugfs to shrink pool

[Gioh Kim]

2014-10-25 ìì 5:38, Laura Abbott ì ê:
> Hi,
>
> On 10/23/2014 11:47 PM, Gioh Kim wrote:
> > This patch creates debugfs files, /sys/kernel/debug/ion/heaps/system_shrink,
> > to shrink pool or get pool size.
> > Reading the file returns pool size and writing occurs to shrink pool.
>
> Can you clarify here that you are updating the existing debugfs
> infrastructure? Since the shrinker debugfs was commented out before
> it missed the API update so it would be good to note that as well.

OK, I will at the v2 patch.

> > Signed-off-by: Gioh Kim <gioh.kim@xxxxxxx>
> > ---
> >   drivers/staging/android/ion/ion.c |   31 ++++++++-----------------------
> >   1 file changed, 8 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> > index 290d4d2..ecc1121 100644
> > --- a/drivers/staging/android/ion/ion.c
> > +++ b/drivers/staging/android/ion/ion.c
> > @@ -1463,43 +1463,29 @@ static const struct file_operations debug_heap_fops = {
> >       .release = single_release,
> >   };
> >
> > -#ifdef DEBUG_HEAP_SHRINKER
>
> We're now unconditionally adding the shrinker debugfs. I'm not sure if
> this is something we want in production code. Could you turn this
> into a proper Kconfig?

The interface is via debugfs.
I think we don't turn on the debugfs in production code.

> >   static int debug_shrink_set(void *data, u64 val)
> >   {
> >       struct ion_heap *heap = data;
> > -    struct shrink_control sc;
> >       int objs;
> >
> > -    sc.gfp_mask = -1;
> > -    sc.nr_to_scan = 0;
> > -
> > -    if (!val)
> > -        return 0;
> > -
> > -    objs = heap->shrinker.shrink(&heap->shrinker, &sc);
> > -    sc.nr_to_scan = objs;
> > +    if (val)
> > +        objs = val;
> > +    else
> > +        objs = heap->ops->shrink(heap, __GFP_HIGHMEM, 0);
> >
> > -    heap->shrinker.shrink(&heap->shrinker, &sc);
> > +    (void)heap->ops->shrink(heap, __GFP_HIGHMEM, objs);
>
> The shrinker API now has separate functions for counting and scanning
> and ion wraps the calls to those as well to account for the deferred
> freelist. I realize the existing behavior may have been broken but the
> debugfs seems incomplete if it's not taking that into account as well.

ion_heap_shrink_count() and ion_heap_shrink_scan() are called by shrinker.
I think debug_shrink_set()/get() are for debugfs, so they can work without shrinker.

You can count deferred freelist via other debugfs file, for instance /sys/kernel/debug/ion/heaps/system.
I think debug_shrink_set()/get() should work by the number of pages in the pool.


> >       return 0;
> >   }
> >
> >   static int debug_shrink_get(void *data, u64 *val)
> >   {
> >       struct ion_heap *heap = data;
> > -    struct shrink_control sc;
> > -    int objs;
> > -
> > -    sc.gfp_mask = -1;
> > -    sc.nr_to_scan = 0;
> > -
> > -    objs = heap->shrinker.shrink(&heap->shrinker, &sc);
> > -    *val = objs;
> > +    *val = heap->ops->shrink(heap, __GFP_HIGHMEM, 0);
> >       return 0;
> >   }
> >
> >   DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
> >               debug_shrink_set, "%llu\n");
> > -#endif
> >
> >   void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
> >   {
> > @@ -1534,8 +1520,7 @@ void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
> >               path, heap->name);
> >       }
> >
> > -#ifdef DEBUG_HEAP_SHRINKER
> > -    if (heap->shrinker.shrink) {
> > +    if (heap->ops->shrink) {
>
> Technically a heap doesn't need to set ops->shrink to have a shrinker
> set up (not that it really matters for the current setup). Checking
> for scan_objects would be better.
>
> >           char debug_name[64];
> >
> >           snprintf(debug_name, 64, "%s_shrink", heap->name);
> > @@ -1550,7 +1535,7 @@ void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
> >                   path, debug_name);
> >           }
> >       }
> > -#endif
> > +
> >       up_write(&dev->lock);
> >   }
>
> Thanks,
> Laura
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/