Re: [PATCH v2 1/1] blk-mq: fill header with kernel-doc

From: Bart Van Assche
Date: Tue Oct 08 2019 - 14:12:51 EST

On 10/7/19 5:14 PM, Andrà Almeida wrote:
struct blk_mq_hw_ctx {
struct {
+ /** @lock: Lock for accessing dispatch queue */
spinlock_t lock;

This spinlock not only protects dispatch list accesses but also modifications of the dispatch list. How about changing that comment into "protects the dispatch list"?

+ /**
+ * @dispatch: Queue of dispatched requests, waiting for
+ * workers to send them to the hardware.
+ */
struct list_head dispatch;

What is a worker? That word is not used anywhere in the block layer. How about changing that comment into "requests owned by this hardware queue"?

- unsigned long state; /* BLK_MQ_S_* flags */
+ /**
+ * @state: BLK_MQ_S_* flags. Define the state of the hw
+ /**
+ * @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.
+ */
> struct delayed_work run_work;

I'd prefer if algorithm details would be left out from structure documentation since such documentation becomes easily outdated. How about using something like the following description: "used for scheduling a hardware queue run at a later time"?

+ /**
+ * @next_cpu: Index of CPU/workqueue to be used in the next batch
+ * of workers.
+ */

The word "workers" here is confusing. How about the following description: "used by blk_mq_hctx_next_cpu() for round-robin CPU selection from @cpumask"?

+ /**
+ * @next_cpu_batch: Counter of how many works letf in the batch before
+ * changing to the next CPU. A batch has the size
+ */
int next_cpu_batch;

Again, I think this is too much detail about the actual algorithm.

- unsigned long flags; /* BLK_MQ_F_* flags */
+ /** @flags: BLK_MQ_F_* flags. Define the behaviour of the queue. */
+ unsigned long flags;
+ /** @sched_data: Data to support schedulers. */
void *sched_data;

That's a rather vague description. How about mentioning that this pointer is owned by the I/O scheduler that has been attached to a request queue and that the I/O scheduler decides how to use this pointer?

+ /** @queue: Queue of request to be dispatched. */
struct request_queue *queue;

How about "pointer to the request queue that owns this hardware context"?

+ /**
+ * @ctx_map: Bitmap for each software queue. If bit is on, there is a
+ * pending request.
+ */
struct sbitmap ctx_map;

Please add " in that software queue" at the end of the description.

+ /**
+ * @dispatch_from: Queue to be used when there is no scheduler
+ * was selected.
+ */
struct blk_mq_ctx *dispatch_from;

Does the word "queue" refer to a request queue, software queue or hardware queue? Please make that clear.

+ /**
+ * @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_queue_entry_t dispatch_wait;

That description doesn't look correct to me. I think that @dispatch_wait is used to wait if no tags are available. The comment above blk_mq_mark_tag_wait() is as follows:

* Mark us waiting for a tag. For shared tags, this involves hooking us
* into the tag wakeups. For non-shared tags, we can simply mark us
* needing a restart. For both cases, take care to check the condition
* again after marking us as waiting.

+ /** @wait_index: Index of next wait queue to be used. */
atomic_t wait_index;

To be used by what?

+ /** @tags: Request tags. */
struct blk_mq_tags *tags;
+ /** @sched_tags: Request tags for schedulers. */
struct blk_mq_tags *sched_tags;

I think @tags represents tags owned by the block driver and @sched_tags represents tags owned by the I/O scheduler. If no I/O scheduler is associated with a request queue, only a driver tag is allocated. If an I/O scheduler has been associated with a request queue, a request is assigned a tag from @sched_tags when that request is allocated. A tag from @tags is only assigned when a request is dispatched to a hardware queue. See also blk_mq_get_driver_tag().

+ /** @nr_active: Number of active tags. */
atomic_t nr_active;

Isn't this the number of active requests instead of the number of active tags? Please mention that this member is only used when a tag set is shared across request queues.

+ * struct blk_mq_queue_data - Data about a request inserted in a queue
+ *
+ * @rq: Data about the block IO request.

How about changing this into "Request pointer"?

+ * struct blk_mq_ops - list of callback functions that implements block driver
+ * behaviour.
+ */

Is this really a list?

* Driver command data is immediately after the request. So subtract request
- * size to get back to the original request, add request size to get the PDU.
+ * size to get back to the original request.
static inline struct request *blk_mq_rq_from_pdu(void *pdu)
return pdu - sizeof(struct request);

I'm not sure this change is an improvement.