On 10/2/19 12:33 AM, Jens Axboe wrote:
> On 9/30/19 1:48 PM, André Almeida wrote:
>> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
>> index 0bf056de5cc3..d0aab34972b7 100644
>> --- a/include/linux/blk-mq.h
>> +++ b/include/linux/blk-mq.h
>> @@ -11,12 +11,85 @@ struct blk_flush_queue;
>>
>> /**
>> * struct blk_mq_hw_ctx - State for a hardware queue facing the hardware
>> block device
>> + *
>> + * @lock: Lock for accessing dispatch queue
>> + * @dispatch: Queue of dispatched requests, waiting for workers to
>> send them
>> + * to the hardware.
>> + * @state: BLK_MQ_S_* flags. Define the state of the hw queue (active,
>> + * scheduled to restart, stopped).
>> + *
>> + * @run_work: Worker for dispatch scheduled requests to
>> hardware.
>> + * BLK_MQ_CPU_WORK_BATCH workers will be assigned to a CPU,
>> + * then the following ones will be assigned to
>> + * the next_cpu.
>> + * @cpumask: Map of available CPUs.
>> + * @next_cpu: Index of CPU/workqueue to be used in the next
>> batch
>> + * of workers.
>> + * @next_cpu_batch: Counter of how many works letf in the batch before
>> + * changing to the next CPU. A batch has the size of
>> + * BLK_MQ_CPU_WORK_BATCH.
>> + *
>> + * @flags: BLK_MQ_F_* flags. Define the behaviour of the queue.
>> + *
>> + * @sched_data: Data to support schedulers.
>> + * @queue: Queue of request to be dispatched.
>> + * @fq: Queue of requests to be flushed from memory to storage.
>> + *
>> + * @driver_data: Device driver specific queue.
>> + *
>> + * @ctx_map: Bitmap for each software queue. If bit is on, there is a
>> + * pending request.
>> + *
>> + * @dispatch_from: Queue to be used when there is no scheduler was selected.
>> + * @dispatch_busy: Number used by blk_mq_update_dispatch_busy() to decide
>> + * if the hw_queue is busy using Exponential Weighted Moving
>> + * Average algorithm.
>> + *
>> + * @type: HCTX_TYPE_* flags. Type of hardware queue.
>> + * @nr_ctx: Number of software queues.
>> + * @ctxs: Array of software queues.
>> + *
>> + * @dispatch_wait_lock: Lock for dispatch_wait queue.
>> + * @dispatch_wait: Waitqueue for dispatched requests. Request here will
>> + * be processed when
>> + * percpu_ref_is_zero(&q->q_usage_counter) evaluates true
>> + * for q as a request_queue.
>> + * @wait_index: Index of next wait queue to be used.
>> + *
>> + * @tags: Request tags.
>> + * @sched_tags: Request tags for schedulers.
>> + *
>> + * @queued: Number of queued requests.
>> + * @run: Number of dispatched requests.
>> + * @dispatched: Number of dispatch requests by queue.
>> + *
>> + * @numa_node: NUMA node index of this hardware queue.
>> + * @queue_num: Index of this hardware queue.
>> + *
>> + * @nr_active: Number of active tags.
>> + *
>> + * @cpuhp_dead: List to store request if some CPU die.
>> + * @kobj: Kernel object for sysfs.
>> + *
>> + * @poll_considered: Count times blk_poll() was called.
>> + * @poll_invoked: Count how many requests blk_poll() polled.
>> + * @poll_success: Count how many polled requests were completed.
>> + *
>> + * @debugfs_dir: debugfs directory for this hardware queue. Named
>> + * as cpu<cpu_number>.
>> + * @sched_debugfs_dir: debugfs directory for the scheduler.
>> + *
>> + * @hctx_list: List of all hardware queues.
>> + *
>> + * @srcu: Sleepable RCU. Use as lock when type of the hardware queue is
>> + * blocking (BLK_MQ_F_BLOCKING). Must be the last member - see
>> + * also blk_mq_hw_ctx_size().
>> */
>> struct blk_mq_hw_ctx {
>> struct {
>> spinlock_t lock;
>> struct list_head dispatch;
>> - unsigned long state; /* BLK_MQ_S_* flags */
>> + unsigned long state;
>> } ____cacheline_aligned_in_smp;
>>
>> struct delayed_work run_work;
>> @@ -24,7 +97,7 @@ struct blk_mq_hw_ctx {
>> int next_cpu;
>> int next_cpu_batch;
>>
>> - unsigned long flags; /* BLK_MQ_F_* flags */
>> + unsigned long flags;
>>
>> void *sched_data;
>> struct request_queue *queue;
>> @@ -72,41 +145,73 @@ struct blk_mq_hw_ctx {
>>
>> struct list_head hctx_list;
>>
>> - /* Must be the last member - see also blk_mq_hw_ctx_size(). */
>> struct srcu_struct srcu[0];
>> };
>
> I like improving how much is documented, but I absolutely don't like how
> everything is pulled into one section above the struct instead of being
> documented inline instead.
>
> I realize this probably makes it easier to make nice external
> documentation, but imho that's absolutely secondary to having the
> documentation being right there where you read the code.
Actually, there is no difference between the two approaches for
generating the external documentation[1]
>
>> @@ -142,81 +253,100 @@ typedef bool (busy_fn)(struct request_queue *);
>> typedef void (complete_fn)(struct request *);
>> typedef void (cleanup_rq_fn)(struct request *);
>>
>> -
>> +/**
>> + * struct blk_mq_ops - list of callback functions for blk-mq drivers
>> + */
>> struct blk_mq_ops {
>> - /*
>> - * Queue request
>> + /**
>> + * @queue_rq: Queue a new request from block IO.
>> */
>> queue_rq_fn *queue_rq;
>>
>> - /*
>> - * If a driver uses bd->last to judge when to submit requests to
>> - * hardware, it must define this function. In case of errors that
>> - * make us stop issuing further requests, this hook serves the
>> + /**
>> + * @commit_rqs: If a driver uses bd->last to judge when to submit
>> + * requests to hardware, it must define this function. In case of errors
>> + * that make us stop issuing further requests, this hook serves the
>> * purpose of kicking the hardware (which the last request otherwise
>> * would have done).
>> */
>> commit_rqs_fn *commit_rqs;
>
> Stuff like this is MUCH better. Why isn't all of it done like this?
>
Ok, I'll change that :)
Could you tell me the tree/branch that you had applied Bart's "[PATCH
0/8] Block layer patches for kernel v5.5"? I'll need to rebase my work
on top of that because of the patch "[PATCH 6/8] block: Document all
members of blk_mq_tag_set and bkl_mq_queue_map".
Thanks,
André
[1]
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#in-line-member-documentation-comments
