Re: [External] Re: [PATCH v2 1/8] mm/cma: change cma mutex to irq safe spinlock

[Muchun Song]

On Tue, Mar 30, 2021 at 4:01 PM Michal Hocko <mhocko@xxxxxxxx> 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?


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