Re: [PATCH v3 13/26] mm: introduce freetype_t

[Vlastimil Babka (SUSE)]

On 8/18/26 02:35, Yosry Ahmed wrote:
>> >> +static inline
>> >> +struct list_head *free_area_list(struct free_area *area, freetype_t type)
>> >> +{
>> >> +	int idx = freetype_idx(type);
>> >> +
>> >> +	VM_WARN_ON(idx < 0);
>> >> +	return &area->free_list[idx];
>> >
>> > Should we return NULL here if idx < 0 instead of an out of bounds
>> > access?
>> 
>> TBH my descending order of preference is:
>> 
>> 1.
>> 
>> BUG_ON(idx < 0);
>> return &area->free_list[idx];
>> 
>> 2.
>> 
>> if (WARN_ON(idx < 0)) // OR VM_WARN_ON
>>   return NULL;
>> return &area->free_list[idx];
>> 
>> 3.
>> 
>> VM_WARN_ON(idx < 0);
>> return &area->free_list[idx];
>> 
>> 4. 
>> 
>> return &area->free_list[idx];
>> 
>> But I suspect[0] Vlastimil (and Linus) would order it the exact opposite
>> way.
>> 
>> [0]: https://lore.kernel.org/all/bd36e972-8900-4476-a66b-4dc218b21a4d@xxxxxxxxxx/
>> 
>> And I care more about making Vlastimil (and Linus) happy than this tiny
>> detail of the code, so I'll defer to him. The current style is a
>> compromise, but maybe it's just a compromise that makes nobody happy.
> 
> I think I prefer #2 (regardless of whether it's WARN_ON() or
> VM_WARN_ON()), let's see if anyone disagrees.

#2 can't work with VM_WARN_ON as it returns void. Otherwise if it evaluated
the cond with DEBUG_VM and hardcoded false otherwise, it would be ideal. I
don't remember why it's not.

Do the callers all handle NULL safely? In that case #2 has some merit (maybe
with WARN_ON_ONCE()) otherwise #3