Message-ID: <alpine.LSU.2.11.2009181723090.12148@eggly.anvils>
Date:   Fri, 18 Sep 2020 18:00:48 -0700 (PDT)
From:   Hugh Dickins <hughd@...gle.com>
To:     Matthew Wilcox <willy@...radead.org>
cc:     Alex Shi <alex.shi@...ux.alibaba.com>, akpm@...ux-foundation.org,
        mgorman@...hsingularity.net, tj@...nel.org, hughd@...gle.com,
        khlebnikov@...dex-team.ru, daniel.m.jordan@...cle.com,
        hannes@...xchg.org, lkp@...el.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, cgroups@...r.kernel.org,
        shakeelb@...gle.com, iamjoonsoo.kim@....com,
        richard.weiyang@...il.com, kirill@...temov.name,
        alexander.duyck@...il.com, rong.a.chen@...el.com, mhocko@...e.com,
        vdavydov.dev@...il.com, shy828301@...il.com,
        Andrea Arcangeli <aarcange@...hat.com>
Subject: Re: [PATCH v18 06/32] mm/thp: narrow lru locking

On Sun, 13 Sep 2020, Matthew Wilcox wrote:
> On Fri, Sep 11, 2020 at 11:37:50AM +0800, Alex Shi wrote:
> > 在 2020/9/10 下午9:49, Matthew Wilcox 写道:
> > > On Mon, Aug 24, 2020 at 08:54:39PM +0800, Alex Shi wrote:
> > >> lru_lock and page cache xa_lock have no reason with current sequence,
> > >> put them together isn't necessary. let's narrow the lru locking, but
> > >> left the local_irq_disable to block interrupt re-entry and statistic update.
> > > 
> > > What stats are you talking about here?
> > 
> > Hi Matthew,
> > 
> > Thanks for comments!
> > 
> > like __dec_node_page_state(head, NR_SHMEM_THPS); will have preemptive warning...
> 
> OK, but those stats are guarded by 'if (mapping)', so this patch doesn't
> produce that warning because we'll have taken the xarray lock and disabled
> interrupts.
> 
> > > How about this patch instead?  It occurred to me we already have
> > > perfectly good infrastructure to track whether or not interrupts are
> > > already disabled, and so we should use that instead of ensuring that
> > > interrupts are disabled, or tracking that ourselves.
> > 
> > So your proposal looks like;
> > 1, xa_lock_irq(&mapping->i_pages); (optional)
> > 2, spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
> > 3, spin_lock_irqsave(&pgdat->lru_lock, flags);
> > 
> > Is there meaningful for the 2nd and 3rd flags?
> 
> Yes.  We want to avoid doing:
> 
> 	if (mapping)
> 		spin_lock(&ds_queue->split_queue_lock);
> 	else
> 		spin_lock_irq(&ds_queue->split_queue_lock);
> ...
> 	if (mapping)
> 		spin_unlock(&ds_queue->split_queue_lock);
> 	else
> 		spin_unlock_irq(&ds_queue->split_queue_lock);
> 
> Just using _irqsave has the same effect and is easier to reason about.
> 
> > IIRC, I had a similar proposal as your, the flags used in xa_lock_irqsave(),
> > but objected by Hugh.
> 
> I imagine Hugh's objection was that we know it's safe to disable/enable
> interrupts here because we're in a sleepable context.  But for the
> other two locks, we'd rather not track whether we've already disabled
> interrupts or not.
> 
> Maybe you could dig up the email from Hugh?  I can't find it.

I did not find exactly the objection Alex seems to be remembering, but
I have certainly expressed frustration with the lack of a reason for
the THP split lock reordering, and in private mail in June while I was
testing and sending back fixes: "I'd prefer that you never got into this:
it looks like an unrelated and debatable cleanup, and I can see more
such cleanup to make there, that we'd better not get into right now."

I've several times toyed with just leaving this patch out of the series:
but each time ended up, for better or worse, deciding we'd better keep
it in - partly because we've never tested without it, and it cannot be
dropped without making some other change (to stabilize the memcg in
the !list case) - easily doable, but already done by this patch.

Alex asked me to improve his commit message to satisfy my objections,
here's what I sent him last night:

===
lru_lock and page cache xa_lock have no obvious reason to be taken
one way round or the other: until now, lru_lock has been taken before
page cache xa_lock, when splitting a THP; but nothing else takes them
together.  Reverse that ordering: let's narrow the lru locking - but
leave local_irq_disable to block interrupts throughout, like before.

Hugh Dickins point: split_huge_page_to_list() was already silly, to be
using the _irqsave variant: it's just been taking sleeping locks, so
would already be broken if entered with interrupts enabled.  So we
can save passing flags argument down to __split_huge_page().

Why change the lock ordering here? That was hard to decide. One reason:
when this series reaches per-memcg lru locking, it relies on the THP's
memcg to be stable when taking the lru_lock: that is now done after the
THP's refcount has been frozen, which ensures page memcg cannot change.

Another reason: previously, lock_page_memcg()'s move_lock was presumed
to nest inside lru_lock; but now lru_lock must nest inside (page cache
lock inside) move_lock, so it becomes possible to use lock_page_memcg()
to stabilize page memcg before taking its lru_lock.  That is not the
mechanism used in this series, but it is an option we want to keep open.
===

It's still the case that I want to avoid further cleanups and
bikeshedding here for now.  I took an open-minded look at Alex's
patch versus Matthew's patch, and do prefer Alex's: largely because
it's simple and explicit about where the irq disabling and enabling
is done (exactly where it was done before), and doesn't need irqsave
clutter in between.  If this were to be the only local_irq_disable()
in mm I'd NAK it, but that's not so - and as I said before, I don't
take the RT THP case very seriously anyway.

One slight worry in Matthew's version:

	spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
	count = page_count(head);
	mapcount = total_mapcount(head);
	if (!mapcount && page_ref_freeze(head, 1 + extra_pins)) {
		if (!list_empty(page_deferred_list(head))) {
			ds_queue->split_queue_len--;
			list_del(page_deferred_list(head));
		}
		spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
		if (mapping) {
			if (PageSwapBacked(head))
				__dec_node_page_state(head, NR_SHMEM_THPS);
			else
				__dec_node_page_state(head, NR_FILE_THPS);
		}
		__split_huge_page(page, list, end);

In the Anon case, interrupts are enabled when calling __split_huge_page()
there, but head's refcount is frozen: I'm uneasy about preemption when a
refcount is frozen.  But I'd worry much more if it were the mapping case:
no, that has interrupts safely disabled at that point (as does Anon in
the current kernel, and with Alex's patch).

Hugh

Powered by blists - more mailing lists