Re: [RFC PATCH v2 1/2] printk-rb: add a new printk ringbuffer implementation

[John Ogness]

On 2019-06-19, Andrea Parri <andrea.parri@xxxxxxxxxxxxxxxxxxxx> wrote:
>> I would appreciate it if you could point out a source file that
>> documents its memory barriers the way you would like to see these memory
>> barriers documented.
>
> IMO, you could find some inspiration by looking at the memory barriers
> comments from:
>
>   kernel/sched/core.c:try_to_wake_up()
>   include/linux/wait.h:waitqueue_active()
>   kernel/futex.c [header _and inline annotations]
>
> I'll detail a single example here, and then conclude with some general
> guidelines:
>
> ---
> [from kernel/sched/rt.c]
>
> static inline void rt_set_overload(struct rq *rq)
> {
> 	if (!rq->online)
> 		return;
>
> 	cpumask_set_cpu(rq->cpu, rq->rd->rto_mask);
> 	/*
> 	 * Make sure the mask is visible before we set
> 	 * the overload count. That is checked to determine
> 	 * if we should look at the mask. It would be a shame
> 	 * if we looked at the mask, but the mask was not
> 	 * updated yet.
> 	 *
> 	 * Matched by the barrier in pull_rt_task().
> 	 */
> 	smp_wmb();
> 	atomic_inc(&rq->rd->rto_count);
> }
>
> static void pull_rt_task(struct rq *this_rq)
> {
> 	int this_cpu = this_rq->cpu, cpu;
> 	bool resched = false;
> 	struct task_struct *p;
> 	struct rq *src_rq;
> 	int rt_overload_count = rt_overloaded(this_rq);
>
> 	if (likely(!rt_overload_count))
> 		return;
>
> 	/*
> 	 * Match the barrier from rt_set_overloaded; this guarantees that if we
> 	 * see overloaded we must also see the rto_mask bit.
> 	 */
> 	smp_rmb();
>
> 	/* If we are the only overloaded CPU do nothing */
> 	if (rt_overload_count == 1 &&
> 	    cpumask_test_cpu(this_rq->cpu, this_rq->rd->rto_mask))
> 		return;
>
> 	[...]
>
> }
> ---
>
> Notice that the comments provide the following information: for _each_
> memory barrier primitive,
>
>   1) the _memory accesses_ being ordered
>
>      the store to ->rto_mask and the store to ->rto_count for the smp_wmb()
>      the load from ->rto_count and the from ->rto_mask for the smp_rmb()
>
>   2) the _matching barrier_ (and its location)
>
>   3) an informal description of the _underlying guarantee(s)_  (c.f.,
>      "if we see overloaded we must also see the rto_mask bit").
>
> One can provide this information by embedding some snippet/pseudo-code
> in its comments as illustrated in the examples pointed out above.
>
> I'd suggest to _not be stingy with memory barriers explanations:  this
> eases/makes it possible the review itself as well as future changes or
> fixes to the implementation.

Thank you for the specific examples and explanations. I need to frame
your email and hang it next to my monitor for reference.

> FWIW (and as anticipated time ago in a private email), when I see code
> like this I tend to look elsewhere...  ;-/

Do you really mean "code" or are you just referring to "code comments"?
If you really mean code, then I'd appreciate some feedback about what
should change.

Your private email helped me a great deal. The memory barrier work in v2
is vastly superior to v1, even if it still crap in your eyes. I
appreciate you continuing to support me on this.

John Ogness