Re: [Xen-devel] [XEN PATCH v2 1/2] Check zone before merging adjacent blocks in heap

From: David Woodhouse <dwmw2@infradead.org>
To: Julien Grall <julien@xen.org>,
	George Dunlap <george.dunlap@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Jeff Kubascik <jeff.kubascik@dornerworks.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [XEN PATCH v2 1/2] Check zone before merging adjacent blocks in heap
Date: Wed, 05 Feb 2020 11:36:41 +0000	[thread overview]
Message-ID: <5fb3a88ee20e7bea8418680f3353982424dd816b.camel@infradead.org> (raw)
In-Reply-To: <9d1c6e2b-f155-96f9-deeb-1b1f30c94081@xen.org>

[-- Attachment #1.1: Type: text/plain, Size: 1565 bytes --]

On Wed, 2020-02-05 at 10:22 +0000, Julien Grall wrote:
> 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.

We'd need an extra bit for that.

Perhaps we could recognise that PGC_broken is not valid in combination
with some of the four PGC_state states, and take that bit back to turn
PGC_state into a 3-bit field, which could hold 8 states of which I
think we only currently need 7?

 { zero, inuse, offlining, offlining_broken, offline, offline_broken, free }

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

We'd zero it in init_heap_pages() for every page, and it would only be
still set to all 1s for pages from the boot allocator; as long as all
1s *is* proven to be an invalid state (I think it is); then the same
kind of trick checking unlikely(pg->count_info == -1) in the free
functions as in my earlier straw man, would probably work.

I think I prefer zero being the "untouched" state though, rather than
all 1s. 

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel