Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 17 Apr 2020 17:47:14 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc:     Dave Hansen <dave.hansen@linux.intel.com>,
        Andy Lutomirski <luto@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH, RFC] x86/mm/pat: Restore large pages after fragmentation
Message-ID: <20200417154714.GI20730@hirez.programming.kicks-ass.net>
References: <20200416213229.19174-1-kirill.shutemov@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200416213229.19174-1-kirill.shutemov@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Apr 17, 2020 at 12:32:29AM +0300, Kirill A. Shutemov wrote:
> +static void cpa_restore_large_pages(struct cpa_data *cpa,
> +		struct list_head *pgtables)
> +{
> +	unsigned long start, addr, end;
> +	int i;
> +

> +	start = __cpa_addr(cpa, 0);
> +	end = start + PAGE_SIZE * cpa->numpages;
> +
> +	for (addr = start; addr >= start && addr < end; addr += PUD_SIZE)
> +		restore_large_pages(addr, pgtables);

Isn't that loop slightly broken?

Consider:

	         s                     e
	|---------|---------|---------|---------|
                 a0        a1        a2        a3

Where s,e are @start,@end resp. and a# are the consecutive values of
@addr with PUD sized steps.

Then, since a3 is >= @end, we'll not take that iteration and we'll not
try and merge that last PUD, even though we possibly could. One fix is
to truncate @start (and with that @addr) to the beginning of the PUD.

Also, I'm afraid that with my proposal this loop needs to do PMD size
steps. In that regard your version does make some sense. But it is
indeed less efficient for small ranges.

One possible fix is to pass @start,@end into the
restore/reconstruct/collapse such that we can iterate the minimal set of
page-tables for each level.