On 5/13/21 9:02 PM, Paul E. McKenney wrote:
> On Thu, May 13, 2021 at 08:10:51AM +0200, Manfred Spraul wrote:
>> Hi Paul,
>>
>> On 5/12/21 10:17 PM, Paul E. McKenney wrote:
>> [...]
>>> 	int foo;
>>> 	DEFINE_RWLOCK(foo_rwlock);
>>>
>>> 	void update_foo(int newval)
>>> 	{
>>> 		write_lock(&foo_rwlock);
>>> 		foo = newval;
>>> 		do_something(newval);
>>> 		write_unlock(&foo_rwlock);
>>> 	}
>>>
>>> 	int read_foo(void)
>>> 	{
>>> 		int ret;
>>>
>>> 		read_lock(&foo_rwlock);
>>> 		do_something_else();
>>> 		ret = foo;
>>> 		read_unlock(&foo_rwlock);
>>> 		return ret;
>>> 	}
>>>
>>> 	int read_foo_diagnostic(void)
>>> 	{
>>> 		return data_race(foo);
>>> 	}
>> The text didn't help, the example has helped:
>>
>> It was not clear to me if I have to use data_race() both on the read and the
>> write side, or only on one side.
>>
>> Based on this example: plain C may be paired with data_race(), there is no
>> need to mark both sides.
> Actually, you just demonstrated that this example is quite misleading.
> That data_race() works only because the read is for diagnostic
> purposes.  I am queuing a commit with your Reported-by that makes
> read_foo_diagnostic() just do a pr_info(), like this:
>
> 	void read_foo_diagnostic(void)
> 	{
> 		pr_info("Current value of foo: %d\n", data_race(foo));
> 	}
>
> So thank you for that!

I would not like this change at all.
Assume you chase a rare bug, and notice an odd pr_info() output.
It will take you really long until you figure out that a data_race() 
mislead you.
Thus for a pr_info(), I would consider READ_ONCE() as the correct thing.

What about something like the attached change?

--

     Manfred