From: Pan Xinhui <[email protected]>
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 <[email protected]>
---
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
On Wed, Jul 22, 2015 at 01:38:48PM +0800, Pan Xinhui wrote:
> From: Pan Xinhui <[email protected]>
>
> 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 <[email protected]>
> ---
> 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.
--
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
>
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.
--
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