Re: A question about QuickQuiz 5.46

On Sat, 30 Dec 2023 15:57:34 -0800, Paul E. McKenney wrote:
> On Sat, Dec 23, 2023 at 06:12:31PM +0900, Akira Yokosawa wrote:
>> Hi all,
>>
>> I have a comment on this old thread.
>>
>> On 2023/05/21 19:42, Akira Yokosawa wrote:
>>> On Sun, 21 May 2023 07:54:25 +0000, Hao Lee wrote:
>>>> On Fri, May 19, 2023 at 10:27:48PM +0900, Akira Yokosawa wrote:
>>>>> On Fri, 19 May 2023 06:28:47 +0000, Hao Lee wrote:
>>>>>> On Thu, May 18, 2023 at 10:50:51AM +0900, Akira Yokosawa wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Wed, 17 May 2023 12:01:10 +0000, Hao Lee wrote:
>>>>>>>> Hi, Paul
>>>>>>>>
>>>>>>>> I noticed that QuickQuiz 5.46 is causing confusion due to the fact that
>>>>>>>> balance_count() and flush_local_count() are both located within the
>>>>>>>> slowpath and are called sequentially. As a result, it is impossible for
>>>>>>>> them to compete for the counterandmax thread-local variable.
>>>>>>>
>>>>>>> Right.
>>>>>>> And that's exactly what is mentioned in the answer to the quiz.
>>>>>>>
>>>>>>> I guess it confused you because you didn't expect such an easy one,
>>>>>>> no ? :-)
>>>>>>
>>>>>> In fact I'm confused about the question description.
>>>>>> I believe that this quiz should focus on the concurrency of global
>>>>>> variables, rather than on the concurrency of the atomic variable
>>>>>> `counterandmax`, which is currently the main focus of the description.
>>>>>
>>>>> Hmm, sorry but I'm afraid I don't get your point ...
>>>>>
>>>>> Could you share the question you'd expect to see in place of
>>>>> Quick Quiz 5.46?
>>>>
>>>> Hi, I don't think this question should exist according to my
>>>> understanding...
>>>>
>>>> Let's approach this question from a different perspective. The answer
>>>> emphasizes that the gblcnt_mutex prevents concurrency problems. Let's
>>>> assume that neither balance_count() nor flush_local_count() is holding
>>>> the gblcnt_mutex. Would the counterandmax variable encounter concurrency
>>>> problems? I don't think so because counterandmax is an atomic variable
>>>> and is always safe, even without using any locks.
>>>
>>> I'm not sure, but you might be missing subtle wording in the quiz part
>>> of Quick Quiz 5.46 (quoted below):
>>>
>>>     Given that the atomic_set() primitive does a simple store
>>>     to the specified atomic_t, ...
>>>
>>> Here, "simple store" means a normal store which doesn't involve
>>> any atomic operations (at instruction level).
>>>
>>> atomic_read() and atomic_set() are for accessing atomic_t variables
>>> without any costly atomic operations.
>>
>> This was obvious up until commit cee469f9a7aa ("api: Switch x86 atomics
>> to gcc C11-like atomics").
>>
>> Before the change, atomic_read() and atomic_set() were defined in
>> CodeSamples/api.h as:
>>
>>     typedef struct { volatile int counter; } atomic_t;
>>
>>     #define atomic_read(v)         ((v)->counter)
>>     #define atomic_set(v, i)       (((v)->counter) = (i))
>>
>> Now, they are defined in CodeSamples/api-pthreads/api-gcc.h as:
>>
>>     typedef struct { volatile int counter; } atomic_t;
>>
>>     #define atomic_read(v) \
>>            __atomic_load_n(&(v)->counter, __ATOMIC_RELAXED)
>>     #define atomic_set(v, i) \
>>            __atomic_store_n(&(v)->counter, (i), __ATOMIC_RELAXED)
>>
>> It is the C compiler's call what instructions to use for accessing
>> atomic variables.  Therefore, what Hao said above (quoted again for
>> your convenience):
>>
>>>>                                                                 Let's
>>>> assume that neither balance_count() nor flush_local_count() is holding
>>>> the gblcnt_mutex. Would the counterandmax variable encounter concurrency
>>>> problems? I don't think so because counterandmax is an atomic variable
>>>> and is always safe, even without using any locks.
>>
>> is actually quite true.
>>
>> Now I think QQz 5.46 needs at least some rewording.
>>
>> Hao, apologies for my missing the change under CodeSamples/ at the time.
>>
>> Paul, what do you think?
> 
> Let's see...
> 
> Hao Lee initially suggested that the focus be on the global nonatomics,
> which could work.  A related question might be "Why does counterandmax
> need to be atomic?"  Another approach would be to focus on wrong settings
> for counterandmax based on concurrent invocation.  And there are a number
> of other possible confusions that might be caused by this code.
> 
> So would either of you like to propose a specific change?

I'm sending a patch set as replies.
1/2 is a trivial fix of coding style.
2/2 is an RFC updating the QQ.

diff stat looks like this:

-------
Akira Yokosawa (2):
  count_lim_atomic: Enclose complex loop with {}
  count: Update QQ 5.46

 CodeSamples/count/count_lim_atomic.c |  5 ++--
 count/count.tex                      | 36 +++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 8 deletions(-)


base-commit: 4ca36874306b39ed5bfce5a50e289f966f485334
-- 
2.34.1

> 
> 							Thanx, Paul
> 
>>         Thanks, Akira
>>
>>>
>>> Which is why Quick Quiz 5.46 has been there for more than a decade.
>>>
>>> Perhaps, you might be interested in Quick Quiz B.12 as well.
>>>  
>>> HTH,
>>>         Akira
>>>
>>>>
>>>>>
>>>>>         Thanks, Akira
>>>>>
>>>>>>
>>>>>>>
>>>>>>>         Thanks, Akira
>>>>>>>
>>>>>>>>                                                               Therefore,
>>>>>>>> I believe that this quiz may be referring to the global variables
>>>>>>>> globalcount and globalreserve.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Hao Lee
>>