Re: [PATCH v9 17/25] block/rnbd: client: main functionality

[Bart Van Assche <bvanassche@acm.org>]

On 2020-02-21 02:47, Jack Wang wrote:
> +/**
> + * rnbd_get_cpu_qlist() - finds a list with HW queues to be rerun
> + * @sess:	Session to find a queue for
> + * @cpu:	Cpu to start the search from
> + *
> + * Description:
> + *     Each CPU has a list of HW queues, which needs to be rerun.  If a list
> + *     is not empty - it is marked with a bit.  This function finds first
> + *     set bit in a bitmap and returns corresponding CPU list.
> + */
> +static struct rnbd_cpu_qlist *
> +rnbd_get_cpu_qlist(struct rnbd_clt_session *sess, int cpu)
> +{
> +	int bit;
> +
> +	/* First half */
> +	bit = find_next_bit(sess->cpu_queues_bm, nr_cpu_ids, cpu);
> +	if (bit < nr_cpu_ids) {
> +		return per_cpu_ptr(sess->cpu_queues, bit);
> +	} else if (cpu != 0) {
> +		/* Second half */
> +		bit = find_next_bit(sess->cpu_queues_bm, cpu, 0);
> +		if (bit < cpu)
> +			return per_cpu_ptr(sess->cpu_queues, bit);
> +	}
> +
> +	return NULL;
> +}

Please make the "first half" and "second half" comments unambiguous. To
me it seems like the code under "first half" searches through the second
half of the bitmap and that the code under "second half" searches
through the first half of the bitmap.

> +	/**
> +	 * That is simple percpu variable which stores cpu indeces, which are
> +	 * incremented on each access.  We need that for the sake of fairness
> +	 * to wake up queues in a round-robin manner.
> +	 */

Please start block comments with "/*".

> +static void wait_for_rtrs_disconnection(struct rnbd_clt_session *sess)
> +	__releases(&sess_lock)
> +	__acquires(&sess_lock)
> +{
> +	DEFINE_WAIT_FUNC(wait, autoremove_wake_function);

Please use DEFINE_WAIT() instead of open-coding it.

Thanks,

Bart.