Re: [PATCH V3] x86/mm/pat: Do a small optimization and fix in reserve_memtype
hi, Borislav
thanks for your reply. :)
On 2015年07月22日 18:46, Borislav Petkov wrote:
> On Wed, Jul 22, 2015 at 05:06:04PM +0800, Pan Xinhui wrote:
>> how about:
>> memtype_lock protects the rb-tree root and the rb-nodes which is a field of
>> memtype from delete/add/lookup in a race.
>
> Use this:
>
> "All pat_rbtree operations need to be performed while holding the
> memtype_lock."
>
thanks!
>> Actually I have same questions. I find these output logs are added in
>> commit: 6997ab4982a29925e79f72c3a59823cf944c3529(x86: add PAT related
>> debug prints) In the past, *new_type == actual_type == new->type on
>> success. codes are below. author use actual_type there.
>
> So this function is one bit PITA. So req_type is used to compute actual
> type a bit higher:
>
> actual_type = pat_x_mtrr_type(start, end, req_type);
>
> and from then on actual_type is being used.
>
> BUT!, in order to have *all* debugging information, the last dprintk()
agree, output all debugging information.
> call should dump actual_type and req_type because this way we show what
then why not append "act %s" to the dprintk format string?
> pat_x_mtrr_type() did too. And we don't need to dump new->type because
> this is the !err case and in that case we assigned new_type to it, which
> we dump already.
yes, new->type is same with *new_type, and dump same value twice.
>
> Ok?
>
Let me think for a while. I wonder why there is not any comment that could tell
developers what "track %s" mean.
In different places of this file, "track %s" can mean what type of memory it is
now, or it used to be.
So I think this output filed "track %s" is just whatever people want to need to
print out.
I prefer to dump all debugging information here, so I agree with your idea.
thanks
> Btw, you could also simplify this:
>
> if (is_range_ram == 1) {
>
> err = reserve_ram_pages_type(start, end, req_type, new_type);
>
> return err;
> }
>
> to:
>
> if (is_range_ram == 1)
> return reserve_ram_pages_type(start, end, req_type, new_type);
>
seems better now, thanks!
> while at it.
>
> Thanks.
>
thanks
xinhui
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3] x86/mm/pat: Do a small optimization and fix in reserve_memtype
On Wed, Jul 22, 2015 at 05:06:04PM +0800, Pan Xinhui wrote:
> how about:
> memtype_lock protects the rb-tree root and the rb-nodes which is a field of
> memtype from delete/add/lookup in a race.
Use this:
"All pat_rbtree operations need to be performed while holding the
memtype_lock."
> Actually I have same questions. I find these output logs are added in
> commit: 6997ab4982a29925e79f72c3a59823cf944c3529(x86: add PAT related
> debug prints) In the past, *new_type == actual_type == new->type on
> success. codes are below. author use actual_type there.
So this function is one bit PITA. So req_type is used to compute actual
type a bit higher:
actual_type = pat_x_mtrr_type(start, end, req_type);
and from then on actual_type is being used.
BUT!, in order to have *all* debugging information, the last dprintk()
call should dump actual_type and req_type because this way we show what
pat_x_mtrr_type() did too. And we don't need to dump new->type because
this is the !err case and in that case we assigned new_type to it, which
we dump already.
Ok?
Btw, you could also simplify this:
if (is_range_ram == 1) {
err = reserve_ram_pages_type(start, end, req_type, new_type);
return err;
}
to:
if (is_range_ram == 1)
return reserve_ram_pages_type(start, end, req_type, new_type);
while at it.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3] x86/mm/pat: Do a small optimization and fix in reserve_memtype
hi, Borislav
thanks for your kind reply. :)
On 2015年07月22日 15:46, Borislav Petkov wrote:
> On Wed, Jul 22, 2015 at 01:38:48PM +0800, Pan Xinhui wrote:
>> From: Pan Xinhui
>>
>> It's more reasonable to unlock memtype_lock right after
>> rbt_memtype_check_insert. memtype_lock protects all data stored in
>> rb-tree from multiple access. It's not cool to call kfree, pr_info, etc
>> with this lock held. So move spin_unlock a little ahead.
>>
>> If *new* succeed to be stored into the rb-tree, we might hit panic.
>> Because we access *new* in dprintk "cattr_name(new->type)". Data stored
>> in the rb-tree might be freed at any possbile time. It's abviously wrong
>> to access such data without lock held. As new->type might be changed in
>> rbt_memtype_check_insert, so save new->type to actual_type, then use
>> actual_type in dprintk.
>>
>> Signed-off-by: Pan Xinhui
>> ---
>> change from v2:
>> update comments.
>> change from V1:
>> fix an access of *new* without memtype_lock held.
>> ---
>> arch/x86/mm/pat.c | 15 +--
>> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> This patch still doesn't update the comments over memtype_lock.
>
sorry for that.
how about:
memtype_lock protects the rb-tree root and the rb-nodes which is a field of
memtype from delete/add/lookup in a race.
>>
>> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
>> index 188e3e0..894a096 100644
>> --- a/arch/x86/mm/pat.c
>> +++ b/arch/x86/mm/pat.c
>> @@ -538,22 +538,25 @@ int reserve_memtype(u64 start, u64 end, enum
>> page_cache_mode req_type,
>> new->type = actual_type;
>>
>> spin_lock(_lock);
>> -
>> err = rbt_memtype_check_insert(new, new_type);
>> +/*
>> + * new->type might be changed in rbt_memtype_check_insert.
>> + * So save new->type to actual_type as dprintk uses it.
>> + * We are not allowed to touch new after unlocking memtype_lock.
>> + */
>> +actual_type = new->type;
>
> We already assign actual_type to new->type above. I think the dprintk
> needs actual_type and not what new->type has been changed to as that is
> in new_type.
>
Actually I have same questions. I find these output logs are added in commit:
6997ab4982a29925e79f72c3a59823cf944c3529(x86: add PAT related debug prints)
In the past, *new_type == actual_type == new->type on success. codes are below.
author use actual_type there.
376 if (ret_type) {
377 printk(
378 "reserve_memtype added 0x%Lx-0x%Lx, track %s, req %s, ret %s\n",
379 start, end, cattr_name(actual_type),
380 cattr_name(req_type), cattr_name(*ret_type));
381 } else {
382 printk(
383 "reserve_memtype added 0x%Lx-0x%Lx, track %s, req %s\n",
384 start, end, cattr_name(actual_type),
385 cattr_name(req_type));
386 }
But after reserve_memtype reworked, only new->type == *new_type on success.
actual_type is not synced with the them. So someone use new->type instead of
actual_type in dprintk.
I am not very clear why author need these debug information. So to avoid any
misunderstanding, I keep the same behavior of this dprintk. Keep what the
dpinrk does in the past.
If someone really think this debug information need change, maybe it's better
to send a new patch to fix it.
because *new_type is equal to new->type or new->type just did not change when
new_type is NULL. perhaps we can assign actual_type in such way below.
+ actual_type = new_type ? *new_type : actual_type;
thanks
xinxhui
>> +spin_unlock(_lock);
>> +
>> if (err) {
>> pr_info("x86/PAT: reserve_memtype failed [mem %#010Lx-%#010Lx],
>> track %s, req %s\n",
>> start, end - 1,
>> cattr_name(new->type), cattr_name(req_type));
>> kfree(new);
>> -spin_unlock(_lock);
>> -
>> return err;
>> }
>>
>> -spin_unlock(_lock);
>> -
>> dprintk("reserve_memtype added [mem %#010Lx-%#010Lx], track %s, req %s,
>> ret %s\n",
>> -start, end - 1, cattr_name(new->type), cattr_name(req_type),
>> +start, end - 1, cattr_name(actual_type), cattr_name(req_type),
>> new_type ? cattr_name(*new_type) : "-");
>>
>> return err;
>> --
>> 1.9.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3] x86/mm/pat: Do a small optimization and fix in reserve_memtype
On Wed, Jul 22, 2015 at 01:38:48PM +0800, Pan Xinhui wrote:
> From: Pan Xinhui
>
> It's more reasonable to unlock memtype_lock right after
> rbt_memtype_check_insert. memtype_lock protects all data stored in
> rb-tree from multiple access. It's not cool to call kfree, pr_info, etc
> with this lock held. So move spin_unlock a little ahead.
>
> If *new* succeed to be stored into the rb-tree, we might hit panic.
> Because we access *new* in dprintk "cattr_name(new->type)". Data stored
> in the rb-tree might be freed at any possbile time. It's abviously wrong
> to access such data without lock held. As new->type might be changed in
> rbt_memtype_check_insert, so save new->type to actual_type, then use
> actual_type in dprintk.
>
> Signed-off-by: Pan Xinhui
> ---
> change from v2:
> update comments.
> change from V1:
> fix an access of *new* without memtype_lock held.
> ---
> arch/x86/mm/pat.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
This patch still doesn't update the comments over memtype_lock.
>
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index 188e3e0..894a096 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -538,22 +538,25 @@ int reserve_memtype(u64 start, u64 end, enum
> page_cache_mode req_type,
> new->type = actual_type;
>
> spin_lock(_lock);
> -
> err = rbt_memtype_check_insert(new, new_type);
> + /*
> + * new->type might be changed in rbt_memtype_check_insert.
> + * So save new->type to actual_type as dprintk uses it.
> + * We are not allowed to touch new after unlocking memtype_lock.
> + */
> + actual_type = new->type;
We already assign actual_type to new->type above. I think the dprintk
needs actual_type and not what new->type has been changed to as that is
in new_type.
> + spin_unlock(_lock);
> +
> if (err) {
> pr_info("x86/PAT: reserve_memtype failed [mem %#010Lx-%#010Lx],
> track %s, req %s\n",
> start, end - 1,
> cattr_name(new->type), cattr_name(req_type));
> kfree(new);
> - spin_unlock(_lock);
> -
> return err;
> }
>
> - spin_unlock(_lock);
> -
> dprintk("reserve_memtype added [mem %#010Lx-%#010Lx], track %s, req %s,
> ret %s\n",
> - start, end - 1, cattr_name(new->type), cattr_name(req_type),
> + start, end - 1, cattr_name(actual_type), cattr_name(req_type),
> new_type ? cattr_name(*new_type) : "-");
>
> return err;
> --
> 1.9.1
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3] x86/mm/pat: Do a small optimization and fix in reserve_memtype
hi, Borislav
thanks for your kind reply. :)
On 2015年07月22日 15:46, Borislav Petkov wrote:
On Wed, Jul 22, 2015 at 01:38:48PM +0800, Pan Xinhui wrote:
From: Pan Xinhui xinhuix@intel.com
It's more reasonable to unlock memtype_lock right after
rbt_memtype_check_insert. memtype_lock protects all data stored in
rb-tree from multiple access. It's not cool to call kfree, pr_info, etc
with this lock held. So move spin_unlock a little ahead.
If *new* succeed to be stored into the rb-tree, we might hit panic.
Because we access *new* in dprintk cattr_name(new-type). Data stored
in the rb-tree might be freed at any possbile time. It's abviously wrong
to access such data without lock held. As new-type might be changed in
rbt_memtype_check_insert, so save new-type to actual_type, then use
actual_type in dprintk.
Signed-off-by: Pan Xinhui xinhuix@intel.com
---
change from v2:
update comments.
change from V1:
fix an access of *new* without memtype_lock held.
---
arch/x86/mm/pat.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
This patch still doesn't update the comments over memtype_lock.
sorry for that.
how about:
memtype_lock protects the rb-tree root and the rb-nodes which is a field of
memtype from delete/add/lookup in a race.
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index 188e3e0..894a096 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -538,22 +538,25 @@ int reserve_memtype(u64 start, u64 end, enum
page_cache_mode req_type,
new-type = actual_type;
spin_lock(memtype_lock);
-
err = rbt_memtype_check_insert(new, new_type);
+/*
+ * new-type might be changed in rbt_memtype_check_insert.
+ * So save new-type to actual_type as dprintk uses it.
+ * We are not allowed to touch new after unlocking memtype_lock.
+ */
+actual_type = new-type;
We already assign actual_type to new-type above. I think the dprintk
needs actual_type and not what new-type has been changed to as that is
in new_type.
Actually I have same questions. I find these output logs are added in commit:
6997ab4982a29925e79f72c3a59823cf944c3529(x86: add PAT related debug prints)
In the past, *new_type == actual_type == new-type on success. codes are below.
author use actual_type there.
376 if (ret_type) {
377 printk(
378 reserve_memtype added 0x%Lx-0x%Lx, track %s, req %s, ret %s\n,
379 start, end, cattr_name(actual_type),
380 cattr_name(req_type), cattr_name(*ret_type));
381 } else {
382 printk(
383 reserve_memtype added 0x%Lx-0x%Lx, track %s, req %s\n,
384 start, end, cattr_name(actual_type),
385 cattr_name(req_type));
386 }
But after reserve_memtype reworked, only new-type == *new_type on success.
actual_type is not synced with the them. So someone use new-type instead of
actual_type in dprintk.
I am not very clear why author need these debug information. So to avoid any
misunderstanding, I keep the same behavior of this dprintk. Keep what the
dpinrk does in the past.
If someone really think this debug information need change, maybe it's better
to send a new patch to fix it.
because *new_type is equal to new-type or new-type just did not change when
new_type is NULL. perhaps we can assign actual_type in such way below.
+ actual_type = new_type ? *new_type : actual_type;
thanks
xinxhui
+spin_unlock(memtype_lock);
+
if (err) {
pr_info(x86/PAT: reserve_memtype failed [mem %#010Lx-%#010Lx],
track %s, req %s\n,
start, end - 1,
cattr_name(new-type), cattr_name(req_type));
kfree(new);
-spin_unlock(memtype_lock);
-
return err;
}
-spin_unlock(memtype_lock);
-
dprintk(reserve_memtype added [mem %#010Lx-%#010Lx], track %s, req %s,
ret %s\n,
-start, end - 1, cattr_name(new-type), cattr_name(req_type),
+start, end - 1, cattr_name(actual_type), cattr_name(req_type),
new_type ? cattr_name(*new_type) : -);
return err;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3] x86/mm/pat: Do a small optimization and fix in reserve_memtype
On Wed, Jul 22, 2015 at 01:38:48PM +0800, Pan Xinhui wrote:
From: Pan Xinhui xinhuix@intel.com
It's more reasonable to unlock memtype_lock right after
rbt_memtype_check_insert. memtype_lock protects all data stored in
rb-tree from multiple access. It's not cool to call kfree, pr_info, etc
with this lock held. So move spin_unlock a little ahead.
If *new* succeed to be stored into the rb-tree, we might hit panic.
Because we access *new* in dprintk cattr_name(new-type). Data stored
in the rb-tree might be freed at any possbile time. It's abviously wrong
to access such data without lock held. As new-type might be changed in
rbt_memtype_check_insert, so save new-type to actual_type, then use
actual_type in dprintk.
Signed-off-by: Pan Xinhui xinhuix@intel.com
---
change from v2:
update comments.
change from V1:
fix an access of *new* without memtype_lock held.
---
arch/x86/mm/pat.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
This patch still doesn't update the comments over memtype_lock.
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index 188e3e0..894a096 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -538,22 +538,25 @@ int reserve_memtype(u64 start, u64 end, enum
page_cache_mode req_type,
new-type = actual_type;
spin_lock(memtype_lock);
-
err = rbt_memtype_check_insert(new, new_type);
+ /*
+ * new-type might be changed in rbt_memtype_check_insert.
+ * So save new-type to actual_type as dprintk uses it.
+ * We are not allowed to touch new after unlocking memtype_lock.
+ */
+ actual_type = new-type;
We already assign actual_type to new-type above. I think the dprintk
needs actual_type and not what new-type has been changed to as that is
in new_type.
+ spin_unlock(memtype_lock);
+
if (err) {
pr_info(x86/PAT: reserve_memtype failed [mem %#010Lx-%#010Lx],
track %s, req %s\n,
start, end - 1,
cattr_name(new-type), cattr_name(req_type));
kfree(new);
- spin_unlock(memtype_lock);
-
return err;
}
- spin_unlock(memtype_lock);
-
dprintk(reserve_memtype added [mem %#010Lx-%#010Lx], track %s, req %s,
ret %s\n,
- start, end - 1, cattr_name(new-type), cattr_name(req_type),
+ start, end - 1, cattr_name(actual_type), cattr_name(req_type),
new_type ? cattr_name(*new_type) : -);
return err;
--
1.9.1
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3] x86/mm/pat: Do a small optimization and fix in reserve_memtype
On Wed, Jul 22, 2015 at 05:06:04PM +0800, Pan Xinhui wrote:
how about:
memtype_lock protects the rb-tree root and the rb-nodes which is a field of
memtype from delete/add/lookup in a race.
Use this:
All pat_rbtree operations need to be performed while holding the
memtype_lock.
Actually I have same questions. I find these output logs are added in
commit: 6997ab4982a29925e79f72c3a59823cf944c3529(x86: add PAT related
debug prints) In the past, *new_type == actual_type == new-type on
success. codes are below. author use actual_type there.
So this function is one bit PITA. So req_type is used to compute actual
type a bit higher:
actual_type = pat_x_mtrr_type(start, end, req_type);
and from then on actual_type is being used.
BUT!, in order to have *all* debugging information, the last dprintk()
call should dump actual_type and req_type because this way we show what
pat_x_mtrr_type() did too. And we don't need to dump new-type because
this is the !err case and in that case we assigned new_type to it, which
we dump already.
Ok?
Btw, you could also simplify this:
if (is_range_ram == 1) {
err = reserve_ram_pages_type(start, end, req_type, new_type);
return err;
}
to:
if (is_range_ram == 1)
return reserve_ram_pages_type(start, end, req_type, new_type);
while at it.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3] x86/mm/pat: Do a small optimization and fix in reserve_memtype
hi, Borislav
thanks for your reply. :)
On 2015年07月22日 18:46, Borislav Petkov wrote:
On Wed, Jul 22, 2015 at 05:06:04PM +0800, Pan Xinhui wrote:
how about:
memtype_lock protects the rb-tree root and the rb-nodes which is a field of
memtype from delete/add/lookup in a race.
Use this:
All pat_rbtree operations need to be performed while holding the
memtype_lock.
thanks!
Actually I have same questions. I find these output logs are added in
commit: 6997ab4982a29925e79f72c3a59823cf944c3529(x86: add PAT related
debug prints) In the past, *new_type == actual_type == new-type on
success. codes are below. author use actual_type there.
So this function is one bit PITA. So req_type is used to compute actual
type a bit higher:
actual_type = pat_x_mtrr_type(start, end, req_type);
and from then on actual_type is being used.
BUT!, in order to have *all* debugging information, the last dprintk()
agree, output all debugging information.
call should dump actual_type and req_type because this way we show what
then why not append act %s to the dprintk format string?
pat_x_mtrr_type() did too. And we don't need to dump new-type because
this is the !err case and in that case we assigned new_type to it, which
we dump already.
yes, new-type is same with *new_type, and dump same value twice.
Ok?
Let me think for a while. I wonder why there is not any comment that could tell
developers what track %s mean.
In different places of this file, track %s can mean what type of memory it is
now, or it used to be.
So I think this output filed track %s is just whatever people want to need to
print out.
I prefer to dump all debugging information here, so I agree with your idea.
thanks
Btw, you could also simplify this:
if (is_range_ram == 1) {
err = reserve_ram_pages_type(start, end, req_type, new_type);
return err;
}
to:
if (is_range_ram == 1)
return reserve_ram_pages_type(start, end, req_type, new_type);
seems better now, thanks!
while at it.
Thanks.
thanks
xinhui
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V3] x86/mm/pat: Do a small optimization and fix in reserve_memtype
From: Pan Xinhui
It's more reasonable to unlock memtype_lock right after
rbt_memtype_check_insert. memtype_lock protects all data stored in
rb-tree from multiple access. It's not cool to call kfree, pr_info, etc
with this lock held. So move spin_unlock a little ahead.
If *new* succeed to be stored into the rb-tree, we might hit panic.
Because we access *new* in dprintk "cattr_name(new->type)". Data stored
in the rb-tree might be freed at any possbile time. It's abviously wrong
to access such data without lock held. As new->type might be changed in
rbt_memtype_check_insert, so save new->type to actual_type, then use
actual_type in dprintk.
Signed-off-by: Pan Xinhui
---
change from v2:
update comments.
change from V1:
fix an access of *new* without memtype_lock held.
---
arch/x86/mm/pat.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index 188e3e0..894a096 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -538,22 +538,25 @@ int reserve_memtype(u64 start, u64 end, enum
page_cache_mode req_type,
new->type = actual_type;
spin_lock(_lock);
-
err = rbt_memtype_check_insert(new, new_type);
+ /*
+* new->type might be changed in rbt_memtype_check_insert.
+* So save new->type to actual_type as dprintk uses it.
+* We are not allowed to touch new after unlocking memtype_lock.
+*/
+ actual_type = new->type;
+ spin_unlock(_lock);
+
if (err) {
pr_info("x86/PAT: reserve_memtype failed [mem %#010Lx-%#010Lx],
track %s, req %s\n",
start, end - 1,
cattr_name(new->type), cattr_name(req_type));
kfree(new);
- spin_unlock(_lock);
-
return err;
}
- spin_unlock(_lock);
-
dprintk("reserve_memtype added [mem %#010Lx-%#010Lx], track %s, req %s,
ret %s\n",
- start, end - 1, cattr_name(new->type), cattr_name(req_type),
+ start, end - 1, cattr_name(actual_type), cattr_name(req_type),
new_type ? cattr_name(*new_type) : "-");
return err;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V3] x86/mm/pat: Do a small optimization and fix in reserve_memtype
From: Pan Xinhui xinhuix@intel.com
It's more reasonable to unlock memtype_lock right after
rbt_memtype_check_insert. memtype_lock protects all data stored in
rb-tree from multiple access. It's not cool to call kfree, pr_info, etc
with this lock held. So move spin_unlock a little ahead.
If *new* succeed to be stored into the rb-tree, we might hit panic.
Because we access *new* in dprintk cattr_name(new-type). Data stored
in the rb-tree might be freed at any possbile time. It's abviously wrong
to access such data without lock held. As new-type might be changed in
rbt_memtype_check_insert, so save new-type to actual_type, then use
actual_type in dprintk.
Signed-off-by: Pan Xinhui xinhuix@intel.com
---
change from v2:
update comments.
change from V1:
fix an access of *new* without memtype_lock held.
---
arch/x86/mm/pat.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index 188e3e0..894a096 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -538,22 +538,25 @@ int reserve_memtype(u64 start, u64 end, enum
page_cache_mode req_type,
new-type = actual_type;
spin_lock(memtype_lock);
-
err = rbt_memtype_check_insert(new, new_type);
+ /*
+* new-type might be changed in rbt_memtype_check_insert.
+* So save new-type to actual_type as dprintk uses it.
+* We are not allowed to touch new after unlocking memtype_lock.
+*/
+ actual_type = new-type;
+ spin_unlock(memtype_lock);
+
if (err) {
pr_info(x86/PAT: reserve_memtype failed [mem %#010Lx-%#010Lx],
track %s, req %s\n,
start, end - 1,
cattr_name(new-type), cattr_name(req_type));
kfree(new);
- spin_unlock(memtype_lock);
-
return err;
}
- spin_unlock(memtype_lock);
-
dprintk(reserve_memtype added [mem %#010Lx-%#010Lx], track %s, req %s,
ret %s\n,
- start, end - 1, cattr_name(new-type), cattr_name(req_type),
+ start, end - 1, cattr_name(actual_type), cattr_name(req_type),
new_type ? cattr_name(*new_type) : -);
return err;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
