On 2021/6/27 14:03, Michael S. Tsirkin wrote:
So if now we need this to be reliable then we also need smp_wmb before writing r->queue[consumer_head], there could be other gotchas.
Yes, This patch does not make it strictly reliable. T think I could mention that in the commit log?
OK so it's not that it makes it more reliable - this patch simply makes a possible false positive less likely while making a false negative more likely. Our assumption is that a false negative is cheaper then?
How do we know that it is?
And even if we prove the ptr_ring itself is faster now, how do we know what affects callers in a better way a false positive or a false negative?
I would rather we worked on actually making it reliable e.g. if we can guarantee no false positives, that would be a net win.
I thought deeper about the case you mentioned above, it seems for the above to happen, the consumer_head need to be rolled back to zero and incremented to the point when caller of __ptr_ring_empty() is still *not* able to see the r->queue[] which has been set to NULL in __ptr_ring_discard_one().
It seems smp_wmb() only need to be done once when consumer_head is rolled back to zero, and maybe that is enough to make sure the case you mentioned is fixed too?
And the smp_wmb() is only done once in a round of producing/ consuming, so the performance impact should be minimized?(of course we need to test it too).
Sorry I don't really understand the question here. I think I agree it's enough to do one smp_wmb between the write of r->queue and write of consumer_head to help guarantee no false positives. What other code changes are necessary I can't yet say without more a deeper code review.
Ok, thanks for the reviewing. Will add handling the case you mentioned above in V3 if there is no noticable performanc impact for handling the above case.