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