On 2017-06-19 22:55:37 [+0200], Jason A. Donenfeld wrote: > On Mon, Jun 19, 2017 at 9:45 AM, Sebastian Andrzej Siewior > <bigeasy@xxxxxxxxxxxxx> wrote: > > ehm. You sure? I simply delayed the lock-dropping _after_ the state > > variable was been modified. So it was basically what your patch did > > except it was unlocked later… > > Yes, I'm sure. You moved the call to invalidate_batched_entropy() to > be after the assignment of crng_init. However, the call to > invalidate_batched_entropy() must be made _before_ the assignment of > crng_init. so you need to find a another way then. Doing the assignment after dropping the lock opens another race. > >> > Are use about that? I am not sure that the gcc will inline "crng_init" > >> > read twice. It is not a local variable. READ_ONCE() is usually used > >> > where gcc could cache a memory access but you do not want this. But hey! > >> > If someone knows better I am here to learn. > >> > >> The whole purpose is that I _want_ it to cache the memory access so > >> that it is _not_ inlined. So, based on your understanding, it does > >> exactly what I intended it to do. The reason is that I'd like to avoid > >> a lock imbalance, which could happen if the read is inlined. > > > > So it was good as it was which means you can drop that READ_ONCE(). > > Except READ_ONCE ensures that the compiler will never inline it, so it > actually needs to stay. I don't think the compiler is allowed to inline it the way you describe it. This would render any assignment to local variable useless. Also the READ_ONCE creates worse code in this case (because the read can not be delayed). Sebastian
