Message-ID: <57119844135c2b3ac5d075d077cd8c8e@codeaurora.org>
Date:   Mon, 14 Sep 2020 14:52:37 -0700
From:   Chris Goldsworthy <cgoldswo@...eaurora.org>
To:     David Hildenbrand <david@...hat.com>
Cc:     akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        pratikp@...eaurora.org, pdaly@...eaurora.org,
        sudaraja@...eaurora.org, iamjoonsoo.kim@....com,
        linux-arm-msm-owner@...r.kernel.org,
        Vinayak Menon <vinmenon@...eaurora.org>,
        linux-kernel-owner@...r.kernel.org
Subject: Re: [PATCH v2] mm: cma: indefinitely retry allocations in cma_alloc

On 2020-09-14 11:33, Chris Goldsworthy wrote:
> On 2020-09-14 02:31, David Hildenbrand wrote:
>> What about long-term pinnings? IIRC, that can happen easily e.g., with
>> vfio (and I remember there is a way via vmsplice).
>> 
>> Not convinced trying forever is a sane approach in the general case 
>> ...
> 
> Hi David,
> 
> I've botched the threading, so there are discussions with respect to
> the previous patch-set that is missing on this thread, which I will
> summarize below:
> 
> V1:
> [1] https://lkml.org/lkml/2020/8/5/1097
> [2] https://lkml.org/lkml/2020/8/6/1040
> [3] https://lkml.org/lkml/2020/8/11/893
> [4] https://lkml.org/lkml/2020/8/21/1490
> [5] https://lkml.org/lkml/2020/9/11/1072
> 
> [1] features version of the patch featured a finite number of retries,
> which has been stable for our kernels. In [2], Andrew questioned
> whether we could actually find a way of solving the problem on the
> grounds that doing a finite number of retries doesn't actually fix the
> problem (more importantly, in [4] Andrew indicated that he would
> prefer not to merge the patch as it doesn't solve the issue).  In [3],
> I suggest one actual fix for this, which is to use
> preempt_disable/enable() to prevent context switches from occurring
> during the periods in copy_one_pte() and exit_mmap() (I forgot to
> mention this case in the commit text) in which _refcount > _mapcount
> for a page - you would also need to prevent interrupts from occurring
> to if we were to fully prevent the issue from occurring.  I think this
> would be acceptable for the copy_one_pte() case, since there _refcount
> > _mapcount for little time.  For the exit_mmap() case, however, _refcount is greater than _mapcount whilst the page-tables are being torn down for a process - that could be too long for disabling preemption / interrupts.
> 
> So, in [4], Andrew asks about two alternatives to see if they're
> viable: (1) acquiring locks on the exit_mmap path and migration paths,
> (2) retrying indefinitely.  In [5], I discuss how using locks could
> increase the time it takes to perform a CMA allocation, such that a
> retry approach would avoid increased CMA allocation times. I'm also
> uncertain about how the locking scheme could be implemented
> effectively without introducing a new per-page lock that will be used
> specifically to solve this issue, and I'm not sure this would be
> accepted.
> 
> We're fine with doing indefinite retries, on the grounds that if there
> is some long-term pinning that occurs when alloc_contig_range returns
> -EBUSY, that it should be debugged and fixed.  Would it be possible to
> make this infinite-retrying something that could be enabled or
> disabled by a defconfig option?
> 
> Thanks,
> 
> Chris.

Actually, if we were willing to have a defconfig option for enabling / 
disabling indefinite retries on the return of -EBUSY, would it be 
possibly to re-structure the patch to allow either (1) indefinite 
retrying, or (2) doing a fixed number of retires (as some people might 
want to tolerate CMA allocation failures in favor of making progress)?

-- 
The Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project

Powered by blists - more mailing lists