Re: [RFC PATCH v2 3/8] mm: Hot page tracking and promotion

[Bharata B Rao]

On 03-Oct-25 4:47 PM, Jonathan Cameron wrote:
> On Wed, 10 Sep 2025 20:16:48 +0530
> Bharata B Rao <bharata@xxxxxxx> wrote:
>> +
>> +struct max_heap {
>> +	size_t nr;
>> +	size_t size;
>> +	struct pghot_info **data;
>> +	DECLARE_FLEX_ARRAY(struct pghot_info *, preallocated);
> 
> That macro is all about use in unions rather than generally being needed.
> Do you need that here rather than
> 	struct pg_hot_info *preallocated[];
> 
> Can you add a __counted_by() marking?

I was using DEFINE_MIN_HEAP macro earlier which gave problems
when I had to define per-node instance of the heap with the
same name. The workaround for that resulted in the use of above
flex array.

Not needed, I will revert back to using array of pointers with
__counted_by() marking.

>> +
>> +	for (i = 0; i < hash_size; i++) {
>> +		INIT_HLIST_HEAD(&phi_hash[i].hash);
>> +		spin_lock_init(&phi_hash[i].lock);
>> +	}
>> +
>> +	phi_cache = KMEM_CACHE(pghot_info, 0);
>> +	if (unlikely(!phi_cache)) {
>> +		ret = -ENOMEM;
>> +		goto out;
> Whilst not strictly necessary I'd add multiple labels so only things
> that have been allocated are free rather than relying on them being
> NULL otherwise.  Whilst not a correctness thing it makes it slightly
> easier to check tear down paths are correct.

In general I agree but for freeing with a loop exit, the current
method appeared much simpler.

I will take care of rest of the review comments.

Regards,
Bharata.