Re: [RFC PATCH v2 1/3] kunit: Add kunit_add_action() to defer a call until test exit

[Maxime Ripard]

On Wed, Apr 05, 2023 at 03:47:55PM +0800, David Gow wrote:
> On Tue, 4 Apr 2023 at 21:32, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
> >
> > Hi David,
> >
> > Looks great, thanks for sending a second version
> >
> > On Fri, Mar 31, 2023 at 04:04:09PM +0800, David Gow wrote:
> > > +/**
> > > + * kunit_remove_action_token() - Cancel a deferred action.
> > > + * @test: Test case the action is associated with.
> > > + * @cancel_token: The cancellation token returned by kunit_add_action()
> > > + *
> > > + * Prevent an action deferred using kunit_add_action() from executing when the
> > > + * test ends.
> > > + *
> > > + * You can also use the (test, function, context) triplet to remove an action
> > > + * with kunit_remove_action().
> > > + */
> > > +void kunit_remove_action_token(struct kunit *test, struct kunit_action_ctx *cancel_token);
> >
> > It's not clear to me why we still need the token. If
> > kunit_remove_action() works fine, why would we need to store the token?
> >
> > Can't we just make kunit_add_action() return an int to indicate whether
> > it failed or not, and that's it?
> >
> 
> So the distinction here is that the (function, context) pair doesn't
> uniquely identify an action, as you can add the same action multiple
> times, with other actions interleaved. A token encodes _which_ of
> these actions is being triggered/cancelled: the non-token variants
> always cancel the most recent matching function. Without the token,
> there's no way of removing an action "further down the stack".

> Take, for example, two functions, add_one() and double(), which are
> (*ctx)++ and (*ctx) *= 2, respectively.
> int var = 0;
> tok1 = kunit_add_action(test, add_one, &var);
> kunit_add_action(test, double, &var);
> tok3 = kunit_add_action(test, add_one, &var);
> 
> // The call:
> kunit_remove_action(test, add_one, &var);
> // is equivalent to
> kunit_remove_action_token(test, tok3);
> // and gives var = 1 as a final result
> 
> // If instead we want to remove the first add_one, we use:
> kunit_remove_action_token(test, tok1);
> // which cannot be done using kunit_remove_action()
> // gives var = 2 instead.

Sure, I still think we're kind of over-engineering this. request_irq,
devm_add_action and drmm_add_action all use that the irq/device, address
of the function and the context value to differentiate between
instances, and we never had the need for any token despite having
thousand of users.

Given that actions are supposed to be unrolled in the opposite order, I
think that removing the last action that match makes the most sense.

> There's also a (minor) performance benefit to using the token
> versions, as we don't need to do a (currently O(n)) search through the
> list of KUnit resources to find the matching entry. I doubt too many
> tests will defer enough to make this a problem.
> 
> 
> That being said, given no-one actually needs this behaviour yet, it's
> definitely something we could add later if it becomes necessary. I
> doubt it'd be useful for most of the normal resource management
> use-cases.

Yeah, I guess it's the best approach for now.

Thanks!
Maxime