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)