Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <be3c0a7d-91f1-ab55-f560-2ece4720d405@gmail.com>
Date:   Tue, 7 Jun 2022 22:34:21 +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 2/4] mm: kmemleak: add rbtree 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-3-patrick.wang.shcn@gmail.com>
 <Yp4RgegLBhvVeaid@arm.com>
From:   Patrick Wang <patrick.wang.shcn@gmail.com>
In-Reply-To: <Yp4RgegLBhvVeaid@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2022/6/6 22:38, Catalin Marinas wrote:
> On Fri, Jun 03, 2022 at 11:54:13AM +0800, Patrick Wang wrote:
>> @@ -536,27 +543,32 @@ static struct kmemleak_object *find_and_get_object(unsigned long ptr, int alias)
>>   }
>>   
>>   /*
>> - * Remove an object from the object_tree_root and object_list. Must be called
>> - * with the kmemleak_lock held _if_ kmemleak is still enabled.
>> + * Remove an object from the object_tree_root (or object_phys_tree_root)
>> + * and object_list. Must be called with the kmemleak_lock held _if_ kmemleak
>> + * is still enabled.
>>    */
>>   static void __remove_object(struct kmemleak_object *object)
>>   {
>> -	rb_erase(&object->rb_node, &object_tree_root);
>> +	rb_erase(&object->rb_node, object->flags & OBJECT_PHYS ?
>> +				   &object_phys_tree_root :
>> +				   &object_tree_root);
> 
> This pattern appears in a few place, I guess it's better with a macro,
> say get_object_tree_root(object). But see how many are left, I have some
> comments below on reducing the diff.

Will do.

> 
>> @@ -709,12 +724,12 @@ static void delete_object_full(unsigned long ptr)
>>    * delete it. If the memory block is partially freed, the function may create
>>    * additional metadata for the remaining parts of the block.
>>    */
>> -static void delete_object_part(unsigned long ptr, size_t size)
>> +static void delete_object_part(unsigned long ptr, size_t size, bool is_phys)
>>   {
>>   	struct kmemleak_object *object;
>>   	unsigned long start, end;
>>   
>> -	object = find_and_remove_object(ptr, 1);
>> +	object = find_and_remove_object(ptr, 1, is_phys);
>>   	if (!object) {
>>   #ifdef DEBUG
>>   		kmemleak_warn("Partially freeing unknown object at 0x%08lx (size %zu)\n",
> 
> The previous patch introduced a check on object->flags for
> delete_object_part(). I think you can just use is_phys directly now when
> calling create_object().

Will do.

> 
>> @@ -1275,7 +1290,7 @@ static void scan_block(void *_start, void *_end,
>>   		 * is still present in object_tree_root and object_list
>>   		 * (with updates protected by kmemleak_lock).
>>   		 */
>> -		object = lookup_object(pointer, 1);
>> +		object = lookup_object(pointer, 1, false);
>>   		if (!object)
>>   			continue;
>>   		if (object == scanned)
>> @@ -1299,7 +1314,7 @@ static void scan_block(void *_start, void *_end,
>>   		raw_spin_unlock(&object->lock);
>>   
>>   		if (excess_ref) {
>> -			object = lookup_object(excess_ref, 0);
>> +			object = lookup_object(excess_ref, 0, false);
>>   			if (!object)
>>   				continue;
>>   			if (object == scanned)
>> @@ -1728,7 +1743,7 @@ static int dump_str_object_info(const char *str)
>>   
>>   	if (kstrtoul(str, 0, &addr))
>>   		return -EINVAL;
>> -	object = find_and_get_object(addr, 0);
>> +	object = find_and_get_object(addr, 0, false);
>>   	if (!object) {
>>   		pr_info("Unknown object at 0x%08lx\n", addr);
>>   		return -EINVAL;
> 
> I think find_and_get_object() is never called on a phys object, so you
> can probably simplify these a bit. Just add an is_phys argument where
> strictly necessary and maybe even add a separate function like
> lookup_object_phys() to reduce the other changes.

Will add lookup_object_phys() function and find_and_get_object_phys()
function. The find_and_get_object() function is also called in many
places.

Thanks,
Patrick