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

[Andrzej Hajda]

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@xxxxxxxxxxx>
> 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
>>

--
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/