On 10/07/2024 06:44, Michael Paquier wrote:
On Tue, Jul 09, 2024 at 12:12:04PM +0300, Heikki Linnakangas wrote:
I thought about it, but no. If the generation number doesn't match, there
are a few possibilities:
1. The entry was what we were looking for, but it was concurrently detached.
Return NULL is correct in that case.
2. The entry was what we were looking for, but it was concurrently detached,
and was then immediately reattached. NULL is a fine return value in that
case too. When Run runs concurrently with Detach+Attach, you don't get any
guarantee whether the actual apparent order is "Detach, Attach, Run",
"Detach, Run, Attach", or "Run, Detach, Attach". NULL result corresponds to
the "Detach, Run, Attach" ordering.
3. The entry was not actually what we were looking for. The name comparison
falsely matched just because the slot was concurrently detached and recycled
for a different injection point. We must continue the search in that case.
I added a comment to the top of the loop to explain scenario 2. And a
comment to the "continue" to explain scnario 3, because that's a bit subtle.
Okay. I am fine with your arguments here. There is still an argument
imo about looping back at the beginning of ActiveInjectionPoints
entries if we find an entry with a matching name but the generation
does not match with the local copy for the detach-attach concurrent
case, but just moving on with the follow-up entries is also OK by me,
as well.
The new comments in InjectionPointCacheRefresh() are nice
improvements. Thanks for that.
Ok, committed this.
--
Heikki Linnakangas
Neon (https://neon.tech)
