Linux-rt-users Archive on lore.kernel.org
 help / color / Atom feed
From: Vlastimil Babka <vbabka@suse.cz>
To: Mel Gorman <mgorman@techsingularity.net>
Cc: Linux-MM <linux-mm@kvack.org>,
	Linux-RT-Users <linux-rt-users@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Chuck Lever <chuck.lever@oracle.com>,
	Jesper Dangaard Brouer <brouer@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@kernel.org>, Michal Hocko <mhocko@kernel.org>
Subject: Re: [PATCH 07/11] mm/page_alloc: Remove duplicate checks if migratetype should be isolated
Date: Thu, 15 Apr 2021 13:24:13 +0200
Message-ID: <a0146dfe-36de-387e-8ee0-48f282661234@suse.cz> (raw)
In-Reply-To: <20210415093337.GH3697@techsingularity.net>

On 4/15/21 11:33 AM, Mel Gorman wrote:
> On Wed, Apr 14, 2021 at 07:21:42PM +0200, Vlastimil Babka wrote:
>> On 4/14/21 3:39 PM, Mel Gorman wrote:
>> > Both free_pcppages_bulk() and free_one_page() have very similar
>> > checks about whether a page's migratetype has changed under the
>> > zone lock. Use a common helper.
>> > 
>> > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
>> 
>> Seems like for free_pcppages_bulk() this patch makes it check for each page on
>> the pcplist
>> - zone->nr_isolate_pageblock != 0 instead of local bool (the performance might
>> be the same I guess on modern cpu though)
>> - is_migrate_isolate(migratetype) for a migratetype obtained by
>> get_pcppage_migratetype() which cannot be migrate_isolate so the check is useless.
>> 
>> As such it doesn't seem a worthwhile cleanup to me considering all the other
>> microoptimisations?
>> 
> 
> The patch was a preparation patch for the rest of the series to avoid code
> duplication and to consolidate checks together in one place to determine
> if they are even correct.
> 
> Until zone_pcp_disable() came along, it was possible to have isolated PCP
> pages in the lists even though zone->nr_isolate_pageblock could be 0 during
> memory hot-remove so the split in free_pcppages_bulk was not necessarily
> correct at all times.
> 
> The remaining problem is alloc_contig_pages, it does not disable
> PCPs so both checks are necessary. If that also disabled PCPs
> then check_migratetype_isolated could be deleted but the cost to
> alloc_contig_pages might be too high.

I see. Well that's unfortunate if checking zone->nr_isolate_pageblock is not
sufficient, as it was supposed to be :(
But I don't think the check_migratetype_isolated() check was helping in such
scenario as it was, anyway.

It's testing this:

+       if (unlikely(has_isolate_pageblock(zone) ||
+               is_migrate_isolate(migratetype))) {

In the context of free_one_page(), the 'migratetype' variable holds a value
that's read from pageblock in one of the callers of free_one_page():

migratetype = get_pcppage_migratetype(page);

and because it's read outside of zone lock, it might be a MIGRATE_ISOLATE even
though after we obtain the zone lock, we might find out it's not anymore. This
is explained in commit ad53f92eb416 ("mm/page_alloc: fix incorrect isolation
behavior by rechecking migratetype") as scenario 1.

However, in the context of free_pcppages_bulk(), the migratetype we are checking
in check_migratetype_isolated() is this one:

int mt = get_pcppage_migratetype(page);

That was the one determined while adding the page to pcplist, and is stored in
the struct page and we know it's not MIGRATE_ISOLATE otherwise the page would
not go to pcplist. But by rechecking this stored value, we would not be finding
the case where the underlying pageblock's migratetype would change to
MIGRATE_ISOLATE, anyway...

> I'll delete this patch for now because it's relatively minor and there
> should be other ways of keeping the code duplication down.
> 


  reply index

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-14 13:39 [PATCH 0/11 v3] Use local_lock for pcp protection and reduce stat overhead Mel Gorman
2021-04-14 13:39 ` [PATCH 01/11] mm/page_alloc: Split per cpu page lists and zone stats Mel Gorman
2021-04-14 13:39 ` [PATCH 02/11] mm/page_alloc: Convert per-cpu list protection to local_lock Mel Gorman
2021-04-14 13:39 ` [PATCH 03/11] mm/vmstat: Convert NUMA statistics to basic NUMA counters Mel Gorman
2021-04-14 13:39 ` [PATCH 04/11] mm/vmstat: Inline NUMA event counter updates Mel Gorman
2021-04-14 16:20   ` Vlastimil Babka
2021-04-14 16:26     ` Vlastimil Babka
2021-04-15  9:34       ` Mel Gorman
2021-04-14 13:39 ` [PATCH 05/11] mm/page_alloc: Batch the accounting updates in the bulk allocator Mel Gorman
2021-04-14 16:31   ` Vlastimil Babka
2021-04-14 13:39 ` [PATCH 06/11] mm/page_alloc: Reduce duration that IRQs are disabled for VM counters Mel Gorman
2021-04-14 17:10   ` Vlastimil Babka
2021-04-14 13:39 ` [PATCH 07/11] mm/page_alloc: Remove duplicate checks if migratetype should be isolated Mel Gorman
2021-04-14 17:21   ` Vlastimil Babka
2021-04-15  9:33     ` Mel Gorman
2021-04-15 11:24       ` Vlastimil Babka [this message]
2021-04-14 13:39 ` [PATCH 08/11] mm/page_alloc: Explicitly acquire the zone lock in __free_pages_ok Mel Gorman
2021-04-15 10:24   ` Vlastimil Babka
2021-04-14 13:39 ` [PATCH 09/11] mm/page_alloc: Avoid conflating IRQs disabled with zone->lock Mel Gorman
2021-04-15 12:25   ` Vlastimil Babka
2021-04-15 14:11     ` Mel Gorman
2021-04-14 13:39 ` [PATCH 10/11] mm/page_alloc: Update PGFREE outside the zone lock in __free_pages_ok Mel Gorman
2021-04-15 13:04   ` Vlastimil Babka
2021-04-14 13:39 ` [PATCH 11/11] mm/page_alloc: Embed per_cpu_pages locking within the per-cpu structure Mel Gorman
2021-04-15 14:53   ` Vlastimil Babka
2021-04-15 15:29     ` Mel Gorman

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=a0146dfe-36de-387e-8ee0-48f282661234@suse.cz \
    --to=vbabka@suse.cz \
    --cc=brouer@redhat.com \
    --cc=chuck.lever@oracle.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-rt-users@vger.kernel.org \
    --cc=mgorman@techsingularity.net \
    --cc=mhocko@kernel.org \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    /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

Linux-rt-users Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rt-users/0 linux-rt-users/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rt-users linux-rt-users/ https://lore.kernel.org/linux-rt-users \
		linux-rt-users@vger.kernel.org
	public-inbox-index linux-rt-users

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rt-users


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git