From: "Kirill A. Shutemov" <kirill@shutemov.name>
To: Ingo Molnar <mingo@kernel.org>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
Ingo Molnar <mingo@redhat.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
x86@kernel.org, Thomas Gleixner <tglx@linutronix.de>,
"H. Peter Anvin" <hpa@zytor.com>,
Andrew Morton <akpm@linux-foundation.org>,
Andy Lutomirski <luto@amacapital.net>,
Cyrill Gorcunov <gorcunov@openvz.org>,
Borislav Petkov <bp@suse.de>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/6] Boot-time switching between 4- and 5-level paging for 4.15, Part 1
Date: Tue, 31 Oct 2017 15:04:29 +0300 [thread overview]
Message-ID: <20171031120429.ehaqy2iciewcij35@node.shutemov.name> (raw)
In-Reply-To: <20171031094727.cvipkxzo2zhrxst3@gmail.com>
On Tue, Oct 31, 2017 at 10:47:27AM +0100, Ingo Molnar wrote:
>
> * Kirill A. Shutemov <kirill@shutemov.name> wrote:
> > I don't think this design is reasonable.
> >
> > - It introduces memory references where we haven't had them before.
> >
> > At this point all variable would fit a cache line, which is not that
> > bad. But I don't see what would stop the list from growing in the
> > future.
>
> Is any of these actually in a hotpath?
Probably, no. Closest to hotpath I see so far is page_zone_id() in page
allocator.
> Also, note the context: your changes turn some of these into variables. Yes, I
> suggest structuring them all and turning them all into variables, exactly because
> the majority are now dynamic, yet their _naming_ suggests that they are constants.
Another way to put it would be that you suggest significant rework of kernel
machinery based on cosmetic nitpick. :)
> > - We loose ability to optimize out change with static branches
> > (cpu_feature_enabled() instead of pgtable_l5_enabled variable).
> >
> > It's probably, not that big of an issue here, but if we are going to
> > use the same approach for other dynamic macros in the patchset, it
> > might be.
>
> Here too I think the (vast) majority of the uses here are for bootup/setup/init
> purposes, where clarity and maintainability of code matters a lot.
I would argue that it makes maintainability worse.
It makes dependencies between values less obvious. For instance, checking
MAXMEM definition on x86-64 makes it obvious that it depends directly
on MAX_PHYSMEM_BITS.
If we would convert MAXMEM to variable, we would need to check where the
variable is initialized and make sure that nobody changes it afterwards.
Does it sound like a win for maintainability?
> > - AFAICS, it requires changes to all architectures to provide such
> > structures as we now partly in generic code.
> >
> > Or to introduce some kind of compatibility layer, but it would make
> > the kernel as a whole uglier than cleaner. Especially, given that
> > nobody beyond x86 need this.
>
> Yes, all the uses should be harmonized (no compatibility layer) - but as you can
> see it from the histogram I generated it's a few dozen uses, i.e. not too bad.
Without a compatibility layer, I would need to change every architecture.
It's few dozen patches easily. Not fun.
---------------------------------8<------------------------------------
Putting, my disagreement with the design aside, I try to prototype it.
And stumble an issue that I don't see how to solve.
If we are going to convert macros to variable whether they need to be
variable in the configuration we quickly put ourself into corner:
- SECTIONS_SHIFT is dependent on MAX_PHYSMEM_BITS.
- SECTIONS_SHIFT is used to define SECTIONS_WIDTH, but only if
CONFIG_SPARSEMEM_VMEMMAP is not enabled. SECTIONS_WIDTH is zero
otherwise.
At this point we can convert both SECTIONS_SHIFT and SECTIONS_WIDTH to
variables.
But SECTIONS_WIDTH used on preprocessor level to determinate NODES_WIDTH,
which used to determinate if we going to define NODE_NOT_IN_PAGE_FLAGS and
the value of LAST_CPUPID_WIDTH.
Making SECTIONS_WIDTH variable breaks the preprocessor logic. But problems
don't stop there:
- LAST_CPUPID_WIDTH determinate if LAST_CPUPID_NOT_IN_PAGE_FLAGS is defined.
- LAST_CPUPID_NOT_IN_PAGE_FLAGS is used define struct page and therefore
cannot be dynamic (read variable).
In my patchset I made X86_5LEVEL select SPARSEMEM_VMEMMAP. It breaks the
chain and SECTIONS_WIDTH is never dynamic.
But how get it work with the design?
I can only think of hack like making machine.physmem.sections.shift a
constant macro if we don't want it dynamic for the configuration and leave
SECTHION_WITH as a constant in generic code.
To me it's ugly as hell.
--
Kirill A. Shutemov
next prev parent reply other threads:[~2017-10-31 12:04 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-29 14:08 [PATCH 0/6] Boot-time switching between 4- and 5-level paging for 4.15, Part 1 Kirill A. Shutemov
2017-09-29 14:08 ` [PATCH 1/6] mm/sparsemem: Allocate mem_section at runtime for SPARSEMEM_EXTREME Kirill A. Shutemov
2017-10-20 12:27 ` [tip:x86/mm] mm/sparsemem: Allocate mem_section at runtime for CONFIG_SPARSEMEM_EXTREME=y tip-bot for Kirill A. Shutemov
2017-11-02 12:31 ` Sudeep Holla
2017-11-02 13:34 ` Kirill A. Shutemov
2017-11-02 13:42 ` Sudeep Holla
2017-11-02 14:12 ` Kirill A. Shutemov
2017-11-02 15:07 ` Sudeep Holla
2017-11-02 15:37 ` Thierry Reding
2017-11-06 19:00 ` Bjorn Andersson
2017-11-07 1:15 ` Will Deacon
2017-09-29 14:08 ` [PATCH 2/6] mm/zsmalloc: Prepare to variable MAX_PHYSMEM_BITS Kirill A. Shutemov
2017-10-14 0:00 ` Nitin Gupta
2017-10-16 14:44 ` Kirill A. Shutemov
2017-10-18 23:39 ` Nitin Gupta
2017-09-29 14:08 ` [PATCH 3/6] x86/kasan: Use the same shadow offset for 4- and 5-level paging Kirill A. Shutemov
2017-10-20 12:28 ` [tip:x86/mm] " tip-bot for Andrey Ryabinin
2017-09-29 14:08 ` [PATCH 4/6] x86/xen: Provide pre-built page tables only for XEN_PV and XEN_PVH Kirill A. Shutemov
2017-10-20 12:28 ` [tip:x86/mm] x86/xen: Provide pre-built page tables only for CONFIG_XEN_PV=y and CONFIG_XEN_PVH=y tip-bot for Kirill A. Shutemov
2017-09-29 14:08 ` [PATCH 5/6] x86/xen: Drop 5-level paging support code from XEN_PV code Kirill A. Shutemov
2017-10-20 12:29 ` [tip:x86/mm] x86/xen: Drop 5-level paging support code from the " tip-bot for Kirill A. Shutemov
2017-09-29 14:08 ` [PATCH 6/6] x86/boot/compressed/64: Detect and handle 5-level paging at boot-time Kirill A. Shutemov
2017-10-03 8:27 ` [PATCH 0/6] Boot-time switching between 4- and 5-level paging for 4.15, Part 1 Kirill A. Shutemov
2017-10-17 15:42 ` Kirill A. Shutemov
2017-10-20 8:18 ` Ingo Molnar
2017-10-20 9:41 ` Kirill A. Shutemov
2017-10-20 15:23 ` Ingo Molnar
2017-10-20 16:23 ` Kirill A. Shutemov
2017-10-23 11:56 ` Ingo Molnar
2017-10-23 12:21 ` Kirill A. Shutemov
2017-10-23 12:40 ` Ingo Molnar
2017-10-23 12:48 ` Kirill A. Shutemov
2017-10-24 9:40 ` Ingo Molnar
2017-10-24 11:38 ` Kirill A. Shutemov
2017-10-24 12:47 ` Ingo Molnar
2017-10-24 13:12 ` Kirill A. Shutemov
2017-10-26 7:37 ` Ingo Molnar
2017-10-26 14:40 ` Kirill A. Shutemov
2017-10-31 9:47 ` Ingo Molnar
2017-10-31 12:04 ` Kirill A. Shutemov [this message]
2017-10-20 9:49 ` Minchan Kim
2017-10-20 12:18 ` Kirill A. Shutemov
2017-10-24 11:32 ` hpa
2017-10-24 11:43 ` Kirill A. Shutemov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171031120429.ehaqy2iciewcij35@node.shutemov.name \
--to=kirill@shutemov.name \
--cc=akpm@linux-foundation.org \
--cc=bp@suse.de \
--cc=gorcunov@openvz.org \
--cc=hpa@zytor.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=luto@amacapital.net \
--cc=mingo@kernel.org \
--cc=mingo@redhat.com \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).