Re: [PATCH v3 4/7] mm: kmemleak: split __create_object into two functions

[Catalin Marinas]

Thanks for this, it looks better.

On Wed, Oct 18, 2023 at 06:29:49PM +0800, Liu Shixin wrote:
> -/*
> - * Create the metadata (struct kmemleak_object) corresponding to an allocated
> - * memory block and add it to the object_list and object_tree_root (or
> - * object_phys_tree_root).
> - */
> -static void __create_object(unsigned long ptr, size_t size,
> -			    int min_count, gfp_t gfp, bool is_phys)
> +static struct kmemleak_object * __alloc_object(gfp_t gfp)
>  {
> -	unsigned long flags;
> -	struct kmemleak_object *object, *parent;
> -	struct rb_node **link, *rb_parent;
> -	unsigned long untagged_ptr;
> -	unsigned long untagged_objp;
> +	struct kmemleak_object *object;
>  
>  	object = mem_pool_alloc(gfp);
>  	if (!object) {
>  		pr_warn("Cannot allocate a kmemleak_object structure\n");
>  		kmemleak_disable();
> -		return;
> +		return NULL;
>  	}
>  
>  	INIT_LIST_HEAD(&object->object_list);
> @@ -649,13 +639,8 @@ static void __create_object(unsigned long ptr, size_t size,
>  	INIT_HLIST_HEAD(&object->area_list);
>  	raw_spin_lock_init(&object->lock);
>  	atomic_set(&object->use_count, 1);
> -	object->flags = OBJECT_ALLOCATED | (is_phys ? OBJECT_PHYS : 0);
> -	object->pointer = ptr;
> -	object->size = kfence_ksize((void *)ptr) ?: size;
>  	object->excess_ref = 0;
> -	object->min_count = min_count;
>  	object->count = 0;			/* white color initially */
> -	object->jiffies = jiffies;
>  	object->checksum = 0;
>  	object->del_state = 0;

I'd keep all the initialisation in one place even if it means passing
more arguments to __alloc_object(). It feels a bit weird and error prone
to split the initialisation in two places. Otherwise I'm fine with the
split.

-- 
Catalin