Re: [GIT PULL] hotfixes for 6.3-rc1

From: Vlastimil Babka <vbabka@suse.cz>
To: "Huang, Ying" <ying.huang@intel.com>,
	Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	linux-mm@kvack.org, mm-commits@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Matthew Wilcox <willy@infradead.org>
Subject: Re: [GIT PULL] hotfixes for 6.3-rc1
Date: Wed, 8 Mar 2023 11:39:28 +0100	[thread overview]
Message-ID: <43f35191-9147-0aec-cb57-0bc14d041039@suse.cz> (raw)
In-Reply-To: <87jzzu7jt9.fsf@yhuang6-desk2.ccr.corp.intel.com>

On 3/6/23 02:25, Huang, Ying wrote:
> Hi, Linus,
> 
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
>> On Sat, Mar 4, 2023 at 3:21 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>>>
>>> Ah. Ying did it this way:
>>
>> Yeah, I saw that patch flying past, but I actually think that it only
>> silences the warning almost by mistake. There's nothing fundamental in
>> there that a compiler wouldn't just follow across two assignments, and
>> it just happens to now not trigger any more.
>>
>> Assigning to a union entry is a more fundamental operation in that
>> respect. Not that the compiler still doesn't see that it's assigning a
>> value that in the end is not really type compatible, so a different
>> version of gcc could still warn, but at that point I feel like it's
>> more of an actual compiler bug than just "oh, the compiler didn't
>> happen to follow the cast through a temporary".
> 
> Yes.  Your fix is much better.  This can be used for
> __page_set_anon_rmap() family too to make the code look better?

Those are trickier due to the PAGE_MAPPING_ANON tagging bit which your
code doesn't need to handle because you can simply store an untagged
anon_vma pointer. Otherwise we could have the union at the struct page
level already (but probably not at this point as IIRC Matthew is
planning to have completely separate types for anon and file folios).

So right now we have e.g. folio_get_anon_vma() using unsigned long as
the intermediate variable, page_move_anon_rmap() using a void *
variable, __page_set_anon_rmap() casting through a (void *) ... Is there
a single recommended way for "tagged pointers" that we could unify that to?

> Best Regards,
> Huang, Ying
> 