Code review comment for lp:~laurynas-biveinis/percona-server/bp-split-5.6

On Fri, 13 Sep 2013 12:54:44 -0000, Laurynas Biveinis wrote:
>>> Right. os_atomic_load_ulint() has additional restrictions on its arg over
>> os_atomic_inc_ulint(). I believe that these restrictions are reasonable. It
>> is a performance bug in any case to perform misaligned atomic ops even with
>> those ops that make it technically possible. I have added ut_ad()s to catch
>> this. I can rename os_atomic_ prefix to os_atomic_aligned_ prefix too,
>> although that one looks like an overkill to me.
>>>
>>
>> The same restrictions would apply even if os_atomic_load_ulint() didn't
>> exist, right? I.e. the same restrictions would apply if we simply
>> accessed those variables without any helper functions?
>
>
> What would be the desired access semantics in that case? If "anything goes", then no restrictions would apply.
>
>
>> Let me ask you a few simple questions and this time around I demand
>> "yes/no" answers.
>>
>> - Do you agree that os_atomic_load_ulint() / os_atomic_store_ulint() do
>> not do what they promise to do?
>
>
> Yes.

OK, so we agree that naming is unfortunate.

>
>
>> - Do you agree that naming them os_ordered_load_ulint() /
>> os_ordered_store_ulint() would better reflect what they do?
>
>
> No.

What would be a better naming then?

>
>
>> - Do you agree that naming them that way also makes it obvious that
>> using them in most places is simply unnecessary (e.g. in
>> buf_get_total_stat(), buf_mark_space_corrupt(), buf_print_instance(),
>> buf_get_n_pending_read_ios(), etc.)?
>
>
> No.

OK, then 2 followup questions:

1. Why do we need os_atomic_load_ulint() in buf_get_total_stat(), for
example? Here's an example of a valid answer: "Because that will result
in incorrect values being used in case ...". And some examples of
invalid answers: "non-cache-coherent architectures, visibility, memory
model, sunspots, crop circles, global warming, ...".

2. Why do only 2 out of 9 values are being loaded with
os_atomic_load_ulint() in buf_get_total_stat()? Why don't the remaining
ones need them?