On Wed, May 19, 2021 at 08:34:04PM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote: [...] > > Let's have a look a several possible scenarios: > > > > Scenario A) batch containing two commands: dorm + wake up > > > > From preparation phase. > > > > - dorm: preparation phase sets the dormant flag (hooks are > > still registered). > > - wake up: unset the dormant flag. This needs to skip hook > > registration, because they are already registered. > > (it needs a way to check that hooks are registered). > > > > From commit phase. > > > > - dorm: dormant flag is unset, no-op. > > - wake-up: dormant flag is unset, no-op. > > > > From abort phase (reversed), it undoes preparation phase. > > > > - wake-up: set the dormant flag, unregister hooks. > > - dorm: unset the dormant flag, register hooks (not possible) > > > > Problems: Needs a function to check if hooks are present. > > abort phase needs to register hooks. > > I agree that abort and/or commit phases cannot register hooks. > > > Scenario B) batch containing two commands: wake up + dorm > > > > From preparation phase. > > > > - wake up: unset the dormant flag. This needs to register hooks. > > - dorm: preparation phase sets the dormant flag (hooks are > > still registered). > > > > From commit phase. > > > > - wake-up: dormant flag is set, unregister hooks. > > - dorm: dormant flag is set, unregister hooks (again). > > > > From abort phase (reversed), it undoes preparation phase. > > > > - dorm: unset the dormant flag, register hooks (not possible) > > - wake-up: set dormant flag, unregister hooks. > > > > Problems: commit phase needs try_unregister hook function. > > abort phase needs to unregister hooks. > > ... but that is doable in the sense that unregister can't fail. Right, but adding "unregister hooks" to the abort path breaks a different scenario. This might unregister a hook that, because of a later wake-up action, needs to stay there, because you cannot call register a hook from the abort path, it's a bit of a whac-a-mole game. > > I also tried looking at the transaction state instead of the dormant > > flags, similar problems. > > > > I also tried adding more internal flags to annotate context. I looked > > at adding fields to nft_table to count for the number of pending hook > > registration / unregistration. It's all tricky. > > Ok, too bad. > > > The patch I posted is addressing the issue by skipping hook > > registration / unregistration for dormant flags updates. > > > > What's your concern with the approach I'm proposing? > > No concern, I did not understand the problem with hook register in > abort/commit. > > I also dislike that dormat tables now retrain the hook overhead, but > I guess dormat tables are an exception and not the norm, so maybe > unfounded concern. You are right that this approach incurs in the hook evaluation penalty from the packet path. But I don't think there's a need to optimize this feature at this stage. If it turns out that this needs to be optimized, maybe it should be possible to add a core feature to disable hook while leaving in registered (ie. hook "dormant" state). So I'm just inclined to keep it simple while making sure that any possible (silly) robot-generated sequence with this toggle works fine.
