On 2021/06/10 2:25, Bart Van Assche wrote:
> On 6/8/21 10:03 PM, Damien Le Moal wrote:
>> On 2021/06/09 8:07, Bart Van Assche wrote:
>>> struct deadline_data {
>>> /*
>>> * run time data
>>> */
>>>
>>> /*
>>> - * requests (deadline_rq s) are present on both sort_list and fifo_list
>>> + * Requests are present on both sort_list[] and fifo_list[][]. The
>>> + * first index of fifo_list[][] is the I/O priority class (DD_*_PRIO).
>>> + * The second index is the data direction (rq_data_dir(rq)).
>>> */
>>> struct rb_root sort_list[DD_DIR_COUNT];
>>> - struct list_head fifo_list[DD_DIR_COUNT];
>>> + struct list_head fifo_list[DD_PRIO_COUNT][DD_DIR_COUNT];
>>
>> Would it make sense to pack these 2 into a sub structure ? e.g.:
>>
>> struct deadline_lists {
>> struct rb_root sort_list;
>> struct list_head fifo_list[DD_PRIO_COUNT];
>> };
>>
>> struct deadline_data {
>> ...
>> /*
>> * Requests are present on both sort_list[] and fifo_list[][]. The
>> * first index of fifo_list[][] is the I/O priority class (DD_*_PRIO).
>> * The second index is the data direction (rq_data_dir(rq)).
>> */
>> struct deadline_lists lists[DD_DIR_COUNT];
> The deadline_fifo_request() function and several other functions
> examine both directions so I think that grouping per direction would
> complicate these functions. Grouping per I/O priority might help to make
> these functions easier to read. Do you want me to look further into this?
If the code is simplified, yes, let's do it ! And Hannes will be happy as the
array of arrays will be removed :)
>>> + struct deadline_data *dd = q->elevator->elevator_data;
>>> + const u8 ioprio_class = dd_rq_ioclass(next);
>>> + const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
>>> +
>>> + if (next->elv.priv[0]) {
>>> + dd_count(dd, merged, prio);
>>> + } else {
>>> + WARN_ON_ONCE(true);
>>> + }
>>
>> No need for the curly brackets I think.
>
> I can leave these out but you may want to know that leaving the curly
> brackets out from this patch will make it necessary to introduce these
> in the next patch in this series.
>
>>> +/* Number of requests queued for a given priority level. */
>>> +static u32 dd_queued(struct deadline_data *dd, enum dd_prio prio)
>>> +{
>>> + return dd_sum(dd, inserted, prio) - dd_sum(dd, completed, prio);
>>
>> This also includes requests that are being executed on the device. Is that OK ?
>
> Yes, and that's on purpose. Please note that this function is only used
> to export statistics to user space.
Yes, I got it that it is only for stats shown to the user. I am just wondering
if we need to separate in-flight requests and queued requests. That should be
easy to do since you also have the dispatched count.
But again, not entirely sure if it is useful to have such level of detail. So I
guess it is OK as is for now. We can revisit later if needed.
>
> Thanks,
>
> Bart.
>
--
Damien Le Moal
Western Digital Research
