From: Pan Xinhui <[email protected]>
There are many nodes in the PAT memtype rb-tree. When we dump this tree
we call kzalloc every time to copy nodes. Actually these kzalloc are not
necessary. Lets do a optimization now.
Create an *entry* in memtype_seq_start(), and free it in
memtype_seq_stop(). These two callback functions are alwasys called in
pair. Also because memtype_seq_show() is usually used only for
outputing. Seems memtype_seq_show is not the best place to free the
*entry*
And reuse the *entry* in memtype_seq_next() to enhance the performance.
Signed-off-by: Pan Xinhui <[email protected]>
---
arch/x86/mm/pat.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index 268b2c8..6302119 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -1001,45 +1001,42 @@ EXPORT_SYMBOL_GPL(pgprot_writethrough);
#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_X86_PAT)
-static struct memtype *memtype_get_idx(loff_t pos)
+static struct memtype *memtype_get_idx(struct memtype *entry, loff_t pos)
{
- struct memtype *print_entry;
int ret;
- print_entry = kzalloc(sizeof(struct memtype), GFP_KERNEL);
- if (!print_entry)
- return NULL;
-
spin_lock(&memtype_lock);
- ret = rbt_memtype_copy_nth_element(print_entry, pos);
+ ret = rbt_memtype_copy_nth_element(entry, pos);
spin_unlock(&memtype_lock);
- if (!ret) {
- return print_entry;
- } else {
- kfree(print_entry);
- return NULL;
- }
+ return ret ? NULL : entry;
}
static void *memtype_seq_start(struct seq_file *seq, loff_t *pos)
{
+ struct memtype *entry;
+
if (*pos == 0) {
++*pos;
seq_puts(seq, "PAT memtype list:\n");
}
- return memtype_get_idx(*pos);
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ seq->private = entry;
+ if (!entry)
+ return NULL;
+ return memtype_get_idx(entry, *pos);
}
static void *memtype_seq_next(struct seq_file *seq, void *v, loff_t *pos)
{
++*pos;
- return memtype_get_idx(*pos);
+ return memtype_get_idx((struct memtype *)v, *pos);
}
static void memtype_seq_stop(struct seq_file *seq, void *v)
{
+ kfree(seq->private);
}
static int memtype_seq_show(struct seq_file *seq, void *v)
@@ -1048,7 +1045,6 @@ static int memtype_seq_show(struct seq_file *seq, void *v)
seq_printf(seq, "%s @ 0x%Lx-0x%Lx\n", cattr_name(print_entry->type),
print_entry->start, print_entry->end);
- kfree(print_entry);
return 0;
}
--
1.9.1
> -----Original Message-----
> From: [email protected] [mailto:linux-kernel-
> [email protected]] On Behalf Of Pan Xinhui
> Sent: Thursday, July 23, 2015 4:54 AM
> To: [email protected]
> Subject: [PATCH] x86/mm/pat: Do a small optimization when dump PAT memtype
> list
...
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index 268b2c8..6302119 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -1001,45 +1001,42 @@ EXPORT_SYMBOL_GPL(pgprot_writethrough);
>
> #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_X86_PAT)
>
> -static struct memtype *memtype_get_idx(loff_t pos)
> +static struct memtype *memtype_get_idx(struct memtype *entry, loff_t pos)
> {
> - struct memtype *print_entry;
> int ret;
>
> - print_entry = kzalloc(sizeof(struct memtype), GFP_KERNEL);
> - if (!print_entry)
> - return NULL;
> -
> spin_lock(&memtype_lock);
> - ret = rbt_memtype_copy_nth_element(print_entry, pos);
> + ret = rbt_memtype_copy_nth_element(entry, pos);
> spin_unlock(&memtype_lock);
>
> - if (!ret) {
> - return print_entry;
> - } else {
> - kfree(print_entry);
> - return NULL;
> - }
> + return ret ? NULL : entry;
> }
>
...
> static void memtype_seq_stop(struct seq_file *seq, void *v)
> {
> + kfree(seq->private);
> }
>
Consider adding
seq->private = NULL;
so the stale pointer isn't left around. There's probably not
much risk of accessing it, but NULL is safer in case it is.
---
Robert Elliott, HP Server Storage
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
hi, Elliott
thanks for your reply. :)
On 2015年07月23日 22:53, Elliott, Robert (Server Storage) wrote:
>> -----Original Message-----
>> From: [email protected] [mailto:linux-kernel-
>> [email protected]] On Behalf Of Pan Xinhui
>> Sent: Thursday, July 23, 2015 4:54 AM
>> To: [email protected]
>> Subject: [PATCH] x86/mm/pat: Do a small optimization when dump PAT memtype
>> list
> ...
>> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
>> index 268b2c8..6302119 100644
>> --- a/arch/x86/mm/pat.c
>> +++ b/arch/x86/mm/pat.c
>> @@ -1001,45 +1001,42 @@ EXPORT_SYMBOL_GPL(pgprot_writethrough);
>>
>> #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_X86_PAT)
>>
>> -static struct memtype *memtype_get_idx(loff_t pos)
>> +static struct memtype *memtype_get_idx(struct memtype *entry, loff_t pos)
>> {
>> - struct memtype *print_entry;
>> int ret;
>>
>> - print_entry = kzalloc(sizeof(struct memtype), GFP_KERNEL);
>> - if (!print_entry)
>> - return NULL;
>> -
>> spin_lock(&memtype_lock);
>> - ret = rbt_memtype_copy_nth_element(print_entry, pos);
>> + ret = rbt_memtype_copy_nth_element(entry, pos);
>> spin_unlock(&memtype_lock);
>>
>> - if (!ret) {
>> - return print_entry;
>> - } else {
>> - kfree(print_entry);
>> - return NULL;
>> - }
>> + return ret ? NULL : entry;
>> }
>>
> ...
>> static void memtype_seq_stop(struct seq_file *seq, void *v)
>> {
>> + kfree(seq->private);
>> }
>>
>
> Consider adding
> seq->private = NULL;
> so the stale pointer isn't left around. There's probably not
> much risk of accessing it, but NULL is safer in case it is.
>
I know exactly what are you worrying about.
start and stop callback is still not the best place to alloc/free the *entry*. you are worrying about that some codes might touch it.
seq_file.c has offered seq_open_private and seq_release_private. we can make use of them. :)
I will work out patch v2, this time I will Cc you. I am very happy if you could review it at any free time. :)
thanks
xinhui
> ---
> Robert Elliott, HP Server Storage
>