On Fri, Jul 23, 2021 at 02:11:38PM -0400, Alan Stern wrote:

On Fri, Jul 23, 2021 at 10:30:10AM -0700, Paul E. McKenney wrote:

quoted

On Fri, Jul 23, 2021 at 12:59:47PM -0400, Alan Stern wrote:

quoted

On Fri, Jul 23, 2021 at 09:24:31AM -0700, Paul E. McKenney wrote:

quoted

On Thu, Jul 22, 2021 at 10:08:46PM -0400, Alan Stern wrote:

quoted

quoted

quoted

+	void do_something_locked(struct foo *fp)
+	{
+		bool gf = true;
+
+		/* IMPORTANT: Heuristic plus spin_lock()! */
+		if (!data_race(global_flag)) {
+			spin_lock(&fp->f_lock);
+			if (!smp_load_acquire(&global_flag)) {

quoted

quoted

quoted

+	void begin_global(void)
+	{
+		int i;
+
+		spin_lock(&global_lock);
+		WRITE_ONCE(global_flag, true);

Why does this need to be WRITE_ONCE?  It still races with the first read 
of global_flag above.

But also with the smp_load_acquire() of global_flag, right?

What I'm curious about is why, given these two races, you notate one of 
them by changing a normal write to WRITE_ONCE and you notate the other 
by changing a normal read to a data_race() read.  Why not handle them 
both the same way?

Because the code can tolerate the first read returning complete nonsense,
but needs the value from the second read to be exact at that point in
time.

In other words, if the second read races with the WRITE_ONCE, it needs to 
get either the value before the write or the value after the write; 
nothing else will do because it isn't a heuristic here.  Fair point.

quoted

 (If the value changes immediately after being read, the fact that
->f_lock is held prevents begin_global() from completing.)

This seems like something worth explaining in the document.  That 
"IMPORTANT" comment doesn't really get the full point across.

How about this comment instead?

	/* This works even if data_race() returns nonsense. */

							Thanx, Paul