Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i8771445c:Fastmail
Date:   Fri, 14 Apr 2023 11:53:04 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     David Gow <davidgow@google.com>
Cc:     Matti Vaittinen <mazziesaccount@gmail.com>,
        Brendan Higgins <brendan.higgins@linux.dev>,
        Stephen Boyd <sboyd@kernel.org>,
        Shuah Khan <skhan@linuxfoundation.org>,
        Daniel Latypov <dlatypov@google.com>,
        Rae Moar <rmoar@google.com>,
        Benjamin Berg <benjamin@sipsolutions.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Jonathan Cameron <jic23@kernel.org>,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        kunit-dev@googlegroups.com
Subject: Re: [RFC PATCH v2 1/3] kunit: Add kunit_add_action() to defer a call
 until test exit
Message-ID: <7btnpvso6mwocb3vycxklcbq37bwdggpeuju3lp2aaye62jr7r@d27cuwze63bs>
References: <20230331080411.981038-1-davidgow@google.com>
 <20230331080411.981038-2-davidgow@google.com>
 <20230404133231.ingzo7xy7lejpqqb@houat>
 <CABVgOS=SM0NaS4aUTfv06jFWcD4K2k-dU297ssdfT0gZHQnfoQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CABVgOS=SM0NaS4aUTfv06jFWcD4K2k-dU297ssdfT0gZHQnfoQ@mail.gmail.com>
Precedence: bulk

On Wed, Apr 05, 2023 at 03:47:55PM +0800, David Gow wrote:
> On Tue, 4 Apr 2023 at 21:32, Maxime Ripard <maxime@cerno.tech> 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_actio=
n()
> > > + *
> > > + * Prevent an action deferred using kunit_add_action() from executin=
g 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_acti=
on_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?
> >
>=20
> 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) *=3D 2, respectively.
> int var =3D 0;
> tok1 =3D kunit_add_action(test, add_one, &var);
> kunit_add_action(test, double, &var);
> tok3 =3D kunit_add_action(test, add_one, &var);
>=20
> // The call:
> kunit_remove_action(test, add_one, &var);
> // is equivalent to
> kunit_remove_action_token(test, tok3);
> // and gives var =3D 1 as a final result
>=20
> // 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 =3D 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.
>=20
>=20
> 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