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
^^^^^^
Defines?
+ /**
+ * @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
^^^^
left?
+ * changing to the next CPU. A batch has the size
+ * of BLK_MQ_CPU_WORK_BATCH.
+ */
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. */
^^^^^^
Defines?
+ 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.
Bart.
