From: "Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com>
To: Muchun Song <songmuchun@bytedance.com>, Michal Hocko <mhocko@suse.com>
Cc: Mike Kravetz <mike.kravetz@oracle.com>,
Linux Memory Management List <linux-mm@kvack.org>,
LKML <linux-kernel@vger.kernel.org>, Roman Gushchin <guro@fb.com>,
Shakeel Butt <shakeelb@google.com>,
Oscar Salvador <osalvador@suse.de>,
David Hildenbrand <david@redhat.com>,
David Rientjes <rientjes@google.com>,
linmiaohe <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>,
Hillf Danton <hdanton@sina.com>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>,
Will Deacon <will@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>
Subject: RE: [External] Re: [PATCH v2 1/8] mm/cma: change cma mutex to irq safe spinlock
Date: Tue, 30 Mar 2021 08:17:08 +0000 [thread overview]
Message-ID: <7087d077cb814e7abbc5a4a98ab06369@hisilicon.com> (raw)
In-Reply-To: <CAMZfGtUv4O_+W5rHt0P4Xbw=WXJ-ZwHYMrg=iJa2CEkfxb91gA@mail.gmail.com>
> -----Original Message-----
> From: Muchun Song [mailto:songmuchun@bytedance.com]
> Sent: Tuesday, March 30, 2021 9:09 PM
> To: Michal Hocko <mhocko@suse.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>; Linux Memory Management List
> <linux-mm@kvack.org>; LKML <linux-kernel@vger.kernel.org>; Roman Gushchin
> <guro@fb.com>; Shakeel Butt <shakeelb@google.com>; Oscar Salvador
> <osalvador@suse.de>; David Hildenbrand <david@redhat.com>; David Rientjes
> <rientjes@google.com>; linmiaohe <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>; Hillf Danton <hdanton@sina.com>; Joonsoo Kim
> <iamjoonsoo.kim@lge.com>; Song Bao Hua (Barry Song)
> <song.bao.hua@hisilicon.com>; Will Deacon <will@kernel.org>; Andrew Morton
> <akpm@linux-foundation.org>
> Subject: Re: [External] Re: [PATCH v2 1/8] mm/cma: change cma mutex to irq safe
> spinlock
>
> On Tue, Mar 30, 2021 at 4:01 PM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Mon 29-03-21 16:23:55, Mike Kravetz wrote:
> > > Ideally, cma_release could be called from any context. However,
> > > that is not possible because a mutex is used to protect the per-area bitmap.
> > > Change the bitmap to an irq safe spinlock.
> >
> > I would phrase the changelog slightly differerent "
> > cma_release is currently a sleepable operatation because the bitmap
> > manipulation is protected by cma->lock mutex. Hugetlb code which
> > relies on cma_release for CMA backed (giga) hugetlb pages, however,
> > needs to be irq safe.
> >
> > The lock doesn't protect any sleepable operation so it can be changed
> > to a (irq aware) spin lock. The bitmap processing should be quite fast
> > in typical case but if cma sizes grow to TB then we will likely need
> > to replace the lock by a more optimized bitmap implementation.
> > "
> >
> > it seems that you are overusing irqsave variants even from context
> > which are never called from the IRQ context so they do not need storing flags.
> >
> > [...]
> > > @@ -391,8 +391,9 @@ static void cma_debug_show_areas(struct cma *cma)
> > > unsigned long start = 0;
> > > unsigned long nr_part, nr_total = 0;
> > > unsigned long nbits = cma_bitmap_maxno(cma);
> > > + unsigned long flags;
> > >
> > > - mutex_lock(&cma->lock);
> > > + spin_lock_irqsave(&cma->lock, flags);
> >
> > spin_lock_irq should be sufficient. This is only called from the
> > allocation context and that is never called from IRQ context.
>
> This makes me think more. I think that spin_lock should be sufficient. Right?
>
It seems Mike's point is that cma_release might be called from both
irq context and process context.
If it is running in process context, we need the irq-disable to lock
the irq context which might jump to call cma_release at the same time.
We have never seen cma_release has been really called in irq context
by now, anyway.
>
> >
> > > pr_info("number of available pages: ");
> > > for (;;) {
> > > next_zero_bit = find_next_zero_bit(cma->bitmap, nbits,
> > > start); @@ -407,7 +408,7 @@ static void cma_debug_show_areas(struct cma
> *cma)
> > > start = next_zero_bit + nr_zero;
> > > }
> > > pr_cont("=> %lu free of %lu total pages\n", nr_total, cma->count);
> > > - mutex_unlock(&cma->lock);
> > > + spin_unlock_irqrestore(&cma->lock, flags);
> > > }
> > > #else
> > > static inline void cma_debug_show_areas(struct cma *cma) { } @@
> > > -430,6 +431,7 @@ struct page *cma_alloc(struct cma *cma, size_t count,
> unsigned int align,
> > > unsigned long pfn = -1;
> > > unsigned long start = 0;
> > > unsigned long bitmap_maxno, bitmap_no, bitmap_count;
> > > + unsigned long flags;
> > > size_t i;
> > > struct page *page = NULL;
> > > int ret = -ENOMEM;
> > > @@ -454,12 +456,12 @@ struct page *cma_alloc(struct cma *cma, size_t count,
> unsigned int align,
> > > goto out;
> > >
> > > for (;;) {
> > > - mutex_lock(&cma->lock);
> > > + spin_lock_irqsave(&cma->lock, flags);
> > > bitmap_no = bitmap_find_next_zero_area_off(cma->bitmap,
> > > bitmap_maxno, start, bitmap_count, mask,
> > > offset);
> > > if (bitmap_no >= bitmap_maxno) {
> > > - mutex_unlock(&cma->lock);
> > > + spin_unlock_irqrestore(&cma->lock, flags);
> > > break;
> > > }
> > > bitmap_set(cma->bitmap, bitmap_no, bitmap_count);
> >
> > same here.
> >
> > > @@ -468,7 +470,7 @@ struct page *cma_alloc(struct cma *cma, size_t count,
> unsigned int align,
> > > * our exclusive use. If the migration fails we will take the
> > > * lock again and unmark it.
> > > */
> > > - mutex_unlock(&cma->lock);
> > > + spin_unlock_irqrestore(&cma->lock, flags);
> > >
> > > pfn = cma->base_pfn + (bitmap_no << cma->order_per_bit);
> > > ret = alloc_contig_range(pfn, pfn + count,
> > > MIGRATE_CMA, diff --git a/mm/cma.h b/mm/cma.h index
> > > 68ffad4e430d..2c775877eae2 100644
> > > --- a/mm/cma.h
> > > +++ b/mm/cma.h
> > > @@ -15,7 +15,7 @@ struct cma {
> > > unsigned long count;
> > > unsigned long *bitmap;
> > > unsigned int order_per_bit; /* Order of pages represented by one bit
> */
> > > - struct mutex lock;
> > > + spinlock_t lock;
> > > #ifdef CONFIG_CMA_DEBUGFS
> > > struct hlist_head mem_head;
> > > spinlock_t mem_head_lock;
> > > diff --git a/mm/cma_debug.c b/mm/cma_debug.c index
> > > d5bf8aa34fdc..6379cfbfd568 100644
> > > --- a/mm/cma_debug.c
> > > +++ b/mm/cma_debug.c
> > > @@ -35,11 +35,12 @@ static int cma_used_get(void *data, u64 *val) {
> > > struct cma *cma = data;
> > > unsigned long used;
> > > + unsigned long flags;
> > >
> > > - mutex_lock(&cma->lock);
> > > + spin_lock_irqsave(&cma->lock, flags);
> > > /* pages counter is smaller than sizeof(int) */
> > > used = bitmap_weight(cma->bitmap, (int)cma_bitmap_maxno(cma));
> > > - mutex_unlock(&cma->lock);
> > > + spin_unlock_irqrestore(&cma->lock, flags);
> > > *val = (u64)used << cma->order_per_bit;
> >
> > same here
> >
> > >
> > > return 0;
> > > @@ -52,8 +53,9 @@ static int cma_maxchunk_get(void *data, u64 *val)
> > > unsigned long maxchunk = 0;
> > > unsigned long start, end = 0;
> > > unsigned long bitmap_maxno = cma_bitmap_maxno(cma);
> > > + unsigned long flags;
> > >
> > > - mutex_lock(&cma->lock);
> > > + spin_lock_irqsave(&cma->lock, flags);
> > > for (;;) {
> > > start = find_next_zero_bit(cma->bitmap, bitmap_maxno, end);
> > > if (start >= bitmap_maxno)
> > > @@ -61,7 +63,7 @@ static int cma_maxchunk_get(void *data, u64 *val)
> > > end = find_next_bit(cma->bitmap, bitmap_maxno, start);
> > > maxchunk = max(end - start, maxchunk);
> > > }
> > > - mutex_unlock(&cma->lock);
> > > + spin_unlock_irqrestore(&cma->lock, flags);
> > > *val = (u64)maxchunk << cma->order_per_bit;
> > >
> > > return 0;
> >
> > and here.
> > --
> > Michal Hocko
> > SUSE Labs
next prev parent reply other threads:[~2021-03-30 8:17 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-29 23:23 [PATCH v2 0/8] make hugetlb put_page safe for all calling contexts Mike Kravetz
2021-03-29 23:23 ` [PATCH v2 1/8] mm/cma: change cma mutex to irq safe spinlock Mike Kravetz
2021-03-30 1:13 ` Roman Gushchin
2021-03-30 1:20 ` Song Bao Hua (Barry Song)
2021-03-30 2:18 ` Mike Kravetz
2021-03-30 8:01 ` Michal Hocko
2021-03-30 8:08 ` [External] " Muchun Song
2021-03-30 8:17 ` Song Bao Hua (Barry Song) [this message]
2021-03-30 8:18 ` Michal Hocko
2021-03-30 8:21 ` Muchun Song
2021-03-31 2:37 ` Mike Kravetz
2021-03-29 23:23 ` [PATCH v2 2/8] hugetlb: no need to drop hugetlb_lock to call cma_release Mike Kravetz
2021-03-30 1:13 ` Roman Gushchin
2021-03-30 8:01 ` Michal Hocko
2021-03-29 23:23 ` [PATCH v2 3/8] hugetlb: add per-hstate mutex to synchronize user adjustments Mike Kravetz
2021-03-30 2:23 ` [External] " Muchun Song
2021-03-29 23:23 ` [PATCH v2 4/8] hugetlb: create remove_hugetlb_page() to separate functionality Mike Kravetz
2021-03-29 23:23 ` [PATCH v2 5/8] hugetlb: call update_and_free_page without hugetlb_lock Mike Kravetz
2021-03-30 2:10 ` Miaohe Lin
2021-03-30 2:21 ` [External] " Muchun Song
2021-03-31 2:39 ` Mike Kravetz
2021-03-29 23:24 ` [PATCH v2 6/8] hugetlb: change free_pool_huge_page to remove_pool_huge_page Mike Kravetz
2021-03-30 2:30 ` [External] " Muchun Song
2021-03-30 8:06 ` Michal Hocko
2021-03-29 23:24 ` [PATCH v2 7/8] hugetlb: make free_huge_page irq safe Mike Kravetz
2021-03-29 23:24 ` [PATCH v2 8/8] hugetlb: add lockdep_assert_held() calls for hugetlb_lock 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=7087d077cb814e7abbc5a4a98ab06369@hisilicon.com \
--to=song.bao.hua@hisilicon.com \
--cc=akpm@linux-foundation.org \
--cc=almasrymina@google.com \
--cc=aneesh.kumar@linux.ibm.com \
--cc=david@redhat.com \
--cc=guro@fb.com \
--cc=hdanton@sina.com \
--cc=iamjoonsoo.kim@lge.com \
--cc=linmiaohe@huawei.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=longman@redhat.com \
--cc=mhocko@suse.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=will@kernel.org \
--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).