Re: [PATCH v7 08/14] xen/page_alloc: introduce preserved page flags macro

From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>,
	Carlo Nonato <carlo.nonato@minervasys.tech>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v7 08/14] xen/page_alloc: introduce preserved page flags macro
Date: Wed, 27 Mar 2024 13:48:32 +0000	[thread overview]
Message-ID: <cdee3c70-c1cc-4787-8d75-ac00c95463b7@xen.org> (raw)
In-Reply-To: <fe433260-c8c9-419d-8b05-50e2cc50d127@suse.com>

On 27/03/2024 13:38, Jan Beulich wrote:
> On 27.03.2024 14:28, Julien Grall wrote:
>> Hi Carlo,
>>
>> On 27/03/2024 11:10, Carlo Nonato wrote:
>>> Hi guys,
>>>
>>>> Question is: How would you justify such a change? IOW I'm not convinced
>>>> (yet) this wants doing there.
>>>
>>> You mean in this series?
>>>
>>>> Looking at the code, the flag is originally set in
>>>> alloc_domheap_pages(). So I guess it would make sense to do it in
>>>> free_domheap_pages().
>>>
>>> We don't hold the heap_lock there.
>> Regardless of the safety question (I will answer below), count_info is
>> not meant to be protected by heap_lock. The lock is protecting the heap
>> and ensure we are not corrupting them when there are concurrent call to
>> free_heap_pages().
>>
>>   > Is it safe to change count_info without it?
>>
>> count_info is meant to be accessed locklessly. The flag PGC_extra cannot
>> be set/clear concurrently because you can't allocate a page that has not
>> yet been freed.
>>
>> So it would be fine to use clear_bit(..., ...);
> 
> Actually we hardly ever use clear_bit() on count_info. Normally we use
> ordinary C operators.

I knew you would say that. I am not convince it is safe to always using 
count_info without any atomic operations. But I never got around to 
check all them.

> Atomic (and otherwise lockless) updates are useful
> only if done like this everywhere.

You are right. But starting to use the bitops is not going to hurt 
anyone (other than maybe performance, but once we convert all of them, 
then this will become moot). In fact, it helps start to slowly move 
towards the aim to have count_info safe.

Cheers,

-- 
Julien Grall