linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Michal Hocko <mhocko@suse.com>
To: Mike Kravetz <mike.kravetz@oracle.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Shakeel Butt <shakeelb@google.com>,
	Oscar Salvador <osalvador@suse.de>,
	David Hildenbrand <david@redhat.com>,
	Muchun Song <songmuchun@bytedance.com>,
	David Rientjes <rientjes@google.com>,
	Miaohe Lin <linmiaohe@huawei.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Matthew Wilcox <willy@infradead.org>,
	HORIGUCHI NAOYA <naoya.horiguchi@nec.com>,
	"Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>,
	Waiman Long <longman@redhat.com>, Peter Xu <peterx@redhat.com>,
	Mina Almasry <almasrymina@google.com>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [RFC PATCH 5/8] hugetlb: change free_pool_huge_page to remove_pool_huge_page
Date: Tue, 23 Mar 2021 08:57:46 +0100	[thread overview]
Message-ID: <YFmfegQjCKuY05jy@dhcp22.suse.cz> (raw)
In-Reply-To: <fd723ea8-da7c-bd59-8d8a-e506be1b3af5@oracle.com>

On Mon 22-03-21 16:28:07, Mike Kravetz wrote:
> On 3/22/21 7:31 AM, Michal Hocko wrote:
> > On Fri 19-03-21 15:42:06, Mike Kravetz wrote:
> > [...]
> >> @@ -2090,9 +2084,15 @@ static void return_unused_surplus_pages(struct hstate *h,
> >>  	while (nr_pages--) {
> >>  		h->resv_huge_pages--;
> >>  		unused_resv_pages--;
> >> -		if (!free_pool_huge_page(h, &node_states[N_MEMORY], 1))
> >> +		page = remove_pool_huge_page(h, &node_states[N_MEMORY], 1);
> >> +		if (!page)
> >>  			goto out;
> >> -		cond_resched_lock(&hugetlb_lock);
> >> +
> >> +		/* Drop lock and free page to buddy as it could sleep */
> >> +		spin_unlock(&hugetlb_lock);
> >> +		update_and_free_page(h, page);
> >> +		cond_resched();
> >> +		spin_lock(&hugetlb_lock);
> >>  	}
> >>  
> >>  out:
> > 
> > This is likely a matter of taste but the repeated pattern of unlock,
> > update_and_free_page, cond_resched and lock seems rather clumsy.
> > Would it be slightly better/nicer to remove_pool_huge_page into a
> > list_head under a single lock invocation and then free up the whole lot
> > after the lock is dropped?
> 
> Yes, we can certainly do that.
> One downside I see is that the list can contain a bunch of pages not
> accounted for in hugetlb and not free in buddy (or cma).  Ideally, we
> would want to keep those in sync if possible.  Also, the commit that
> added the cond_resched talked about freeing up 12 TB worth of huge pages
> and it holding the lock for 150 seconds.  The new code is not holding
> the lock while calling free to buddy, but I wonder how long it would
> take to remove 12 TB worth of huge pages and add them to a separate list?

Well, the remove_pool_huge_page is just a accounting part and that
should be pretty invisible even when the number of pages is large. The
lockless nature (from hugetlb POV) of the final page release is the
heavy weight operation and whether you do it in chunks or in a single go
(with cond_resched) should be visible either. We already do the same
thing when uncharging memcg pages (mem_cgroup_uncharge_list). 

So I would agree with you that this would be a much bigger problem if
both the hugetlb and freeing path were equally heavy weight and the
delay between first pages uncaccounted and freed would be noticeable.

But I do not want to push for this. I just hated the hugetlb_lock dances
as this is ugly and repetitive pattern.
-- 
Michal Hocko
SUSE Labs


  reply	other threads:[~2021-03-23  7:57 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-19 22:42 [RFC PATCH 0/8] make hugetlb put_page safe for all calling contexts Mike Kravetz
2021-03-19 22:42 ` [RFC PATCH 1/8] hugetlb: add per-hstate mutex to synchronize user adjustments Mike Kravetz
2021-03-22 13:59   ` Michal Hocko
2021-03-22 16:57     ` Mike Kravetz
2021-03-23  7:48       ` Michal Hocko
2021-03-19 22:42 ` [RFC PATCH 2/8] hugetlb: recompute min_count when dropping hugetlb_lock Mike Kravetz
2021-03-22 14:07   ` Michal Hocko
2021-03-22 23:07     ` Mike Kravetz
2021-03-23  7:50       ` Michal Hocko
2021-03-23  8:01         ` Peter Zijlstra
2021-03-23  8:14           ` Michal Hocko
2021-03-23 23:18             ` Mike Kravetz
2021-03-24  8:36               ` Michal Hocko
2021-03-24 16:43                 ` Mike Kravetz
2021-03-19 22:42 ` [RFC PATCH 3/8] hugetlb: create remove_hugetlb_page() to separate functionality Mike Kravetz
2021-03-22 14:15   ` Michal Hocko
2021-03-22 17:01     ` Mike Kravetz
2021-03-19 22:42 ` [RFC PATCH 4/8] hugetlb: call update_and_free_page without hugetlb_lock Mike Kravetz
2021-03-22 14:19   ` Michal Hocko
2021-03-19 22:42 ` [RFC PATCH 5/8] hugetlb: change free_pool_huge_page to remove_pool_huge_page Mike Kravetz
2021-03-22 14:31   ` Michal Hocko
2021-03-22 23:28     ` Mike Kravetz
2021-03-23  7:57       ` Michal Hocko [this message]
2021-03-24  1:03         ` Mike Kravetz
2021-03-24  8:40           ` Michal Hocko
2021-03-24 16:38             ` Mike Kravetz
2021-03-24 16:50               ` Michal Hocko
2021-03-19 22:42 ` [RFC PATCH 6/8] hugetlb: make free_huge_page irq safe Mike Kravetz
2021-03-21 19:55   ` Mike Kravetz
2021-03-22 13:36   ` [hugetlb] cd190f60f9: BUG:sleeping_function_called_from_invalid_context_at_mm/hugetlb.c kernel test robot
2021-03-22 14:35   ` [RFC PATCH 6/8] hugetlb: make free_huge_page irq safe Michal Hocko
2021-03-19 22:42 ` [RFC PATCH 7/8] hugetlb: add update_and_free_page_no_sleep for irq context Mike Kravetz
2021-03-20  1:18   ` Hillf Danton
2021-03-25  0:26     ` Mike Kravetz
2021-03-22  8:41   ` Peter Zijlstra
2021-03-22 17:42     ` Mike Kravetz
2021-03-22 18:10       ` Roman Gushchin
2021-03-23 18:51         ` Mike Kravetz
2021-03-23 19:07           ` Roman Gushchin
2021-03-24  8:43           ` Michal Hocko
2021-03-24 16:53             ` Mike Kravetz
2021-03-22 20:43       ` Peter Zijlstra
2021-03-22 14:42   ` Michal Hocko
2021-03-22 14:46     ` Michal Hocko
2021-03-19 22:42 ` [RFC PATCH 8/8] hugetlb: track hugetlb pages allocated via cma_alloc Mike Kravetz

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=YFmfegQjCKuY05jy@dhcp22.suse.cz \
    --to=mhocko@suse.com \
    --cc=akpm@linux-foundation.org \
    --cc=almasrymina@google.com \
    --cc=aneesh.kumar@linux.ibm.com \
    --cc=david@redhat.com \
    --cc=linmiaohe@huawei.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=longman@redhat.com \
    --cc=mike.kravetz@oracle.com \
    --cc=naoya.horiguchi@nec.com \
    --cc=osalvador@suse.de \
    --cc=peterx@redhat.com \
    --cc=peterz@infradead.org \
    --cc=rientjes@google.com \
    --cc=shakeelb@google.com \
    --cc=songmuchun@bytedance.com \
    --cc=willy@infradead.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).