Re: [PATCH v5 05/11] block: mq-deadline: Clean up deadline_check_fifo()

[Damien Le Moal <dlemoal@kernel.org>]

On 5/18/23 00:01, Bart Van Assche wrote:
> On 5/16/23 18:02, Damien Le Moal wrote:
>> On 5/17/23 07:33, Bart Van Assche wrote:
>>> -	if (time_after_eq(jiffies, (unsigned long)rq->fifo_time))
>>> -		return 1;
>>> -
>>> -	return 0;
>>> +	return time_is_before_eq_jiffies((unsigned long)rq->fifo_time);
>>
>> This looks wrong: isn't this reversing the return value ?
>> Shouldn't this be:
>>
>> 	return time_after_eq(jiffies, (unsigned long)rq->fifo_time));
>>
>> To return true if the first request in fifo list *expired* as indicated by the
>> function kdoc comment.
> 
> Hi Damien,
> 
>  From include/linux/jiffies.h:
> 
> #define time_is_before_eq_jiffies(a) time_after_eq(jiffies, a)

Thanks for clarifying. However, it begs the question: is this macro name correct
at all ? Why does "after" is changed to "before" ? That seems bogus to me at
worst and super confusing at best. This macro should really be:

#define time_after_eq_jiffies(a) time_after_eq(jiffies, a)

> 
> Hence, time_is_before_eq_jiffies((unsigned long)rq->fifo_time) is 
> equivalent to time_after_eq(jiffies, (unsigned long)rq->fifo_time). Both 
> expressions check whether or not rq->fifo_time is in the past.
> 
> Bart.

-- 
Damien Le Moal
Western Digital Research