Re: [PATCH 1/2] kmemleak: enable tracking for percpu pointers

[Pavel Tikhomirov <ptikhomirov@virtuozzo.com>]

On 7/30/24 22:15, Catalin Marinas wrote:
> Hi Pavel,
> 
> On Thu, Jul 25, 2024 at 12:12:15PM +0800, Pavel Tikhomirov wrote:
>> @@ -1308,12 +1319,23 @@ static bool update_checksum(struct kmemleak_object *object)
>>   {
>>   	u32 old_csum = object->checksum;
>>   
>> -	if (WARN_ON_ONCE(object->flags & (OBJECT_PHYS | OBJECT_PERCPU)))
>> +	if (WARN_ON_ONCE(object->flags & OBJECT_PHYS))
>>   		return false;
>>   
>>   	kasan_disable_current();
>>   	kcsan_disable_current();
>> -	object->checksum = crc32(0, kasan_reset_tag((void *)object->pointer), object->size);
>> +	if (object->flags & OBJECT_PERCPU) {
>> +		unsigned int cpu;
>> +
>> +		object->checksum = 0;
>> +		for_each_possible_cpu(cpu) {
>> +			void *ptr = per_cpu_ptr((void __percpu *)object->pointer, cpu);
>> +
>> +			object->checksum ^= crc32(0, kasan_reset_tag((void *)ptr), object->size);
>> +		}
> 
> Slight worry this may take too long for large-ish objects with a large
> number of CPUs. But we can revisit if anyone complains.
> 
>> +	} else {
>> +		object->checksum = crc32(0, kasan_reset_tag((void *)object->pointer), object->size);
>> +	}
>>   	kasan_enable_current();
>>   	kcsan_enable_current();
>>   
>> @@ -1365,6 +1387,64 @@ static int scan_should_stop(void)
>>   	return 0;
>>   }
>>   
>> +static void scan_pointer(struct kmemleak_object *scanned,
>> +			 unsigned long pointer, unsigned int objflags)
> 
> Nitpick: I'd have called this lookup_pointer or something like that.
> When I first saw it, I tried to figure out why it doesn't have a size
> argument but it became clear that it's not actually scanning/reading the
> location at 'pointer' but simply looking the value up in various trees.
> Up to you if you want to change the name to something else. The patch
> looks good.

Yes, I agree "scan_" is a bit too confusing. Let's try 
"pointer_update_refs", as it is more self-explanatory, and represents 
what the function does. The "lookup_pointer" seems like a function which 
should return a pointer, e.g. like "lookup_object" returns an object, 
but we can always switch to "lookup_pointer" if you like.

I will send v2 with this change shortly.

> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> 
> Thanks.
> 

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.