Hello,
Current shrinking is not page unit, block unit.
But shrinker returns the pools size with page unit.
So it is confused.
And there is no ways to shrink at any moment and any amount.
I have 2 patches:
1. Patch 1/2: make pool be shrinked by page unit
This patch trys to shrink pool in page unit.
2. Patch 2/2: enable debugfs to shrink pool directly
This patch enable debugfs to specify shrink amount.
Thanks for any feedback.
This patchset is based on linux-next-20141023.
Gioh Kim (2):
staging: ion: shrink page-pool by page unit
staging: ion: debugfs to shrink pool
drivers/staging/android/ion/ion.c | 31 +++++++------------------
drivers/staging/android/ion/ion_page_pool.c | 5 ++--
drivers/staging/android/ion/ion_system_heap.c | 7 ++++--
3 files changed, 16 insertions(+), 27 deletions(-)
--
1.7.9.5
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.
Signed-off-by: Gioh Kim <[email protected]>
---
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
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);
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) {
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);
}
--
1.7.9.5
This patch shrink page-pool by page unit.
Signed-off-by: Gioh Kim <[email protected]>
---
drivers/staging/android/ion/ion_page_pool.c | 5 +++--
drivers/staging/android/ion/ion_system_heap.c | 7 +++++--
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c
index 5864f3d..165152f 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -116,7 +116,7 @@ static int ion_page_pool_total(struct ion_page_pool *pool, bool high)
int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask,
int nr_to_scan)
{
- int freed;
+ int freed = 0;
bool high;
if (current_is_kswapd())
@@ -127,7 +127,7 @@ int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask,
if (nr_to_scan == 0)
return ion_page_pool_total(pool, high);
- for (freed = 0; freed < nr_to_scan; freed++) {
+ while (freed <= nr_to_scan) {
struct page *page;
mutex_lock(&pool->mutex);
@@ -141,6 +141,7 @@ int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask,
}
mutex_unlock(&pool->mutex);
ion_page_pool_free_pages(pool, page);
+ freed += (1 << pool->order);
}
return freed;
diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
index da2a63c..36e73c3 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -211,7 +211,7 @@ static int ion_system_heap_shrink(struct ion_heap *heap, gfp_t gfp_mask,
int nr_to_scan)
{
struct ion_system_heap *sys_heap;
- int nr_total = 0;
+ int nr_total = 0, nr_freed;
int i;
sys_heap = container_of(heap, struct ion_system_heap, heap);
@@ -219,7 +219,10 @@ static int ion_system_heap_shrink(struct ion_heap *heap, gfp_t gfp_mask,
for (i = 0; i < num_orders; i++) {
struct ion_page_pool *pool = sys_heap->pools[i];
- nr_total += ion_page_pool_shrink(pool, gfp_mask, nr_to_scan);
+ nr_freed += ion_page_pool_shrink(pool, gfp_mask, nr_to_scan);
+ nr_total += nr_freed;
+ /* nr_to_scan can be negative */
+ nr_to_scan -= nr_freed;
}
return nr_total;
--
1.7.9.5
Hello,
On Thu, Oct 23, 2014 at 6:22 PM, Gioh Kim <[email protected]> wrote:
> This patch shrink page-pool by page unit.
>
> Signed-off-by: Gioh Kim <[email protected]>
> ---
> drivers/staging/android/ion/ion_page_pool.c | 5 +++--
> drivers/staging/android/ion/ion_system_heap.c | 7 +++++--
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
--- cut --
> diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
> index da2a63c..36e73c3 100644
> --- a/drivers/staging/android/ion/ion_system_heap.c
> +++ b/drivers/staging/android/ion/ion_system_heap.c
> @@ -211,7 +211,7 @@ static int ion_system_heap_shrink(struct ion_heap *heap, gfp_t gfp_mask,
> int nr_to_scan)
> {
> struct ion_system_heap *sys_heap;
> - int nr_total = 0;
> + int nr_total = 0, nr_freed;
> int i;
>
> sys_heap = container_of(heap, struct ion_system_heap, heap);
> @@ -219,7 +219,10 @@ static int ion_system_heap_shrink(struct ion_heap *heap, gfp_t gfp_mask,
> for (i = 0; i < num_orders; i++) {
> struct ion_page_pool *pool = sys_heap->pools[i];
>
> - nr_total += ion_page_pool_shrink(pool, gfp_mask, nr_to_scan);
> + nr_freed += ion_page_pool_shrink(pool, gfp_mask, nr_to_scan);
nr_freed is not initialised to zero. This might result in updating of
a garbage value stored initially after nr_freed is created.
> + nr_total += nr_freed;
> + /* nr_to_scan can be negative */
> + nr_to_scan -= nr_freed;
> }
>
> return nr_total;
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
2014-10-24 오전 5:36, Devendra Naga 쓴 글:
> Hello,
>
> On Thu, Oct 23, 2014 at 6:22 PM, Gioh Kim <[email protected]> wrote:
>> This patch shrink page-pool by page unit.
>>
>> Signed-off-by: Gioh Kim <[email protected]>
>> ---
>> drivers/staging/android/ion/ion_page_pool.c | 5 +++--
>> drivers/staging/android/ion/ion_system_heap.c | 7 +++++--
>> 2 files changed, 8 insertions(+), 4 deletions(-)
>
>>
>
> --- cut --
>
>> diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
>> index da2a63c..36e73c3 100644
>> --- a/drivers/staging/android/ion/ion_system_heap.c
>> +++ b/drivers/staging/android/ion/ion_system_heap.c
>> @@ -211,7 +211,7 @@ static int ion_system_heap_shrink(struct ion_heap *heap, gfp_t gfp_mask,
>> int nr_to_scan)
>> {
>> struct ion_system_heap *sys_heap;
>> - int nr_total = 0;
>> + int nr_total = 0, nr_freed;
>> int i;
>>
>> sys_heap = container_of(heap, struct ion_system_heap, heap);
>> @@ -219,7 +219,10 @@ static int ion_system_heap_shrink(struct ion_heap *heap, gfp_t gfp_mask,
>> for (i = 0; i < num_orders; i++) {
>> struct ion_page_pool *pool = sys_heap->pools[i];
>>
>> - nr_total += ion_page_pool_shrink(pool, gfp_mask, nr_to_scan);
>> + nr_freed += ion_page_pool_shrink(pool, gfp_mask, nr_to_scan);
>
> nr_freed is not initialised to zero. This might result in updating of
> a garbage value stored initially after nr_freed is created.
Yes, your're right. It's my mistake.
I testes this on my platform with custom kernel and copied codes.
I missed a line for it.
I'm sorry.
>
>> + nr_total += nr_freed;
>> + /* nr_to_scan can be negative */
>> + nr_to_scan -= nr_freed;
>> }
>>
>> return nr_total;
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>
2014-10-24 오전 8:16, Gioh Kim 쓴 글:
>
>
> 2014-10-24 오전 5:36, Devendra Naga 쓴 글:
>> Hello,
>>
>> On Thu, Oct 23, 2014 at 6:22 PM, Gioh Kim <[email protected]> wrote:
>>> This patch shrink page-pool by page unit.
>>>
>>> Signed-off-by: Gioh Kim <[email protected]>
>>> ---
>>> drivers/staging/android/ion/ion_page_pool.c | 5 +++--
>>> drivers/staging/android/ion/ion_system_heap.c | 7 +++++--
>>> 2 files changed, 8 insertions(+), 4 deletions(-)
>>
>>>
>>
>> --- cut --
>>
>>> diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
>>> index da2a63c..36e73c3 100644
>>> --- a/drivers/staging/android/ion/ion_system_heap.c
>>> +++ b/drivers/staging/android/ion/ion_system_heap.c
>>> @@ -211,7 +211,7 @@ static int ion_system_heap_shrink(struct ion_heap *heap, gfp_t gfp_mask,
>>> int nr_to_scan)
>>> {
>>> struct ion_system_heap *sys_heap;
>>> - int nr_total = 0;
>>> + int nr_total = 0, nr_freed;
>>> int i;
>>>
>>> sys_heap = container_of(heap, struct ion_system_heap, heap);
>>> @@ -219,7 +219,10 @@ static int ion_system_heap_shrink(struct ion_heap *heap, gfp_t gfp_mask,
>>> for (i = 0; i < num_orders; i++) {
>>> struct ion_page_pool *pool = sys_heap->pools[i];
>>>
>>> - nr_total += ion_page_pool_shrink(pool, gfp_mask, nr_to_scan);
>>> + nr_freed += ion_page_pool_shrink(pool, gfp_mask, nr_to_scan);
>>
>> nr_freed is not initialised to zero. This might result in updating of
>> a garbage value stored initially after nr_freed is created.
>
> Yes, your're right. It's my mistake.
> I testes this on my platform with custom kernel and copied codes.
> I missed a line for it.
> I'm sorry.
Following is the correct code:
nr_freed = ion_page_pool_shrink(pool, gfp_mask, nr_to_scan);
>
>>
>>> + nr_total += nr_freed;
>>> + /* nr_to_scan can be negative */
>>> + nr_to_scan -= nr_freed;
>>> }
>>>
>>> return nr_total;
>>> --
>>> 1.7.9.5
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
>>
On Fri, Oct 24, 2014 at 5:01 AM, Gioh Kim <[email protected]> wrote:
>
>
> 2014-10-24 오전 8:16, Gioh Kim 쓴 글:
>
>>
>>
>> 2014-10-24 오전 5:36, Devendra Naga 쓴 글:
>>>
>>> Hello,
>>>
>>> On Thu, Oct 23, 2014 at 6:22 PM, Gioh Kim <[email protected]> wrote:
>>>> sys_heap = container_of(heap, struct ion_system_heap, heap);
>>>> @@ -219,7 +219,10 @@ static int ion_system_heap_shrink(struct ion_heap
>>>> *heap, gfp_t gfp_mask,
>>>> for (i = 0; i < num_orders; i++) {
>>>> struct ion_page_pool *pool = sys_heap->pools[i];
>>>>
>>>> - nr_total += ion_page_pool_shrink(pool, gfp_mask,
>>>> nr_to_scan);
>>>> + nr_freed += ion_page_pool_shrink(pool, gfp_mask,
>>>> nr_to_scan);
>>>
>>>
>>> nr_freed is not initialised to zero. This might result in updating of
>>> a garbage value stored initially after nr_freed is created.
>>
>>
>> Yes, your're right. It's my mistake.
>> I testes this on my platform with custom kernel and copied codes.
>> I missed a line for it.
>> I'm sorry.
>
>
> Following is the correct code:
> nr_freed = ion_page_pool_shrink(pool, gfp_mask, nr_to_scan);
>
Thanks that is correct. You can send another patch with this fix (it
can be patchset v2 or rfc v2).
>
>>
>>>
>>>> + nr_total += nr_freed;
>>>> + /* nr_to_scan can be negative */
>>>> + nr_to_scan -= nr_freed;
>>>> }
>>>>
>>>> return nr_total;
>>>> --
>>>> 1.7.9.5
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>>>> in
>>>> the body of a message to [email protected]
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> Please read the FAQ at http://www.tux.org/lkml/
>>>
>>>
>
On Thu, Oct 23, 2014 at 09:52:20PM +0900, 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.
The patch looks fine. Of course, the original code was debugfs before
it bitrotted and stopped compiling, but is there there a reason why we
shouldn't move this to sysfs instead? Just curious.
regards,
dan carpenter
On Mon, Oct 27, 2014 at 02:42:40PM +0300, Dan Carpenter wrote:
> On Thu, Oct 23, 2014 at 09:52:20PM +0900, 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.
>
> The patch looks fine. Of course, the original code was debugfs before
> it bitrotted and stopped compiling, but is there there a reason why we
> shouldn't move this to sysfs instead? Just curious.
>
I read the Laura's review comments and it turns out we don't want this
enabled by default and debugfs is fine. Question answered.
regards,
dan carpenter