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

["André Almeida" <andrealmeid@collabora.com>]

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