linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Michal Hocko <mhocko@kernel.org>
To: Oscar Salvador <osalvador@techadventures.net>
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: Mon, 23 Jul 2018 10:30:43 +0200	[thread overview]
Message-ID: <20180723083043.GF17905@dhcp22.suse.cz> (raw)
In-Reply-To: <20180719205235.GA14010@techadventures.net>

On Thu 19-07-18 22:52:35, Oscar Salvador wrote:
> 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.

It is always preferable to have numbers when you are claiming
performance benefits.

> 
> > 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.

Well, on the other hand, is the change really worth it? Moving code
around is not free either. Any git blame tracking to understand why the
code is done this way will be harder.

But just to make it clear, I do not want to discourage you from touching
this area. I just believe that if you really want to do that then the
result shouldn't just tweak one specific case and tweak it. It would be
much more appreciated if the new code had much better structure and less
hacks.
-- 
Michal Hocko
SUSE Labs

  parent reply	other threads:[~2018-07-23  8:30 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
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 [this message]
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=20180723083043.GF17905@dhcp22.suse.cz \
    --to=mhocko@kernel.org \
    --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=osalvador@suse.de \
    --cc=osalvador@techadventures.net \
    --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).