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;
MIME-Version: 1.0
References: <20200603232311.GA205619@roeck-us.net> <20200604083512.GN6857@suse.de>
In-Reply-To: <20200604083512.GN6857@suse.de>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu, 4 Jun 2020 10:16:07 -0700
Message-ID: <CAHk-=wj2_YdxPaRFqBUUDZvtZKKG5To2KJhciJmDbchW2NFLnw@mail.gmail.com>
Subject: Re: [PATCH] mm/vmalloc: track which page-table levels were modified
To:     Joerg Roedel <jroedel@suse.de>
Cc:     Guenter Roeck <linux@roeck-us.net>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andy Lutomirski <luto@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jun 4, 2020 at 1:35 AM Joerg Roedel <jroedel@suse.de> wrote:
>
> I posted the fix for this already:
>
>         https://lore.kernel.org/lkml/20200604074446.23944-1-joro@8bytes.org/

Ugh.

I was going to apply this directly, but as I looked at the patch I
just found it fairly illegible.

Is there some reason why the 5level-fixup.h versions use that
very-hard-to-follow macro, rather than the inline functions that the
main mm.h file uses?

I'm _assuming_ it's because it gets included in some place where not
everything is defined yet, so making it a macro means that it works
(later on) when everything has come together..

But the solution to that would seem to make all the p.._alloc_track()
macros just be in a different header file, and make them be all
together. We already have that

   #if !__ARCH_HAS_5LEVEL_HACK

in linux/mm.h, so it's not like we really have isolated that issue
into just 5level-fixup.h anyway, and creating a new
<linux/pagetable-alloc.h> header that has all the variations in one
place, and that is only included by the two (!) users of these things
would seem to be a good idea regardless.

Because <linux/mm.h> is included by pretty much everything. Why do we
have those alloc_track functions defined in such a common header when
they are _so_ special?

Please? I'd obviously like this to be fixed on ppc asap, but I'd also
like the fix to improve on the current somewhat confusing situation..

For extra point, the p??_alloc_track() functions could even be
generated from a macro pattern, because the pattern is pretty much set
in stone.

I think the only thing that really differs is the types and the
PGTBL_xyz_MODIFIED mask, and which entry is tested for "none" (which
is also the only thing that makes the 5level fixup case different -
no?

                       Linus