Re: [RFC V2 3/3] mm: Replace pgtable entry prints with new format

[David Hildenbrand (Arm)]

On 7/2/26 06:29, Anshuman Khandual wrote:
> 
> 
> On 30/06/26 7:06 PM, David Hildenbrand (Arm) wrote:
>> On 6/16/26 08:19, Hugh Dickins wrote:
>>>
>>> Yes, that's what it's for. What we really want is to understand what went
>>> wrong: that's too much to ask of a printk, but it can give us a good clue.
>>>
>>>
>>> Page table entry and pmd entry are good enough: higher levels got
>>> added at some stage, but they are unlikely to be useful here.
>>
>> Yes, I added them when we're processing PUD entries we'd also want
>> P4D entry + PUD entry.
>>
>> This is one approach of having the printing be done mostly
>> manually, supporting 32, 64 and 128bit pte_val(). As raised by Ryan,
>> using local bufs to store the data to not involve printk.
>>
>>
>> I played with printing the byte stream manually, but didn't really like it.
>>
>> Gave it a quick test and it seems to do its trick. I have the feeling that
>> this can be beautified a bit more.
>>
>>
>> From 05af7317b126991a61b0a3d01c2863ce5a578d1b Mon Sep 17 00:00:00 2001
>> From: "David Hildenbrand (Arm)" <david@xxxxxxxxxx>
>> Date: Tue, 30 Jun 2026 15:23:02 +0200
>> Subject: [PATCH] tmp
>>
>> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>> ---
>>  mm/memory.c | 110 +++++++++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 87 insertions(+), 23 deletions(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index ff338c2abe923..ad39cafe110f9 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -519,9 +519,57 @@ static bool is_bad_page_map_ratelimited(void)
>>  	return false;
>>  }
>>  
>> +#define PTVAL_STR_MAX	(sizeof(u64) * 4 + 1)
>> +
>> +static void ptval_bytes_to_str(char *buf, size_t buf_size,
>> +		const void *entry, size_t entry_size)
>> +{
>> +	if (WARN_ON_ONCE(buf_size < entry_size * 2 + 1)) {
>> +		snprintf(buf, buf_size, "overflow");
>> +		return;
>> +	}
>> +
>> +	switch (entry_size) {
>> +	case sizeof(u32):
>> +		snprintf(buf, buf_size, "%08x", *(const u32 *)entry);
>> +		break;
>> +	case sizeof(u64):
>> +		snprintf(buf, buf_size, "%016llx",
>> +			 (unsigned long long)*(const u64 *)entry);
>> +		break;
>> +	case sizeof(u64) * 2: {
> 
> Could this be made sizeof(u128) instead ? But overall this
> approach looks good.

The would be cleaner. We might have to protect this case by something like

#defined(__SIZEOF_INT128__)
	case sizeof(u128):
		...
		break;
#endif
	default:

	...

Can you take over this approach and refine it (and address Andy's comments)?

I'm not quite happy about the

	typeof(pud_val(pud)) entry = pud_val(pud);

stuff, but I didn't see an easy (less ugly) way to avoid it. Maybe there is one :)

-- 
Cheers,

David