| Message ID | 20220910212804.670622-1-davidgow@google.com |
|---|---|
| Headers | show |
| Series | kunit: Support redirecting function calls | expand |
On Sat, Sep 10, 2022 at 2:28 PM David Gow <davidgow@google.com> wrote: > > When writing tests, it'd often be very useful to be able to intercept > calls to a function in the code being tested and replace it with a > test-specific stub. This has always been an obviously missing piece of > KUnit, and the solutions always involve some tradeoffs with cleanliness, > performance, or impact on non-test code. See the folowing document for > some of the challenges: > https://kunit.dev/mocking.html > > This series consists of two prototype patches which add support for this > sort of redirection to KUnit tests: > > 1: static_stub: Any function which might want to be intercepted adds a > call to a macro which checks if a test has redirected calls to it, and > calls the corresponding replacement. > > 2: ftrace_stub: Functions are intercepted using ftrace. > This doesn't require adding a new prologue to each function being > replaced, but does have more dependencies (which restricts it to a small > number of architectures, not including UML), and doesn't work well with > inline functions. > > The API for both implementations is very similar, so it should be easy > to migrate from one to the other if necessary. Both of these > implementations restrict the redirection to the test context: it is > automatically undone after the KUnit test completes, and does not affect > calls in other threads. If CONFIG_KUNIT is not enabled, there should be > no overhead in either implementation. > > Does either (or both) of these features sound useful, and is this > sort-of API the right model? (Personally, I think there's a reasonable > scope for both.) Is anything obviously missing or wrong? Do the names, > descriptions etc. make any sense? David, This will be a great addition to the KUnit framework and another tool in the toolbox for test writers. Both approaches have their merits. If all things were equal the ftrace option would be preffered in my opinion. The ability to add tests without having to touch the source of what you're testing is superior. However, all things aren't equal as you've detailed. There are a number of open items for the ftrace approach that will limit its scope of use. Given that a solid amount of test developers already develop on what they're testing, the static stub option sounds like the one to go with for now, if you had to choose one. 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? Joe > > Note that these patches are definitely still at the "prototype" level, > and things like error-handling, documentation, and testing are still > pretty sparse. There is also quite a bit of room for optimisation. > These'll all be improved for v1 if the concept seems good. > > We're going to be talking about this again at LPC, so it's worth having > another look before then if you're interested and/or will be attending: > https://lpc.events/event/16/contributions/1308/ > > Cheers, > -- David > > --- > > Changes since RFC v1: > https://lore.kernel.org/lkml/20220318021314.3225240-1-davidgow@google.com/ > - Fix some typos (thanks Daniel) > - Use typecheck_fn() to fix typechecking in some cases (thanks Brendan) > - Use ftrace_instruction_pointer_set() in place of kernel livepatch, > which seems to have disappeared: > https://lore.kernel.org/lkml/0a76550d-008d-0364-8244-4dae2981ea05@csgroup.eu/T/ > - Fix a copy-paste name error in the resource finding function. > - Rebase on top of torvalds/master, as it wasn't applying cleanly. > > Note that the Kernel Livepatch -> ftrace change seems to allow more > architectures to work, but while they compile, there still seems to be > issues. So, this will compile on (e.g.) arm64, but fails: > $ ./tools/testing/kunit/kunit.py run 'example*' --kunitconfig lib/kunit/stubs_example.kunitconfig --arch arm64 --make_options LLVM=1 > [05:00:13] # example_ftrace_stub_test: initializing > [05:00:13] # example_ftrace_stub_test: EXPECTATION FAILED at lib/kunit/kunit-example-test.c:179 > [05:00:13] Expected add_one(1) == 0, but > [05:00:13] add_one(1) == 2 > [05:00:13] not ok 6 - example_ftrace_stub_test > [05:00:13] [FAILED] example_ftrace_stub_test > > > > Daniel Latypov (1): > kunit: expose ftrace-based API for stubbing out functions during tests > > David Gow (1): > kunit: Expose 'static stub' API to redirect functions > > include/kunit/ftrace_stub.h | 84 +++++++++++++++++ > include/kunit/static_stub.h | 106 +++++++++++++++++++++ > lib/kunit/Kconfig | 11 +++ > lib/kunit/Makefile | 5 + > lib/kunit/ftrace_stub.c | 137 ++++++++++++++++++++++++++++ > lib/kunit/kunit-example-test.c | 63 +++++++++++++ > lib/kunit/static_stub.c | 125 +++++++++++++++++++++++++ > lib/kunit/stubs_example.kunitconfig | 10 ++ > 8 files changed, 541 insertions(+) > create mode 100644 include/kunit/ftrace_stub.h > create mode 100644 include/kunit/static_stub.h > create mode 100644 lib/kunit/ftrace_stub.c > create mode 100644 lib/kunit/static_stub.c > create mode 100644 lib/kunit/stubs_example.kunitconfig > > -- > 2.37.2.789.g6183377224-goog >
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