Re: [PATCH] mm: change memcg->oom_group access with atomic operations

[Roman Gushchin <roman.gushchin@linux.dev>]

> On Feb 21, 2023, at 4:38 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
> 
> On Tue, Feb 21, 2023 at 03:57:58PM -0800, Roman Gushchin wrote:
>>> On Tue, Feb 21, 2023 at 03:38:24PM -0800, Paul E. McKenney wrote:
>>> On Tue, Feb 21, 2023 at 03:13:36PM -0800, Shakeel Butt wrote:
>>>> On Tue, Feb 21, 2023 at 2:38 PM Paul E. McKenney <paulmck@kernel.org> wrote:
>>>>> 
>>>>> On Tue, Feb 21, 2023 at 02:23:31PM -0800, Roman Gushchin wrote:
>>>>>> On Tue, Feb 21, 2023 at 10:23:59AM -0800, Paul E. McKenney wrote:
>>>>>>> On Tue, Feb 21, 2023 at 08:56:59AM -0800, Shakeel Butt wrote:
>>>>>>>> +Paul & Marco
>>>>>>>> 
>>>>>>>> On Tue, Feb 21, 2023 at 5:51 AM Matthew Wilcox <willy@infradead.org> wrote:
>>>>>>>>> 
>>>>>>>>> On Mon, Feb 20, 2023 at 10:52:10PM -0800, Shakeel Butt wrote:
>>>>>>>>>> On Mon, Feb 20, 2023 at 9:17 PM Roman Gushchin <roman.gushchin@linux.dev> wrote:
>>>>>>>>>>>> On Feb 20, 2023, at 3:06 PM, Shakeel Butt <shakeelb@google.com> wrote:
>>>>>>>>>>>> 
>>>>>>>>>>>> On Mon, Feb 20, 2023 at 01:09:44PM -0800, Roman Gushchin wrote:
>>>>>>>>>>>>>> On Mon, Feb 20, 2023 at 11:16:38PM +0800, Yue Zhao wrote:
>>>>>>>>>>>>>> The knob for cgroup v2 memory controller: memory.oom.group
>>>>>>>>>>>>>> will be read and written simultaneously by user space
>>>>>>>>>>>>>> programs, thus we'd better change memcg->oom_group access
>>>>>>>>>>>>>> with atomic operations to avoid concurrency problems.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Signed-off-by: Yue Zhao <findns94@gmail.com>
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Hi Yue!
>>>>>>>>>>>>> 
>>>>>>>>>>>>> I'm curious, have any seen any real issues which your patch is solving?
>>>>>>>>>>>>> Can you, please, provide a bit more details.
>>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> IMHO such details are not needed. oom_group is being accessed
>>>>>>>>>>>> concurrently and one of them can be a write access. At least
>>>>>>>>>>>> READ_ONCE/WRITE_ONCE is needed here.
>>>>>>>>>>> 
>>>>>>>>>>> Needed for what?
>>>>>>>>>> 
>>>>>>>>>> For this particular case, documenting such an access. Though I don't
>>>>>>>>>> think there are any architectures which may tear a one byte read/write
>>>>>>>>>> and merging/refetching is not an issue for this.
>>>>>>>>> 
>>>>>>>>> Wouldn't a compiler be within its rights to implement a one byte store as:
>>>>>>>>> 
>>>>>>>>>        load-word
>>>>>>>>>        modify-byte-in-word
>>>>>>>>>        store-word
>>>>>>>>> 
>>>>>>>>> and if this is a lockless store to a word which has an adjacent byte also
>>>>>>>>> being modified by another CPU, one of those CPUs can lose its store?
>>>>>>>>> And WRITE_ONCE would prevent the compiler from implementing the store
>>>>>>>>> in that way.
>>>>>>>> 
>>>>>>>> Thanks Willy for pointing this out. If the compiler can really do this
>>>>>>>> then [READ|WRITE]_ONCE are required here. I always have big bad
>>>>>>>> compiler lwn article open in a tab. I couldn't map this transformation
>>>>>>>> to ones mentioned in that article. Do we have name of this one?
>>>>>>> 
>>>>>>> No, recent compilers are absolutely forbidden from doing this sort of
>>>>>>> thing except under very special circumstances.
>>>>>>> 
>>>>>>> Before C11, compilers could and in fact did do things like this.  This is
>>>>>>> after all a great way to keep the CPU's vector unit from getting bored.
>>>>>>> Unfortunately for those who prize optimization above all else, doing
>>>>>>> this can introduce data races, for example:
>>>>>>> 
>>>>>>>    char a;
>>>>>>>    char b;
>>>>>>>    spin_lock la;
>>>>>>>    spin_lock lb;
>>>>>>> 
>>>>>>>    void change_a(char new_a)
>>>>>>>    {
>>>>>>>            spin_lock(&la);
>>>>>>>            a = new_a;
>>>>>>>            spin_unlock(&la);
>>>>>>>    }
>>>>>>> 
>>>>>>>    void change_b(char new_b)
>>>>>>>    {
>>>>>>>            spin_lock(&lb);
>>>>>>>            b = new_b;
>>>>>>>            spin_unlock(&lb);
>>>>>>>    }
>>>>>>> 
>>>>>>> If the compiler "optimized" that "a = new_a" so as to produce a non-atomic
>>>>>>> read-modify-write sequence, it would be introducing a data race.
>>>>>>> And since C11, the compiler is absolutely forbidden from introducing
>>>>>>> data races.  So, again, no, the compiler cannot invent writes to
>>>>>>> variables.
>>>>>>> 
>>>>>>> What are those very special circumstances?
>>>>>>> 
>>>>>>> 1.  The other variables were going to be written to anyway, and
>>>>>>>    none of the writes was non-volatile and there was no ordering
>>>>>>>    directive between any of those writes.
>>>>>>> 
>>>>>>> 2.  The other variables are dead, as in there are no subsequent
>>>>>>>    reads from them anywhere in the program.  Of course in that case,
>>>>>>>    there is no need to read the prior values of those variables.
>>>>>>> 
>>>>>>> 3.  All accesses to all of the variables are visible to the compiler,
>>>>>>>    and the compiler can prove that there are no concurrent accesses
>>>>>>>    to any of them.  For example, all of the variables are on-stack
>>>>>>>    variables whose addresses are never taken.
>>>>>>> 
>>>>>>> Does that help, or am I misunderstanding the question?
>>>>>> 
>>>>>> Thank you, Paul!
>>>>>> 
>>>>>> So it seems like READ_ONCE()/WRITE_ONCE() are totally useless here.
>>>>>> Or I still miss something?
>>>>> 
>>>>> Yes, given that the compiler will already avoid inventing data-race-prone
>>>>> C-language accesses to shared variables, so if that was the only reason
>>>>> that you were using READ_ONCE() or WRITE_ONCE(), then READ_ONCE() and
>>>>> WRITE_ONCE() won't be helping you.
>>>>> 
>>>>> Or perhaps better to put it a different way...  The fact that the compiler
>>>>> is not permitted to invent data-racy reads and writes is exactly why
>>>>> you do not normally need READ_ONCE() and WRITE_ONCE() for accesses in
>>>>> lock-based critical sections.  Instead, you only need READ_ONCE() and
>>>>> WRITE_ONCE() when you have lockless accesses to the same shared variables.
>>>> 
>>>> This is lockless access to memcg->oom_group potentially from multiple
>>>> CPUs, so, READ_ONCE() and WRITE_ONCE() are needed, right?
>>> 
>>> Agreed, lockless concurrent accesses should use READ_ONCE() and WRITE_ONCE().
>>> And if either conflicting access is lockless, it is lockless.  ;-)
>> 
>> Now I'm confused, why we should use it here?
>> Writing is happening from a separate syscall (a single write from a syscall),
>> reading is happening from a oom context. The variable is boolean, it's either
>> 0 or 1. What difference READ_ONCE()/WRITE_ONCE() will make here?
>> Thanks!
> 
> In practice, not much difference other than documenting shared accesses.
> Which can be valuable.
> 
> In theory, when you do a normal C-language store, the compiler is within
> its rights to use the variable for temporary storage between the time
> of the last read from that variable and the next write to that variable.
> Back to practice, I have not heard of this happening for shared variables.
> On the other hand, compilers really do this for on-stack variables whose
> addresses are not taken, which is one of the reasons that gdb might say
> that the variable is optimized out when you try to look at its value.
> 
> So the potential is there, and if it was my code, I would therefore use
> READ_ONCE() and WRITE_ONCE().

Got it, Paul, thank you for the explanation!

It seems like the resolution is that putting READ_ONCE()/WRITE_ONCE() across knobs in mm/memcontrol.c is generally a good idea, but mostly for cosmetic reasons.

Yue, can you, please, update the patch?

Btw, what a thread! Apparently writing & reading a single boolean is not that simple… :)
Thanks for all participants!

Roman