Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <8fc24ba3-1467-b845-a70e-7ed77750e6cd@gmail.com>
Date:   Tue, 7 Jun 2022 22:32:26 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v2 1/4] mm: kmemleak: add OBJECT_PHYS flag for objects
 allocated with physical address
Content-Language: en-US
To:     Catalin Marinas <catalin.marinas@arm.com>
Cc:     akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, yee.lee@mediatek.com
References: <20220603035415.1243913-1-patrick.wang.shcn@gmail.com>
 <20220603035415.1243913-2-patrick.wang.shcn@gmail.com>
 <Yp3rO4WpLzxLA6+7@arm.com>
From:   Patrick Wang <patrick.wang.shcn@gmail.com>
In-Reply-To: <Yp3rO4WpLzxLA6+7@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2022/6/6 19:55, Catalin Marinas wrote:
> On Fri, Jun 03, 2022 at 11:54:12AM +0800, Patrick Wang wrote:
>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
>> index a182f5ddaf68..1e9e0aa93ae5 100644
>> --- a/mm/kmemleak.c
>> +++ b/mm/kmemleak.c
>> @@ -172,6 +172,8 @@ struct kmemleak_object {
>>   #define OBJECT_NO_SCAN		(1 << 2)
>>   /* flag set to fully scan the object when scan_area allocation failed */
>>   #define OBJECT_FULL_SCAN	(1 << 3)
>> +/* flag set for object allocated with physical address */
>> +#define OBJECT_PHYS		(1 << 4)
>>   
>>   #define HEX_PREFIX		"    "
>>   /* number of bytes to print per line; must be 16 or 32 */
>> @@ -575,7 +577,8 @@ static int __save_stack_trace(unsigned long *trace)
>>    * memory block and add it to the object_list and object_tree_root.
>>    */
>>   static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
>> -					     int min_count, gfp_t gfp)
>> +					     int min_count, gfp_t gfp,
>> +					     bool is_phys)
> 
> The patch looks fine but I wonder whether we should have different
> functions for is_phys true/false, we may end up fewer changes overall
> since most places simply pass is_phys == false:

This should be better. Will do.

> 
> static struct kmemleak_object *__create_object(unsigned long ptr, size_t size,
> 					       int min_count, gfp_t gfp,
> 					       bool is_phys);
> 
> static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
> 					     int min_count, gfp_t gfp)
> {
> 	return __create_object(ptr, size, min_count, gfp, false);
> }
> 
> static struct kmemleak_object *create_object_phys(unsigned long ptr, size_t size,
> 						  int min_count, gfp_t gfp)
> {
> 	return __create_object(ptr, size, min_count, gfp, true);
> }
> 
> Same for the other patches that change a few more functions.

Since the physical objects are only used as gray objects.
Changes on irrelevant places will be removed.

The leak check could be taken on physical objects. Conversion
of block address from virtual to physical before lookup should
make this work (this is useless currently). I think we'd better
know about this.

Thanks,
Patrick