While writing an injection point test, I encountered a variant of the race condition that f4083c4 fixed. It had three sessions and this sequence of events:
s1: local-attach to POINT s2: enter InjectionPointRun(POINT), yield CPU just before injection_callback() s3: detach POINT, deleting the InjectionPointCondition record s2: wake up and run POINT as though it had been non-local On Sat, Mar 16, 2024 at 08:40:21AM +0900, Michael Paquier wrote: > On Fri, Mar 15, 2024 at 11:23:31AM +0200, Heikki Linnakangas wrote: > > Wrt. the spinlock and shared memory handling, I think this would be simpler > > if you could pass some payload in the InjectionPointAttach() call, which > > would be passed back to the callback function: > > > > In this case, the payload would be the "slot index" in shared memory. > > > > Or perhaps always allocate, say, 1024 bytes of working area for every > > attached injection point that the test module can use any way it wants. Like > > for storing extra conditions, or for the wakeup counter stuff in > > injection_wait(). A fixed size working area is a little crude, but would be > > very handy in practice. That would be one good way to solve it. (Storing a slot index has the same race condition, but it fixes the race to store a struct containing the PID.) The best alternative I see is to keep an InjectionPointCondition forever after creating it. Give it a "bool valid" field that we set on detach. I don't see a major reason to prefer one of these over the other. One puts a negligible amount of memory pressure on the main segment, but it simplifies the module code. I lean toward the "1024 bytes of working area" idea. Other ideas or opinions? Separately, injection_points_cleanup() breaks the rules by calling InjectionPointDetach() while holding a spinlock. The latter has an elog(ERROR), and reaching that elog(ERROR) leaves a stuck spinlock. I haven't given as much thought to solutions for this one. Thanks, nm
