|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [XEN PATCH v2 1/2] Check zone before merging adjacent blocks in heap
Hi, On 05/02/2020 09:50, David Woodhouse wrote: On Tue, 2020-02-04 at 15:37 +0000, George Dunlap wrote:At very least it's more robust this way; the algorithm is also less "magic". We just had a long discussion this morning trying to re-create the logic for why "Remove MFN 0" was sufficient to prevent this issue, and even then David wasn't sure it was correct at first.Right. So the real reason I'm staring hard at this is because I can't convince myself there aren't places where memory allocated by the boot allocator is later freed with free_xenheap_pages(). We have a few pieces of code which decide whether to use the boot allocator vs. heap based on system_state >= SYS_STATE_boot, and *if* the rule is "thou shalt not allocate with boot allocator and free later" then it's *extremely* fragile and probably being violated — especially because it actually *works* most of the time, except in some esoteric corner cases like MFN#0, boot allocations which cross zones/regions, etc. So because we want to make that *more* likely by allowing vmap() to happen earlier, I'd like to clean things up by addressing those corner cases and making it unconditionally OK to free boot-allocated pages into the heap. I think might be as simple as checking for (first_pg)->count_info == 0 in free_xenheap_pages(). That's quick enough, and if the count_info is zero then I think it does indicate a boot-allocated page, because pages from alloc_xenheap_pages() would have PGC_xen_heap set? It would suffice just to pass such pages to init_heap_pages() instead of directly to free_heap_pages(), I think. Julien? The straw man version of that looks a bit like this... --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -2304,6 +2304,12 @@ void free_xenheap_pages(void *v, unsigned int order)pg = virt_to_page(v); + /* Pages from the boot allocator need to pass through init_heap_pages() */+ if ( unlikely(!pg->count_info) ) Note that there is two versions of free_xenheap_pages(). This one only cover the case where the domheap and xenheap are the same. But you can't use the same trick when xenheap is separated (like on Arm32) because PGC_xen_heap is not set. So you would be calling init_heap_pages() everytime. However, I don't like the idea of relying on count_info == 0. Indeed, there are valid case where count_info == 0 because it means the page is inuse (PCC_state_inuse). It might be best if we introduce a new page state that would be the default value set in the frametable. 0 may be an option if we assign a different value to PGC_state_inuse but we would need to check if there are places relying on PGC_state_inuse == 0. I know that assign_pages() does rely on it, I have sent a patch for it yesterday. Another option is to use an all 1s value and initialize the frametable to all 1s. But I am not entirely sure whether the allocator would be able to cope with it. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |