Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756422AbbGVJIb (ORCPT ); Wed, 22 Jul 2015 05:08:31 -0400 Received: from mga11.intel.com ([192.55.52.93]:26180 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755276AbbGVJI3 (ORCPT ); Wed, 22 Jul 2015 05:08:29 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,522,1432623600"; d="scan'208";a="733291671" Message-ID: <55AF5CFC.1080303@intel.com> Date: Wed, 22 Jul 2015 17:06:04 +0800 From: Pan Xinhui User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Borislav Petkov CC: "linux-kernel@vger.kernel.org" , Thomas Gleixner , mingo@redhat.com, hpa@zytor.com, x86@kernel.org, bp@suse.de, toshi.kani@hp.com, jgross@suse.com, mcgrof@suse.com, "mnipxh@163.com" , "yanmin_zhang@linux.intel.com" Subject: Re: [PATCH V3] x86/mm/pat: Do a small optimization and fix in reserve_memtype References: <55AF2C68.5010304@intel.com> <20150722074649.GD7979@nazgul.tnic> In-Reply-To: <20150722074649.GD7979@nazgul.tnic> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4423 Lines: 110 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(&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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/