Alexey -


> >>>>>> - 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.
> >>>
> >>>
> >>> Vanishingly slightly so, due to the alignment issue, which I believe is
> >> mostly theoretical, but nevertheless am ready to address now.
> >>>
> >>
> >> It doesn't do anything to enforce atomicity, does it? I.e. the following
> >> implementation would be equally "atomic":
> >>
> >> ulint
> >> os_atomic_load_ulint(ulint *ptr)
> >> {
> >>          printf("Hello, world!\n");
> >>          return(*ptr);
> >> }
> >
> >
> > Yes, it would be equally atomic (ignoring visibility and ordering) on x86_64
> as long as pointer is aligned.
> >
> 
> So... we don't need it after all?


But we don't want to ignore visibility and ordering.


> >>>> 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, ...".
> >>>
> >>>
> >>> We have gone through this with a disassembly example already, haven't we?
> >> We need os_atomic_load_ulint() because we don't want a dirty read.  We may
> >> well decide that a dirty read there is fine and then replace it.  But
> that's
> >> orthogonal to what is this primitive and why.
> >>>
> >>
> >> We need an atomic read rather than os_atomic_load_ulint() for the above
> >> reasons. An it will be atomic without using any helper functions.
> >
> >
> > OK, so is the problem that I wanted to introduce the primitives for such
> access, that would also document how is the variable accessed, and that they
> don't have to do much besides a compiler barrier on x86_64?
> >
> 
> Yes, the problem is that you introduce primitives that basically do
> nothing, and then use those primitives unnecessarily and inconsistently.
> Which in turn leads to blowing up the patch size, increased maintenance
> burden, and opens the door for wrong assumptions made when reading the
> existing code and implementing new code.


OK, now I see your concerns, and understand a big part of them but not all of them.  What wrong assumptions are encouraged by the primitives?


> >> Since
> >> I see no answer in the form "Because that will result in incorrect
> >> values being used in case ...", I assume you don't have an answer to
> >> that question.
> >
> >
> > I know that they are not resulting in incorrect values currently, and that
> the worst can happen in x86_64 with the most of possible future code changes
> is that the value loads could be moved earlier, resulting in more out-of-date
> values used.  This and the fact that accessing the variable through the
> primitive serves as self-documentation seem good enough reasons to me.
> >
> 
> Whether they are more "out-of-date" or less "out-of-date" depends on the
> definition of "date". By defining "date" as the "point in time when the
> function is called", "more out-of-date" can be easily converted to "more
> up-to-date".


I am trying to understand this but with little success.  Does this refer to the case where we use values for heuristics and stats, where a value from one point in time is as good as value from some other not too removed point in time?  I couldn't come up with another case where "date" as "point in time when the function is called" would be used.


> >>> Are you also objecting to mutex protection here? If not, why? Note that
> the
> >> three n_flush values here are completely independent.
> >>>
> >>>                mutex_enter(&buf_pool->flush_state_mutex);
> >>>
> >>>                pending_io += buf_pool->n_flush[BUF_FLUSH_LRU];
> >>>                pending_io += buf_pool->n_flush[BUF_FLUSH_SINGLE_PAGE];
> >>>                pending_io += buf_pool->n_flush[BUF_FLUSH_LIST];
> >>>
> >>>                mutex_exit(&buf_pool->flush_state_mutex);
> >>>
> >>
> >> I'm not objecting to mutex protection in that code. Why would I?
> >
> >
> > Because n_flush[] are independent aligned ulints.  If replacing
> os_atomic_load(foo) with = *foo is OK, then removing the mutex protection is
> exactly as OK, isn't it?  If not, what is the difference?
> >
> 
> But that code is correct and does what it wants to do. That's the
> difference, and that's why I have no reasons to object.


Now this I don't understand.  The variables are atomic, we don't care about "up-to-dateness" here (or do we?).  Thus 

mutex_enter(...)
pending_io += ..
mutex_exit(...)

and

pending_io += os_atomic_load_ulint(...);
...

and

pending_io += buf_pool->n_flush[...];
...


are the same?  If yes, why do we go through the mutex trouble in order to call the code correct?


> >>>> 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?
> >>>
> >>>
> >>> Upstream reads all 9 dirtily.  I replaced two of them to be clean instead.
> >> Maybe I need to replace all 9.  Maybe 0.  But that's again orthogonal.
> >>>
> >>
> >> All 9 reads are atomic. But 7 of them don't use compiler barriers
> >> because they don't need it. Neither do the remaining 2, but you are
> >> quite creative to avoid accepting this simple fact.
> >
> >
> > I am not sure where exactly you disagree with my reply here.  Yes, all 9 are
> atomic on x86_64 since they are aligned ulints.  But it's not only about
> atomicity.  Using a primitive makes the intention to get the most up-to-date
> value clear.  Perhaps there is no need for that and a stale value is OK, then
> the variables might be simply read as-is.  And I think I stated the same in my
> previous reply.  But that does not mean the primitive does not do what it's
> supposed to do, it's just we simply don't want its guarantee here and hence
> using it would be redundant.
> >
> 
> Finally, "perhaps there is no need for that"! Yes, there's no need for
> that, and that's what I've been trying to explain for a few days so far.


There is a big difference between "there is no need for that because we don't need such guarantees at this location" and "there is no need for that because there is no use for this primitive at all".

I am removing os_atomic_load_ulint() uses from buf_get_total_stat(), buf_print_instance(), buf_stat_get_pool_info(), buf_LRU_evict_from_unzip_LRU(), the last instance in buf_read_recv_pages(), srv_mon_process_existing_counter() (where it always has been a bug, as called on a private variable).

This would leave them in buf_mark_space_corrupt(), buf_page_io_complete(), buf_get_n_pending_read_ios(), buf_pool_check_no_pending_io(), buf_LRU_get_free_block(), buf_read_page_handle_error(), buf_read_ahead_random(), buf_read_ahead_linear(), buf_read_ibuf_merge_pages(), all but the last instance in buf_read_recv_pages().  Would you object?

Thanks