[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <832d4026-c88f-876b-5f95-4d573bb9d1dd@redhat.com>
Date: Fri, 14 Apr 2023 12:31:43 +0200
From: David Hildenbrand <david@...hat.com>
To: Michal Hocko <mhocko@...e.com>,
Mel Gorman <mgorman@...hsingularity.net>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Vlastimil Babka <vbabka@...e.cz>,
Oscar Salvador <OSalvador@...e.com>,
Yuanxi Liu <y.liu@...uida.com>, Linux-MM <linux-mm@...ck.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm: page_alloc: Assume huge tail pages are valid when
allocating contiguous pages
On 14.04.23 12:19, Michal Hocko wrote:
> On Fri 14-04-23 10:52:04, Mel Gorman wrote:
>> On Fri, Apr 14, 2023 at 10:55:04AM +0200, Michal Hocko wrote:
>>> On Fri 14-04-23 09:22:22, Mel Gorman wrote:
>>> [...]
>>>> +
>>>> + /*
>>>> + * Do not migrate huge pages that span the size of the region
>>>> + * being allocated contiguous. e.g. Do not migrate a 1G page
>>>> + * for a 1G allocation request. CMA is an exception as the
>>>> + * region may be reserved for hardware that requires physical
>>>> + * memory without a MMU or scatter/gather capability.
>>>> + *
>>>> + * Note that the compound check is race-prone versus
>>>> + * free/split/collapse but it should be safe and result in
>>>> + * a premature skip or a useless migration attempt.
>>>> + */
>>>> + if (PageHuge(page) && compound_nr(page) >= nr_pages &&
>>>> + !is_migrate_cma_page(page)) {
>>>> + return false;
>>>
>>> Is the CMA check working as expected?
>>
>> I didn't test it as I don't have a good simulator for CMA contraints which
>> is still a mobile phone concern for devices like cameras.
>>
>>> The function sounds quite generic
>>> and I agree that it would make sense if it was generic but it is used
>>> only for GB pages in fact and unless I am missing something it would
>>> allow to migrate CMA pages and potentially allocate over that region
>>> without any possibility to migrate GB page out so the CMA region would
>>> be essentially unusable for CMA users.
>>
>> It's used primarily for 1G pages but does have other users (debugging
>> mostly, low priority). As it's advertised as a general API, I decided to
>> treat it as such and that meant being nice to CMA if possible. If CMA pages
>> migrate but can still use the target location then it should be fine. If a
>> CMA can migrate to an usable location that breaks a device then that's a bug.
>>
>>> GB pages already have their CMA
>>> allocator path before we get to alloc_contig_pages. Or do I miss
>>> something?
>>
>> I don't think you missed anything. The CMA check is, at best, an effort
>> to have a potentially useful semantic but it's very doubtful anyone will
>> notice or care. I'm perfectly happy just to drop the CMA check because it's a
>> straight-forward fix and more suitable as a -stable backport. I'm also happy
>> to just go with a PageHuge check and ignore any possibility that a 2M page
>> could be migrated to satisfy a 1G allocation. 1G allocation requests after
>> significant uptime is a crapshoot at best and relying on them succeeding is
>> unwise. There is a non-zero possibility that the latency incurred migrating
>> 2M pages and still failing a 1G allocation could itself be classed as a
>> bug with users preferring fast-failure of 1G allocation attempts.
>
> Yes, the simpler the better. If we encounter a real usecase where couple
> of 2MB hugetlb pages stand in the way to GB pages then we can add the
> check so I would just go with reintroducing the PageHuge check alone.
alloc_contig_pages() -> __alloc_contig_pages() ->
alloc_contig_range(MIGRATE_MOVABLE)
Should always fail when stumbling over MIGRATE_CMA pageblocks IIRC.
So we could bail out in that function early if we stumble over any CMA
region.
--
Thanks,
David / dhildenb
Powered by blists - more mailing lists