Re: [PATCH v1 1/3] mm: zswap: fix global shrinker memcg iteration

[Yosry Ahmed]

On Wed, Jun 12, 2024 at 11:16 AM Takero Funaki <flintglass@xxxxxxxxx> wrote:
>
> 2024年6月12日(水) 3:26 Nhat Pham <nphamcs@xxxxxxxxx>:
>
> >
> > As I have noted in v0, I think this is unnecessary and makes it more confusing.
> >
>
> Does spin_lock() ensure that compiler optimizations do not remove
> memory access to an external variable? I think we need to use
> READ_ONCE/WRITE_ONCE for shared variable access even under a spinlock.
> For example,
> https://elixir.bootlin.com/linux/latest/source/mm/mmu_notifier.c#L234

In this example, it seems like mmu_interval_set_seq() updates
interval_sub->invalidate_seq locklessly using WRITE_ONCE(). I think
this is why READ_ONCE() is required in that particular case.

>
> isn't this a common use case of READ_ONCE?
> ```c
> bool shared_flag = false;
> spinlock_t flag_lock;
>
> void somefunc(void) {
>     for (;;) {
>         spin_lock(&flag_lock);
>         /* check external updates */
>         if (READ_ONCE(shared_flag))
>             break;
>         /* do something */
>         spin_unlock(&flag_lock);
>     }
>     spin_unlock(&flag_lock);
> }
> ```
> Without READ_ONCE, the check can be extracted from the loop by optimization.

According to Documentation/memory-barriers.txt, lock acquiring
functions are implicit memory barriers. Otherwise, the compiler would
be able to pull any memory access outside of the lock critical section
and locking wouldn't be reliable.