From: Oscar Salvador <osalvador@techadventures.net>
To: Michal Hocko <mhocko@kernel.org>
Cc: akpm@linux-foundation.org, pasha.tatashin@oracle.com,
vbabka@suse.cz, aaron.lu@intel.com, iamjoonsoo.kim@lge.com,
linux-kernel@vger.kernel.org, linux-mm@kvack.org,
Oscar Salvador <osalvador@suse.de>
Subject: Re: [PATCH v2 3/5] mm/page_alloc: Optimize free_area_init_core
Date: Thu, 19 Jul 2018 22:52:35 +0200 [thread overview]
Message-ID: <20180719205235.GA14010@techadventures.net> (raw)
In-Reply-To: <20180719151555.GH7193@dhcp22.suse.cz>
On Thu, Jul 19, 2018 at 05:15:55PM +0200, Michal Hocko wrote:
> Your changelog doesn't really explain the motivation. Does the change
> help performance? Is this a pure cleanup?
Hi Michal,
Sorry to not have explained this better from the very beginning.
It should help a bit in performance terms as we would be skipping those
condition checks and assignations for zones that do not have any pages.
It is not a huge win, but I think that skipping code we do not really need to run
is worh to have.
> The function is certainly not an example of beauty. It is more an
> example of changes done on top of older ones without much thinking. But
> I do not see your change would make it so much better. I would consider
> it a much nicer cleanup if it was split into logical units each doing
> one specific thing.
About the cleanup, I thought that moving that block of code to a separate function
would make the code easier to follow.
If you think that this is still not enough, I can try to split it and see the outcome.
> Btw. are you sure this change is correct? E.g.
> /*
> * Set an approximate value for lowmem here, it will be adjusted
> * when the bootmem allocator frees pages into the buddy system.
> * And all highmem pages will be managed by the buddy system.
> */
> zone->managed_pages = is_highmem_idx(j) ? realsize : freesize;
>
> expects freesize to be calculated properly and just from quick reading
> the code I do not see why skipping other adjustments is ok for size > 0.
> Maybe this is OK, I dunno and my brain is already heading few days off
> but a real cleanup wouldn't even make me think what the heck is going on
> here.
This changed in commit e69438596bb3e97809e76be315e54a4a444f4797.
Current code does not have "realsize" anymore.
Thanks
--
Oscar Salvador
SUSE L3
next prev parent reply other threads:[~2018-07-19 20:52 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-19 13:27 [PATCH v2 0/5] Refactor free_area_init_node/free_area_init_core osalvador
2018-07-19 13:27 ` [PATCH v2 1/5] mm/page_alloc: Move ifdefery out of free_area_init_core osalvador
2018-07-19 13:27 ` [PATCH v2 2/5] mm: access zone->node via zone_to_nid() and zone_set_nid() osalvador
2018-07-19 13:40 ` Michal Hocko
2018-07-19 13:44 ` Pavel Tatashin
2018-07-19 13:47 ` Michal Hocko
2018-07-19 13:27 ` [PATCH v2 3/5] mm/page_alloc: Optimize free_area_init_core osalvador
2018-07-19 13:44 ` Michal Hocko
2018-07-19 14:03 ` Oscar Salvador
2018-07-19 15:15 ` Michal Hocko
2018-07-19 20:52 ` Oscar Salvador [this message]
2018-07-20 10:03 ` Oscar Salvador
2018-07-23 8:35 ` Michal Hocko
2018-07-23 11:42 ` Oscar Salvador
2018-07-23 8:30 ` Michal Hocko
2018-07-19 13:27 ` [PATCH v2 4/5] mm/page_alloc: Inline function to handle CONFIG_DEFERRED_STRUCT_PAGE_INIT osalvador
2018-07-19 13:45 ` Michal Hocko
2018-07-19 13:27 ` [PATCH v2 5/5] mm/page_alloc: Only call pgdat_set_deferred_range when the system boots osalvador
2018-07-19 13:46 ` Michal Hocko
2018-07-19 13:58 ` Oscar Salvador
2018-07-19 14:03 ` Michal Hocko
2018-07-19 14:27 ` Pavel Tatashin
2018-07-19 15:01 ` Oscar Salvador
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=20180719205235.GA14010@techadventures.net \
--to=osalvador@techadventures.net \
--cc=aaron.lu@intel.com \
--cc=akpm@linux-foundation.org \
--cc=iamjoonsoo.kim@lge.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@kernel.org \
--cc=osalvador@suse.de \
--cc=pasha.tatashin@oracle.com \
--cc=vbabka@suse.cz \
/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).