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;
MIME-Version: 1.0
References: <20220910212804.670622-1-davidgow@google.com> <CAF-60z0Tmr_DkJumaT6O84EQoR_XEJVYaT2TTYcwGFfJETD1KA@mail.gmail.com>
In-Reply-To: <CAF-60z0Tmr_DkJumaT6O84EQoR_XEJVYaT2TTYcwGFfJETD1KA@mail.gmail.com>
From:   Daniel Latypov <dlatypov@google.com>
Date:   Mon, 3 Oct 2022 12:03:08 -0700
Message-ID: <CAGS_qxqWnu8V3Rj=v7z-M+LfNYyEciMtxUBS=5KBeAb8SC_D7A@mail.gmail.com>
Subject: Re: [RFC PATCH v2 0/2] kunit: Support redirecting function calls
To:     Joe Fradley <joefradley@google.com>
Cc:     David Gow <davidgow@google.com>,
        Brendan Higgins <brendanhiggins@google.com>,
        Kees Cook <keescook@chromium.org>,
        Shuah Khan <skhan@linuxfoundation.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Steve Muckle <smuckle@google.com>, kunit-dev@googlegroups.com,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Sep 30, 2022 at 8:59 AM Joe Fradley <joefradley@google.com> wrote:
> Regarding the implementation, could there be more granualitary in the
> activation of these stubs? Considering there's a small amount overhead
> for these stubs when CONFIG_KUNIT is enabled, a developer's environment
> could be adversely affected when they're testing a completely different
> area that doesn't require mocks.
>
> Maybe only activate these with CONFIG_KUNIT_FTRACE_STUBS and
> CONFIG_KUNIT_STATIC_STUBS respectively?

Just to clarify, for ftrace, there shouldn't be overhead unless a stub is used.
So it's not relevant for the following discussion.

But for "static stubs", KUNIT_STATIC_STUB_REDIRECT() checks for
current->kunit_test and kunit_find_resource() call to look for
potential stubs.

The current->kunit_test overhead should go away if we use a static key
[1] instead to track if KUnit is running.
David has a patch he's working on to do just that.
So the overhead for Android when tests aren't running should mostly vanish.

But there's still overhead when you *are* running tests.
As you pointed out, all KUNIT_STATIC_STUB_REDIRECT() callsites will
have overhead, even in functions you never intend to stub out.
I.e. there's an O(n) scan of the resource list for potential stubs
_for every single call_.

We could add a CONFIG_KUNIT_STATIC_STUBS option and have
KUNIT_STATIC_STUB_REDIRECT() be a no-op in that case.
But if you have it turned on, then all the overhead comes back, even
for functions you don't care to test.
And given the goal is that this feature be useful and ubiquitous
enough, that would be the steady state.

I feel like we'd need to wait to see how this feature gets used to determine
1) if we should try and reduce the overhead
2) how we go about doing so (e.g. separate list for stubs, static
keys, hash-based lookups of stubs, etc.)

If people are mostly going to use ftrace, then this might not be worth
trying to address.

[1] https://docs.kernel.org/staging/static-keys.html

Daniel