On 10/1/20 12:15 PM, Alan Stern wrote:
On Wed, Sep 30, 2020 at 09:51:16PM -0700, Paul E. McKenney wrote:Hello! Al Viro posted the following query: ------------------------------------------------------------------------ <viro> fun question regarding barriers, if you have time for that <viro> V->A = V->B = 1; <viro> <viro> CPU1: <viro> to_free = NULL <viro> spin_lock(&LOCK) <viro> if (!smp_load_acquire(&V->B)) <viro> to_free = V <viro> V->A = 0 <viro> spin_unlock(&LOCK) <viro> kfree(to_free) <viro> <viro> CPU2: <viro> to_free = V; <viro> if (READ_ONCE(V->A)) { <viro> spin_lock(&LOCK) <viro> if (V->A) <viro> to_free = NULL <viro> smp_store_release(&V->B, 0); <viro> spin_unlock(&LOCK) <viro> } <viro> kfree(to_free); <viro> 1) is it guaranteed that V will be freed exactly once and that no accesses to *V will happen after freeing it? <viro> 2) do we need smp_store_release() there? I.e. will anything break if it's replaced with plain V->B = 0?Here are my answers to Al's questions: 1) It is guaranteed that V will be freed exactly once. It is not guaranteed that no accesses to *V will occur after it is freed, because the test contains a data race. CPU1's plain "V->A = 0" write races with CPU2's READ_ONCE; if the plain write were replaced with "WRITE_ONCE(V->A, 0)" then the guarantee would hold. Equally well, CPU1's smp_load_acquire could be replaced with a plain read while the plain write is replaced with smp_store_release. 2) The smp_store_release in CPU2 is not needed. Replacing it with a plain V->B = 0 will not break anything.
This was my interpretation also. I made the mistake of reading this right before trying to go to bed the other night and ended up tweeting at Paul that I'd regret it if he gave me scary dreams. Thought about it and read your write up and it is still exactly how I see it.
Jon. -- Computer Architect
