Re: [PATCH 3/5] clk: convert clock name allocations to kstrdup_const

Andrzej Hajda
2015-01-13T07:58:02Z

On 01/13/2015 12:11 AM, Mike Turquette wrote:
> Quoting Andrzej Hajda (2015-01-12 01:18:41)
>> Clock subsystem frequently performs duplication of strings located
>> in read-only memory section. Replacing kstrdup by kstrdup_const
>> allows to avoid such operations.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Looks OK to me. Is there an easy trick to measuring the number of string
> duplications saved short of instrumenting your code with a counter?

I have just added pr_err in kstrdup_const:

diff --git a/mm/util.c b/mm/util.c
index c96fc4b..32a97b2 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -56,8 +56,10 @@ EXPORT_SYMBOL(kstrdup);

const char *kstrdup_const(const char *s, gfp_t gfp)
{
- if (is_kernel_rodata((unsigned long)s))
+ if (is_kernel_rodata((unsigned long)s)) {
+ pr_err("%s: %pS:%s\n", __func__,
__builtin_return_address(0), s);
return s;
+ }

return kstrdup(s, gfp);
}

Probably printk buffer size should be increased:
CONFIG_LOG_BUF_SHIFT=17

Regards
Andrzej

>
> Regards,
> Mike
>
>> ---
>> drivers/clk/clk.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index f4963b7..27e644a 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2048,7 +2048,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>> goto fail_out;
>> }
>>
>> - clk->name = kstrdup(hw->init->name, GFP_KERNEL);
>> + clk->name = kstrdup_const(hw->init->name, GFP_KERNEL);
>> if (!clk->name) {
>> pr_err("%s: could not allocate clk->name\n", __func__);
>> ret = -ENOMEM;
>> @@ -2075,7 +2075,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>>
>> /* copy each string name in case parent_names is __initdata */
>> for (i = 0; i < clk->num_parents; i++) {
>> - clk->parent_names[i] = kstrdup(hw->init->parent_names[i],
>> + clk->parent_names[i] = kstrdup_const(hw->init->parent_names[i],
>> GFP_KERNEL);
>> if (!clk->parent_names[i]) {
>> pr_err("%s: could not copy parent_names\n", __func__);
>> @@ -2090,10 +2090,10 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>>
>> fail_parent_names_copy:
>> while (--i >= 0)
>> - kfree(clk->parent_names[i]);
>> + kfree_const(clk->parent_names[i]);
>> kfree(clk->parent_names);
>> fail_parent_names:
>> - kfree(clk->name);
>> + kfree_const(clk->name);
>> fail_name:
>> kfree(clk);
>> fail_out:
>> @@ -2112,10 +2112,10 @@ static void __clk_release(struct kref *ref)
>>
>> kfree(clk->parents);
>> while (--i >= 0)
>> - kfree(clk->parent_names[i]);
>> + kfree_const(clk->parent_names[i]);
>>
>> kfree(clk->parent_names);
>> - kfree(clk->name);
>> + kfree_const(clk->name);
>> kfree(clk);
>> }
>>
>> --
>> 1.9.1
>>