On 12/14/18 12:11 PM, Bart Van Assche wrote:
> On Fri, 2018-12-14 at 19:39 +0800, Ming Lei wrote:
>> static void print_stat(struct seq_file *m, struct blk_rq_stat *stat)
>> {
>> @@ -856,6 +857,15 @@ int blk_mq_debugfs_register(struct request_queue *q)
>> goto err;
>> }
>>
>> + if (q->rq_qos) {
>> + struct rq_qos *rqos = q->rq_qos;
>> +
>> + while (rqos) {
>> + blk_mq_debugfs_register_rqos(rqos);
>> + rqos = rqos->next;
>> + }
>> + }
>
> Have you considered to use a for-loop instead of a while loop? That would
> allow to remove the if-statement and hence would make the code more compact.
It would fit better with the style in that code do to:
rqos = q->rq_qos;
while (rqos) {
....
rqos = rqos->next;
}
>
>> +int blk_mq_debugfs_register_rqos(struct rq_qos *rqos)
>> +{
>> + struct request_queue *q = rqos->q;
>> + char *dir_name = rq_qos_id_to_name(rqos->id);
>
> Please change "char *" into "const char *".
>
>> enum rq_qos_id {
>> RQ_QOS_WBT,
>> RQ_QOS_CGROUP,
>> @@ -22,6 +26,9 @@ struct rq_qos {
>> struct request_queue *q;
>> enum rq_qos_id id;
>> struct rq_qos *next;
>> +#ifdef CONFIG_BLK_DEBUG_FS
>> + struct dentry *debugfs_dir;
>> +#endif
>> };
>
> The indentation of "debugfs_dir" looks odd to me.
>
>> +static inline char *rq_qos_id_to_name(enum rq_qos_id id)
>> +{
>> + switch (id) {
>> + case RQ_QOS_WBT:
>> + return "wbt";
>> + case RQ_QOS_CGROUP:
>> + return "cgroup";
>> + }
>> + return "unknown";
>> +}
>
> Same comment here as earlier: please use "const char *" instead of "char *"
> for constant strings. Otherwise this patch looks fine to me.
Agree on the rest of your comments.
--
Jens Axboe
