On 10/2/19 12:33 AM, Jens Axboe wrote:

On 9/30/19 1:48 PM, André Almeida wrote:

quoted

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]

quoted

@@ -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