2023年7月21日 23:21，Joel Fernandes [off-list ref] 写道：

On 7/21/23 10:27, Alan Huang wrote:

quoted

quoted

2023年7月21日 20:54，Joel Fernandes [off-list ref] 写道：

quoted

On Jul 20, 2023, at 4:00 PM, Alan Huang [off-list ref] wrote:



quoted

2023年7月21日 03:22，Eric Dumazet [off-list ref] 写道：

quoted

On Thu, Jul 20, 2023 at 8:54 PM Alan Huang [off-list ref] wrote:

Hi,

I noticed a commit c87a124a5d5e(“net: force a reload of first item in hlist_nulls_for_each_entry_rcu”)
and a related discussion [1].

After reading the whole discussion, it seems like that ptr->field was cached by gcc even with the deprecated
ACCESS_ONCE(), so my question is:

     Is that a compiler bug? If so, has this bug been fixed today, ten years later?

     What about READ_ONCE(ptr->field)?

Make sure sparse is happy.

It caused a problem without barrier(), and the deprecated ACCESS_ONCE() didn’t help:

  https://lore.kernel.org/all/519D19DA.50400@yandex-team.ru/ (local)

So, my real question is: With READ_ONCE(ptr->field), are there still some unusual cases where gcc
decides not to reload ptr->field?

I am a bit doubtful there will be strong (any?) interest in replacing the barrier() with READ_ONCE() without any tangible reason, regardless of whether a gcc issue was fixed.

But hey, if you want to float the idea…

We already had the READ_ONCE() in rcu_deference_raw().
The barrier() here makes me think we need write code like below:

READ_ONCE(head->first);
barrier();
READ_ONCE(head->first);
With READ_ONCE (or the deprecated ACCESS_ONCE),
I don’t think a compiler should cache the value of head->first.

Right, it shouldn't need to cache. To Eric's point it might be risky to remove the barrier() and someone needs to explain that issue first (or IMO there needs to be another tangible reason like performance etc). Anyway, FWIW I wrote a simple program and I am not seeing the head->first cached with the pattern you shared above:

#include <stdlib.h>

#define READ_ONCE(x) (*(volatile typeof(x) *)&(x))
#define barrier() __asm__ __volatile__("": : :"memory")

typedef struct list_head {
   int first;
   struct list_head *next;
} list_head;

int main() {
   list_head *head = (list_head *)malloc(sizeof(list_head));
   head->first = 1;
   head->next = 0;

   READ_ONCE(head->first);
   barrier();

Thanks for your time!

However, what I'm trying to say here is that without this barrier(), GCC wouldn't cache this value either.

So, I removed the barrier() and tested, GCC didn’t cache the value of head->first.
(Only tested on x86-64 (all the possible versions of gcc that Compiler Explorer has) where the original issue occurred [1].)

Therefore, the commit message and the related discussion ten years ago is misleading.

Thanks again!

[1] https://lkml.org/lkml/2013/4/16/371

   READ_ONCE(head->first);

   free(head);
   return 0;
}

On ARM 32-bit, 64-bit and x86_64, with -Os and then another experiment with -O2 on new gcc versions.