On 12.01.22 20:15, Kirill A. Shutemov wrote: > On Wed, Jan 12, 2022 at 12:31:10PM +0100, David Hildenbrand wrote: >> >>> >>> Looking at stuff like this, I can't help but think that a: >>> >>> #define PageOffline PageUnaccepted >>> >>> and some other renaming would be a fine idea. I get that the Offline >>> bit can be reused, but I'm not sure that the "Offline" *naming* should >>> be reused. What you're doing here is logically distinct from existing >>> offlining. >> >> Yes, or using a new pagetype bit to make the distinction clearer. >> Especially the function names like maybe_set_page_offline() et. Al are >> confusing IMHO. They are all about accepting unaccepted memory ... and >> should express that. > > "Unaccepted" is UEFI treminology and I'm not sure we want to expose > core-mm to it. Power/S390/ARM may have a different name for the same > concept. Offline/online is neutral terminology, familiar to MM developers. Personally, I'd much rather prefer clear UEFI terminology for now than making the code more confusing to get. We can always generalize later iff there are similar needs by other archs (and if they are able to come up witha better name). But maybe we can find a different name immediately. The issue with online vs. offline I have is that we already have enough confusion: offline page: memory section is offline. These pages are not managed by the buddy. The memmap is stale unless we're dealing with special ZONE_DEVICE memory. logically offline pages: memory section is online and pages are PageOffline(). These pages were removed from the buddy e.g., to free them up in the hypervisor. soft offline pages: memory section is online and pages are PageHWPoison(). These pages are removed from the buddy such that we cannot allocate them to not trigger MCEs. offline pages are exposed to the buddy by onlining them (generic_online_page()), which is init+freeing. PageOffline() and PageHWPoison() are onlined by removing the flag and freeing them to the buddy. Your case is different such that the pages are managed by the buddy and they don't really have online/offline semantics compared to what we already have. All the buddy has to do is prepare them for initial use. I'm fine with reusing PageOffline(), but for the purpose of reading the code, I think we really want some different terminology in page_alloc.c So using any such terminology would make it clearer to me: * PageBuddyUnprepared() * PageBuddyUninitialized() * PageBuddyUnprocessed() * PageBuddyUnready() > > What if I change accept->online in function names and document the meaning > properly? > >> I assume PageOffline() will be set only on the first sub-page of a >> high-order PageBuddy() page, correct? >> >> Then we'll have to monitor all PageOffline() users such that they can >> actually deal with PageBuddy() pages spanning *multiple* base pages for >> a PageBuddy() page. For now it's clear that if a page is PageOffline(), >> it cannot be PageBuddy() and cannot span more than one base page. > >> E.g., fs/proc/kcore.c:read_kcore() assumes that PageOffline() is set on >> individual base pages. > > Right, pages that offline from hotplug POV are never on page allocator's > free lists, so it cannot ever step on them. > -- Thanks, David / dhildenb
