comments style: Re: [RFC PATCH v4 1/9] printk-rb: add a new printk ringbuffer implementation

From: Petr Mladek <pmladek@suse.com>
To: John Ogness <john.ogness@linutronix.de>
Cc: linux-kernel@vger.kernel.org,
	Andrea Parri <andrea.parri@amarulasolutions.com>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Brendan Higgins <brendanhiggins@google.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: comments style: Re: [RFC PATCH v4 1/9] printk-rb: add a new printk ringbuffer implementation
Date: Tue, 20 Aug 2019 10:55:54 +0200	[thread overview]
Message-ID: <20190820085554.deuejmxn4kbqnq7n@pathway.suse.cz> (raw)
In-Reply-To: <20190807222634.1723-2-john.ogness@linutronix.de>

On Thu 2019-08-08 00:32:26, John Ogness wrote:
> --- /dev/null
> +++ b/kernel/printk/dataring.c
> +/**
> + * _datablock_valid() - Check if given positions yield a valid data block.
> + *
> + * @dr:         The associated data ringbuffer.
> + *
> + * @head_lpos:  The newest data logical position.
> + *
> + * @tail_lpos:  The oldest data logical position.
> + *
> + * @begin_lpos: The beginning logical position of the data block to check.
> + *
> + * @next_lpos:  The logical position of the next adjacent data block.
> + *              This value is used to identify the end of the data block.
> + *

Please remove the empty lines between arguments description. They make
the comments too scattered.

> + * A data block is considered valid if it satisfies the two conditions:
> + *
> + * * tail_lpos <= begin_lpos < next_lpos <= head_lpos
> + * * tail_lpos is at most exactly 1 wrap behind head_lpos
> + *
> + * Return: true if the specified data block is valid.
> + */

To be sure, the empty lines between paragraphs are useful.
The following is still well readable:

/**
 * _datablock_valid() - Check if given positions yield a valid data block.
 * @dr:         The associated data ringbuffer.
 * @head_lpos:  The newest data logical position.
 * @tail_lpos:  The oldest data logical position.
 * @begin_lpos: The beginning logical position of the data block to check.
 * @next_lpos:  The logical position of the next adjacent data block.
 *              This value is used to identify the end of the data block.

 * A data block is considered valid if it satisfies the two conditions:
 *
 * * tail_lpos <= begin_lpos < next_lpos <= head_lpos
 * * tail_lpos is at most exactly 1 wrap behind head_lpos
 *
 * Return: true if the specified data block is valid.
 */

> +static unsigned long _dataring_pop(struct dataring *dr,
> +				   unsigned long tail_lpos)
> +{
> +	unsigned long new_tail_lpos;
> +	unsigned long begin_lpos;
> +	unsigned long next_lpos;
> +	struct dr_datablock *db;
> +	struct dr_desc *desc;
> +
> +	/*
> +	 * dA:
> +	 *
> +	 * @db has an address dependency on @tail_pos. Therefore @tail_lpos
> +	 * must be loaded before dB, which accesses @db.
> +	 */
> +	db = to_datablock(dr, tail_lpos);
> +
> +	/*
> +	 * dB:
> +	 *
> +	 * When a writer has completed accessing its data block, it sets the
> +	 * @id thus making the data block available for invalidation. This
> +	 * _acquire() ensures that this task sees all data ringbuffer and
> +	 * descriptor values seen by the writer as @id was set. This is
> +	 * necessary to ensure that the data block can be correctly identified
> +	 * as valid (i.e. @begin_lpos, @next_lpos, @head_lpos are at least the
> +	 * values seen by that writer, which yielded a valid data block at
> +	 * that time). It is not enough to rely on the address dependency of
> +	 * @desc to @id because @head_lpos is not depedent on @id. This pairs
> +	 * with the _release() in dataring_datablock_setid().

This human readable description is really useful.

> +	 *
> +	 * Memory barrier involvement:
> +	 *
> +	 * If dB reads from gA, then dC reads from fG.
> +	 * If dB reads from gA, then dD reads from fH.
> +	 * If dB reads from gA, then dE reads from fE.
> +	 *
> +	 * Note that if dB reads from gA, then dC cannot read from fC.
> +	 * Note that if dB reads from gA, then dD cannot read from fD.
> +	 *
> +	 * Relies on:
> +	 *
> +	 * RELEASE from fG to gA
> +	 *    matching
> +	 * ADDRESS DEP. from dB to dC
> +	 *
> +	 * RELEASE from fH to gA
> +	 *    matching
> +	 * ADDRESS DEP. from dB to dD
> +	 *
> +	 * RELEASE from fE to gA
> +	 *    matching
> +	 * ACQUIRE from dB to dE
> +	 */

But I am not sure how much this is useful. It would take ages to decrypt
all these shortcuts (signs) and translate them into something
human readable. Also it might get outdated easily.

That said, I haven't found yet if there was a system in all
the shortcuts. I mean if they can be descrypted easily
out of head. Also I am not familiar with the notation
of the dependencies.

If this is really needed then I am really scared of some barriers
that guard too many things. This one is a good example.

> +	desc = dr->getdesc(smp_load_acquire(&db->id), dr->getdesc_arg);
> +
> +	/* dD: */

It would be great if all these shortcuts (signs) are followed with
something human readable. Few words might be enough.

> +	next_lpos = READ_ONCE(desc->next_lpos);
> +
> +	if (!_datablock_valid(dr,
> +			      /* dE: */
> +			      atomic_long_read(&dr->head_lpos),
> +			      tail_lpos, begin_lpos, next_lpos)) {
> +		/* Another task has already invalidated the data block. */
> +		goto out;
> +	}
> +
> +
> +++ b/kernel/printk/numlist.c
> +bool numlist_read(struct numlist *nl, unsigned long id, unsigned long *seq,
> +		  unsigned long *next_id)
> +{
> +	struct nl_node *n;
> +
> +	n = nl->node(id, nl->node_arg);
> +	if (!n)
> +		return false;
> +
> +	if (seq) {
> +		/*
> +		 * aA:
> +		 *
> +		 * Adresss dependency on @id.
> +		 */

This is too scattered. If we really need so many shortcuts (signs)
then we should find a better style. The following looks perfectly
fine to me:

		/* aA: Adresss dependency on @id. */
> +		*seq = READ_ONCE(n->seq);
> +	}
> +
> +	if (next_id) {
> +		/*
> +		 * aB:
> +		 *
> +		 * Adresss dependency on @id.
> +		 */
> +		*next_id = READ_ONCE(n->next_id);
> +	}
> +

Best Regards,
Petr