Re: [PATCH RFC] Add "uunit" unit testing framework for CXL code

[Jim Harris <jim.harris@samsung.com>]

On Wed, Jan 03, 2024 at 10:37:09PM -0800, Dan Williams wrote:

Dan,

Thanks for the detailed review!

> Jim Harris wrote:
> > 
> > There is a balance currently between creating stubs for referenced functions,
> > versus just building and linking the referenced source files directly. For
> > example, CXL uses xarray extensively, so we just build and link the xarray
> > code directly into unit tests rather than creating stubs for xarray. See
> > KERNEL_SRC in the Makefile for which kernel source files are directly built
> > and linked in this manner.
> 
> Explain that balance a bit more, is it mainly just a "hey I want that
> function, oh building that new file is going to take me hours to work
> around a bunch of new dependencies, I'll just stub the one function I
> need."?

Yes, that's the general idea. You'll notice in the Makefile that 11 out of the
13 source files that are used directly are in lib/, since these are at the
relative bottom of the dependency chart. drivers/base/ was a bit trickier,
this original RFC builds two of those files directly, but you'll see a lot
of stubs for other drivers/base/ files. IIRC, I tried to use
drivers/base/core.c directly to avoid the 15-20 stubs, but that would have
resulted in needing 30+ stubs for all of its dependencies.

> 
> > Stubs are in uunit/stub.c, and are grouped by the kernel source file containing
> > the real definition. Those groups are listed in alphabetical order.
> > 
> > The build process has five primary steps:
> > 
> > Capture kernel build flags - use 'make -qp' to pull LINUXINCLUDE,
> > KBUILD_CPPFLAGS, KBUILD_CFLAGS from the top-level kernel Makefile.
> 
> Did you look into stealing from tools/build/? I don't think it includes
> those variables, but I suspect it includes some Makefile helpers that
> are open coded. Reusing tools/build/ likely helps with selling others on
> this uunit approach.

No, I'll check that out. Thanks for the tip.

> > 
> > Create unit test build environment - remove thunking, fentry and
> > stack-protector options from the kernel build flags. Also provide
> > KBUILD_XXX strings.
> > 
> > Modified headers - some kernel headers require modification - don't
> > modify them inline though, create a copy in the uunit/build directory,
> > modify them, and set the include directories so that the build will
> > pick up the modified copy first. Explanations for the modified headers
> > are in the Makefile.
> > 
> > Wrap and build kernel source - kernel defines many functions that are
> > also in libc. Kernel also defines some data structures with same name
> > as /usr/include data structures. So we wrap all kernel source files
> > with uunit/include/pre.h and uunit/include/post.h to do some
> > pre-processor magic to eliminate these conflicts.
> 
> How much of an ongoing maintenance burden do you expect this to be? I
> guess it all depends on how fast moving those headers are, but I worry
> about bitrot unless we can get this into some automated environment that
> notices that new changes heading into linux-next are breaking the uunit
> build.

I expect very little if any maintenance burden for the pre.h and post.h
helpers. These handle conflicts for well-established function and type names.
It's certainly possible this list will grow somewhat if this framework scales
outside of just CXL, but generally should be very stable.

The modified headers do carry some maintenance burden however, especially
autoconf.h. I've only tested with a limited number of kernel configurations
and architectures (x86_64, arm64) so the list of config options we need to
disable could grow as this gets exposure to more kernel configs.
 
> > Build unit test applications - found in uunit/app directory, and named
> 
> NIT is "app" idiomatic for CUnit? I would otherwise just call it the
> "test" directory for where the test executables end up.

Ack.
 
> > like core_region_ut.c to associate with core/region.c. The test
> > applications include the stub.c file directly - this will enable future
> > use cases where stubs can be configured to return specific mocked values.
> > 
> > Note that this framework is not CXL-specific - it could be used to unit
> > test a vast majority of other kernel code. Proposal is to just start
> > with this framework in tools/testing/cxl to meet CXL-specific needs.
> 
> Certainly all the pre-work you have done to process kernel headers is
> generally useful work for others that want to adopt this test approach.
> 
> The scaling challenge comes when there is disagreement about
> stub-behavior across tests, right? Or would a test just override the
> default stub?

Ah, you're already getting into stage 2. As we find stubs that require
modified behavior per-test, we will have variables that tests can set to
modify that behavior.

I have an RFC v2 I'm getting ready to send out, I'll add an example of this
just to show the concept. Now that I have your extensive feedback, I'll
probably drop the RFC tag as well.
 
> > Todos:
> > 
> > 1) Integrate with tools/testing/cxl Makefile
> 
> I am ok if this stands alone. As I mentioned above reusing tools/build/
> is likely more important. tools/testing/cxl/ is building a kernel
> module, tools/build/ is there to support userpace executables built out
> of the tools/ directory.

Ack.

> > 2) Break out CXL stubs separately by source file. This will be required as
> >    we start testing more and more CXL files and need to exclude stubs that
> >    are defined in the source file under test.
> 
> You mean like in cases where one test wants one stub behavior and
> another has wants a different one.

No, it's for the case where we have both unit tests and stubs for multiple
source files. For example, if stub.c has port.c stubs in it, we will get
multiple definition failures when compiling port.c unit tests.

In my v2, I just wrap stubs with "#ifndef UUNIT_CORE_PORT_C", and then
the port.c unit tests define this before including stub.c to avoid the
multiple definitions. This seems good enough for now, but may require some
changes if this scales outside of just CXL. Maybe we just use a fully
qualified name like UUNIT_DRIVERS_CXL_CORE_PORT_C.

> 
> > 3) Build common library for building up parts of the CXL topology. For example,
> >    see everything needed to set up ports, decoders, etc. to test the
> >    region.c code. Much of this can be simplified in a way that various
> >    topologies can be built without as much boilerplate code, not only for
> >    region.c but other files as well.
> 
> Do you mean de-duplicating some of the logic in
> cxl_port_setup_targets_test() and cxl_region_setup_targets_test()?

Exactly. I'll have a lot of this in the v2.

> > 4) Improve cxl_region tests to cover error conditions and add extensive
> >    comments explaining how the topology is being built. This overlaps with #3.
> 
> Some comments on the simple tests would help too just to get folks
> ramped about what's going on.

Ack. The comments are absolutely critical and I should have added them in the
v1. They'll be in the v2.

> 
> > 
> > Signed-off-by: Jim Harris <jim.harris@samsung.com>
> > ---
> >  tools/testing/cxl/uunit/Makefile             |  141 +++++++++++
> >  tools/testing/cxl/uunit/app/core_hdm_ut.c    |   82 ++++++
> >  tools/testing/cxl/uunit/app/core_region_ut.c |  237 ++++++++++++++++++
> >  tools/testing/cxl/uunit/include/post.h       |   21 ++
> >  tools/testing/cxl/uunit/include/pre.h        |   22 ++
> >  tools/testing/cxl/uunit/stub.c               |  335 ++++++++++++++++++++++++++
> >  6 files changed, 838 insertions(+)
> >  create mode 100644 tools/testing/cxl/uunit/Makefile
> >  create mode 100644 tools/testing/cxl/uunit/app/core_hdm_ut.c
> >  create mode 100644 tools/testing/cxl/uunit/app/core_region_ut.c
> >  create mode 100644 tools/testing/cxl/uunit/include/post.h
> >  create mode 100644 tools/testing/cxl/uunit/include/pre.h
> >  create mode 100644 tools/testing/cxl/uunit/stub.c
> 
> This overall looks promising, the tools/build/ question might be the
> only consideration for merging in the near term.
> 
> Some inline comments below:
> 
> [..]
> > +
> > +	CU_ASSERT(add_hdm_decoder(&port, &decoder, targets) == 0);
> 
> Likely wants some commentary on why it is ok to pass uninitialized stack
> variables into add_hdm_decoder().

Ack.

> > +}
> > +
> > +static void
> > +cxl_settle_decoders_test(void)
> > +{
> > +	struct cxl_hdm hdm;
> > +	char _regs[0x200];
> > +	void *regs = _regs;
> > +	unsigned int msecs;
> > +
> > +	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(0)] = CXL_HDM_DECODER0_CTRL_COMMITTED;
> > +	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(1)] = CXL_HDM_DECODER0_CTRL_COMMITTED;
> > +	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(2)] = 0;
> 
> Can this be made to look more like idiomatic kernel code? E.g.:
> 
>     writel(val, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(x))
> 
> ...so that code sequences can be copied from the driver into the tests?

Ack.

> > diff --git a/tools/testing/cxl/uunit/app/core_region_ut.c b/tools/testing/cxl/uunit/app/core_region_ut.c
> > new file mode 100644
> > index 000000000000..770ec200c87f
> > --- /dev/null
> > +++ b/tools/testing/cxl/uunit/app/core_region_ut.c
> > @@ -0,0 +1,237 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "pre.h"
> > +#include "drivers/cxl/core/region.c"
> > +#include "post.h"
> > +#include "../stub.c"
> > +
> > +#include <CUnit/Basic.h>
> > +#include <stdlib.h>
> > +
> > +DECLARE_RWSEM(cxl_dpa_rwsem);
> 
> This is here because core_region_ut.c is not allowed to use stubs
> core_hdm_ut.c?

Correct. This will be fixed in v2 with the changes I referenced above.

> > +	resource.start = 0x1000000000;
> > +	resource.end =   0x2000000000;
> 
> Spotted this just because of the recent debug adventure here:
> 
> http://lore.kernel.org/r/6595c10982cf_8dc6829433@dwillia2-xfh.jf.intel.com.notmuch
> 
> ...where that resource.end should be the exclusive "end" of 0x1fffffffff

Ack.

> [..]
> > diff --git a/tools/testing/cxl/uunit/include/post.h b/tools/testing/cxl/uunit/include/post.h
> > new file mode 100644
> > index 000000000000..231f5bc15c3a
> > --- /dev/null
> > +++ b/tools/testing/cxl/uunit/include/post.h
> [..]
> > diff --git a/tools/testing/cxl/uunit/include/pre.h b/tools/testing/cxl/uunit/include/pre.h
> > new file mode 100644
> > index 000000000000..1eb88e51740f
> > --- /dev/null
> > +++ b/tools/testing/cxl/uunit/include/pre.h
> 
> These last 2 files look like a lot of hard fought meticulous magic,
> hence the question above about expected ongoing maintenance burden.

See above. But some extra comments around this wouldn't hurt - I'll add that
in v2.

> > diff --git a/tools/testing/cxl/uunit/stub.c b/tools/testing/cxl/uunit/stub.c
> > new file mode 100644
> > index 000000000000..208dab1e7d6d
> > --- /dev/null
> > +++ b/tools/testing/cxl/uunit/stub.c
> 
> It might be worthwhile to let clang-format do a once over on this and all
> the other files if only to keep the automated coding-style bots from
> tripping on these files.

The formatting is horrible. I need to clean this up and will run clang-format
before sending out the v2.

-Jim