Date: Fri, 17 Jul 2015 16:50:52 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: Pan Xinhui <xinhuix.pan@intel.com>
cc: linux-kernel@vger.kernel.org, mingo@redhat.com, hpa@zytor.com,
        x86@kernel.org, bp@suse.de, jgross@suse.com, mcgrof@suse.com,
        decui@microsoft.com, ross.zwisler@linux.intel.com,
        sfr@canb.auug.org.au, toshi.kani@hp.com,
        "mnipxh@163.com" <mnipxh@163.com>
Subject: Re: [PATCH] x86/mm/pat: let level meaningful even NULL return in,
 lookup_address_in_pgd
In-Reply-To: <55A5048E.3090403@intel.com>
Message-ID: <alpine.DEB.2.11.1507171643200.18576@nanos>
References: <55A5048E.3090403@intel.com>
User-Agent: Alpine 2.11 (DEB 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1598
Lines: 46

On Tue, 14 Jul 2015, Pan Xinhui wrote:
> If pmd or pud is not set, we may set a wrong page mapping level.

No. The behaviour is simply undefined, if the return value of the
function is NULL.

So what you are trying to do is to make the level information accurate
even for the failure case.
 
> We know *address* belongs to *pud*, however for some reasons *pmd* is
> NULL. For example, this address has no physical pages mapped. What we
> could benefit from this patch are below:
> 1) We can walk memory range easier.
> If addressA passed to lookup_address(), and NULL returned. We can pass
> addressA + level_to_size(level) to lookup_address() in next loop.
> ...
> if (!pte) {
> 	/* level_to_size has not been implemented in upstream*/
> 	address += level_to_size(level);
> 	continue;
> }

This example is completely useless because we do not see how the loop
itself looks like and how that improves anything. The proper way to do
this is to post:

     - the patch which changes the function
     - another patch which makes use of the change 

But so far I cannot see any reason why we want to change it.

> ...
> 2) keep same behavior because level is set to PG_LEVEL_4K even when pte
> is NULL.

And what's the actual benefit of #2? Keeping the same behaviour is a
requirement if you don't want to break any users of that function.

Thanks,

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