Re: [PATCH v2] mm/zsmalloc: add statistics support

[Ganesh Mahendran]

Hello Andrew,

Thanks for your review.

2014-12-20 6:32 GMT+08:00 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>:
> On Fri, 19 Dec 2014 20:55:19 +0800 Ganesh Mahendran <opensource.ganesh@xxxxxxxxx> wrote:
>
>> Keeping fragmentation of zsmalloc in a low level is our target. But now
>> we still need to add the debug code in zsmalloc to get the quantitative data.
>>
>> This patch adds a new configuration CONFIG_ZSMALLOC_STAT to enable the
>> statistics collection for developers. Currently only the objects statatitics
>> in each class are collected. User can get the information via debugfs.
>>      cat /sys/kernel/debug/zsmalloc/pool-1/...
>
> Is everyone OK with this now?
>
>> --- a/include/linux/zsmalloc.h
>> +++ b/include/linux/zsmalloc.h
>> @@ -48,4 +48,13 @@ void zs_unmap_object(struct zs_pool *pool, unsigned long handle);
>>
>>  unsigned long zs_get_total_pages(struct zs_pool *pool);
>>
>> +#ifdef CONFIG_ZSMALLOC_STAT
>> +int get_zs_pool_index(struct zs_pool *pool);
>
> The name is inconsistent with the rest of zsmalloc and with preferred
> kernel naming conventions.  Should be "zs_get_pool_index".

Okay, I will modify it.

>
>> +#else
>> +static inline int get_zs_pool_index(struct zs_pool *pool)
>> +{
>> +     return -1;
>> +}
>> +#endif
>> +
>>  #endif
>> diff --git a/mm/Kconfig b/mm/Kconfig
>> index 1d1ae6b..95c5728 100644
>> --- a/mm/Kconfig
>> +++ b/mm/Kconfig
>>
>> ...
>>
>> +static int zs_stats_size_show(struct seq_file *s, void *v)
>> +{
>> +     int i;
>> +     struct zs_pool *pool = (struct zs_pool *)s->private;
>
> The typecast is unneeded and undesirable (it defeats typechecking).
>
>> +     struct size_class *class;
>> +     int objs_per_zspage;
>> +     unsigned long obj_allocated, obj_used, pages_used;
>> +     unsigned long total_objs = 0, total_used_objs = 0, total_pages = 0;
>> +
>> +     seq_printf(s, " %5s %5s %13s %10s %10s\n", "class", "size",
>> +                             "obj_allocated", "obj_used", "pages_used");
>> +
>> +     for (i = 0; i < zs_size_classes; i++) {
>> +             class = pool->size_class[i];
>> +
>> +             if (class->index != i)
>> +                     continue;
>> +
>> +             spin_lock(&class->lock);
>> +             obj_allocated = zs_stat_get(class, OBJ_ALLOCATED);
>> +             obj_used = zs_stat_get(class, OBJ_USED);
>> +             spin_unlock(&class->lock);
>> +
>> +             objs_per_zspage = get_maxobj_per_zspage(class->size,
>> +                             class->pages_per_zspage);
>> +             pages_used = obj_allocated / objs_per_zspage *
>> +                             class->pages_per_zspage;
>> +
>> +             seq_printf(s, " %5u %5u    %10lu %10lu %10lu\n", i,
>> +                     class->size, obj_allocated, obj_used, pages_used);
>> +
>> +             total_objs += obj_allocated;
>> +             total_used_objs += obj_used;
>> +             total_pages += pages_used;
>> +     }
>> +
>> +     seq_puts(s, "\n");
>> +     seq_printf(s, " %5s %5s    %10lu %10lu %10lu\n", "Total", "",
>> +                     total_objs, total_used_objs, total_pages);
>> +
>> +     return 0;
>> +}
>>
>> ...
>>
>> +static int zs_pool_stat_create(struct zs_pool *pool)
>> +{
>> +     char name[10];
>
> This is not good.  If the kernel creates and then destroys a pool 10000
> times, zs_pool_index==10000 and we overrun the buffer.  Could use
> kasprintf() in here to fix this.

Yes, kasprintf() is better. Although I used snprintf().
I will change it.

>
> zs_pool_index isn't a very good name - it doesn't index anything.
> zs_pool_id would be better.

Okay.

>
>> +     struct dentry *entry;
>> +
>> +     if (!zs_stat_root)
>> +             return -ENODEV;
>> +
>> +     pool->index = atomic_inc_return(&zs_pool_index);
>> +     snprintf(name, sizeof(name), "pool-%d", pool->index);
>> +     entry = debugfs_create_dir(name, zs_stat_root);
>> +     if (!entry) {
>> +             pr_warn("pool %d, debugfs dir <%s> creation failed\n",
>> +                             pool->index, name);
>> +             return -ENOMEM;
>
> Sigh.  The debugfs interface does suck.  Doesn't matter much.
>
>> +     }
>> +     pool->stat_dentry = entry;
>> +
>> +     entry = debugfs_create_file("obj_in_classes", S_IFREG | S_IRUGO,
>> +                     pool->stat_dentry, pool, &zs_stat_size_ops);
>> +     if (!entry) {
>> +             pr_warn("pool %d, debugfs file entry <%s> creation failed\n",
>> +                             pool->index, "obj_in_classes");
>> +             return -ENOMEM;
>> +     }
>> +
>> +     return 0;
>> +}
>>
>> ...
>>
>
--
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/