On 11/23/22 16:25, Maxime Ripard wrote:
> Some tests will need to provide their own drm_driver instead of relying
> on the dumb one in the helpers, so let's create a helper that allows to
> do so.
>
> Signed-off-by: Maxime Ripard <maxime(a)cerno.tech>
> ---
Reviewed-by: Javier Martinez Canillas <javierm(a)redhat.com>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
On 11/23/22 16:25, Maxime Ripard wrote:
> The current helper to allocate a DRM device doesn't allow for any
> subclassing by drivers, which is going to be troublesome as we work on
> getting some kunit testing on atomic modesetting code.
>
> Let's use a similar pattern to the other allocation helpers by providing
> the structure size and offset as arguments.
>
> Signed-off-by: Maxime Ripard <maxime(a)cerno.tech>
> ---
Reviewed-by: Javier Martinez Canillas <javierm(a)redhat.com>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
On 11/23/22 16:25, Maxime Ripard wrote:
> The device managed resources are freed when the device is detached, so
> it has to be bound in the first place.
>
> Let's create a fake driver that we will bind to our fake device to
> benefit from the device managed cleanups in our tests.
>
> Signed-off-by: Maxime Ripard <maxime(a)cerno.tech>
> ---
> drivers/gpu/drm/tests/drm_kunit_helpers.c | 60 +++++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
If I understood the platform core code correctly, the probe is always sync in
the case of platform drivers. Unless .probe_type = PROBE_PREFER_ASYNCHRONOUS is
set or a module is loaded using with the "async_probe=1" parameter. So I believe
the wait queue won't be needed. The only DRM driver that forces an async probe is
drivers/gpu/drm/hyperv/hyperv_drm_drv.c AFAICT.
So I would drop this patch from the set for now.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
On 11/23/22 16:25, Maxime Ripard wrote:
> The device managed resources are ran if the device has bus, which is not
> the case of a root_device.
>
> Let's use a platform_device instead.
>
> Signed-off-by: Maxime Ripard <maxime(a)cerno.tech>
> ---
> drivers/gpu/drm/tests/drm_kunit_helpers.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> index 2f974f45c5b4..651aa93a98a8 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> @@ -6,6 +6,7 @@
> #include <kunit/resource.h>
>
> #include <linux/device.h>
> +#include <linux/platform_device.h>
>
> #include "drm_kunit_helpers.h"
>
> @@ -20,12 +21,26 @@ static const struct drm_mode_config_funcs drm_mode_config_funcs = {
>
> struct device *drm_kunit_helper_alloc_device(struct kunit *test)
> {
> - return root_device_register(FAKE_DEVICE_NAME);
> + struct platform_device *pdev;
> + int ret;
> +
> + ret = platform_driver_register(&fake_platform_driver);
Maybe the platform_driver_register() is something that should be done
in the drm_client_modeset_test_init() callback ?
I would also as a part of this series rename the kunit_suite from
"drm_test_pick_cmdline_test_suite" to "drm_test_modeset_test_suite"
or something like that. Since you are already setting the .init and
.exit callbacks to allocate and release the devices. So it is more
than just a test suite for cmdline params testing.
> + KUNIT_ASSERT_EQ(test, ret, 0);
> +
> + pdev = platform_device_alloc(FAKE_DEVICE_NAME, PLATFORM_DEVID_NONE);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
> +
> + ret = platform_device_add(pdev);
> + KUNIT_ASSERT_EQ(test, ret, 0);
> +
> + return &pdev->dev;
> }
>
Probably won't do it as a part of this series to avoid making it cross
subsystem, but I think there's room to further consolidation since this
helper seems to be quite generic and something that would be useful for
other subsystems. So it would make sense for kunit to have helpers to
manage platform drivers and devices.
In fact, I see that drivers/base/test/test_async_driver_probe.c already
has similar helpers that could be exposed to the rest of the kernel.
The patch looks good to me though and any of these changes could be done
as a follow-up:
Reviewed-by: Javier Martinez Canillas <javierm(a)redhat.com>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
On 11/23/22 16:25, Maxime Ripard wrote:
> We'll need in some tests to control when the device needs to be added
> and removed, so let's split the device creation from the DRM device
> creation function.
>
> Signed-off-by: Maxime Ripard <maxime(a)cerno.tech>
> ---
[...]
> -static int dev_init(struct kunit_resource *res, void *ptr)
> +struct device *drm_kunit_helper_alloc_device(struct kunit *test)
> {
> - char *name = ptr;
> - struct device *dev;
> -
> - dev = root_device_register(name);
> - if (IS_ERR(dev))
> - return PTR_ERR(dev);
> -
> - res->data = dev;
> - return 0;
> + return root_device_register(FAKE_DEVICE_NAME);
I was going to ask if a platform device could be used here, to prevent the
issue you mentioned the other day of root_device not having a bus set. But
I see now that you are doing that change in the next patch.
Reviewed-by: Javier Martinez Canillas <javierm(a)redhat.com>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
On 11/23/22 16:25, Maxime Ripard wrote:
> The name doesn't really fit the conventions for the other helpers in
> DRM/KMS, so let's rename it to make it obvious that we allocate a new
> DRM device.
>
> Signed-off-by: Maxime Ripard <maxime(a)cerno.tech>
> ---
Reviewed-by: Javier Martinez Canillas <javierm(a)redhat.com>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
KUnit does a few expensive things when enabled. This hasn't been a
problem because KUnit was only enabled on test kernels, but with a few
people enabling (but not _using_) KUnit on production systems, we need a
runtime way of handling this.
Provide a 'kunit_running' static key (defaulting to false), which allows
us to hide any KUnit code behind a static branch. This should reduce the
performance impact (on other code) of having KUnit enabled to a single
NOP when no tests are running.
Note that, while it looks unintuitive, tests always run entirely within
__kunit_test_suites_init(), so it's safe to decrement the static key at
the end of this function, rather than in __kunit_test_suites_exit(),
which is only there to clean up results in debugfs.
Signed-off-by: David Gow <davidgow(a)google.com>
---
This should be a no-op (other than a possible performance improvement)
functionality-wise, and lays the groundwork for a more optimised static
stub implementation.
The remaining patches in the series add a kunit_get_current_test()
function which is a more friendly and performant wrapper around
current->kunit_test, and use this in the slub test. They also improve
the documentation a bit.
If there are no objections, we'll take the whole series via the KUnit
tree.
No changes since v2:
https://lore.kernel.org/all/20221025071907.1251820-1-davidgow@google.com/
Changes since v1:
https://lore.kernel.org/linux-kselftest/20221021072854.333010-1-davidgow@go…
- No changes in this patch.
- Patch 2/3 is reworked, patch 3/3 is new.
---
include/kunit/test.h | 4 ++++
lib/kunit/test.c | 6 ++++++
2 files changed, 10 insertions(+)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index d7f60e8aab30..b948c32a7b6b 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -16,6 +16,7 @@
#include <linux/container_of.h>
#include <linux/err.h>
#include <linux/init.h>
+#include <linux/jump_label.h>
#include <linux/kconfig.h>
#include <linux/kref.h>
#include <linux/list.h>
@@ -27,6 +28,9 @@
#include <asm/rwonce.h>
+/* Static key: true if any KUnit tests are currently running */
+extern struct static_key_false kunit_running;
+
struct kunit;
/* Size of log associated with test. */
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 90640a43cf62..314717b63080 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -20,6 +20,8 @@
#include "string-stream.h"
#include "try-catch-impl.h"
+DEFINE_STATIC_KEY_FALSE(kunit_running);
+
#if IS_BUILTIN(CONFIG_KUNIT)
/*
* Fail the current test and print an error message to the log.
@@ -612,10 +614,14 @@ int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_
return 0;
}
+ static_branch_inc(&kunit_running);
+
for (i = 0; i < num_suites; i++) {
kunit_init_suite(suites[i]);
kunit_run_tests(suites[i]);
}
+
+ static_branch_dec(&kunit_running);
return 0;
}
EXPORT_SYMBOL_GPL(__kunit_test_suites_init);
--
2.38.1.584.g0f3c55d4c2-goog
DAMOS let users do system operations in a data access pattern oriented
way. The data access pattern, which is extracted by DAMON, is somewhat
accurate more than what user space could know in many cases. However,
in some situation, users could know something more than the kernel about
the pattern or some special requirements for some types of memory or
processes. For example, some users would have slow swap devices and
knows latency-ciritical processes and therefore want to use DAMON-based
proactive reclamation (DAMON_RECLAIM) for only non-anonymous pages of
non-latency-critical processes.
For such restriction, users could exclude the memory regions from the
initial monitoring regions and use non-dynamic monitoring regions update
monitoring operations set including fvaddr and paddr. They could also
adjust the DAMOS target access pattern. For dynamically changing memory
layout and access pattern, those would be not enough.
To help the case, add an interface, namely DAMOS filters, which can be
used to avoid the DAMOS actions be applied to specific types of memory,
to DAMON kernel API (damon.h). At the moment, it supports filtering
anonymous pages and/or specific memory cgroups in or out for each DAMOS
scheme.
This patchset adds the support for all DAMOS actions that 'paddr'
monitoring operations set supports ('pageout', 'lru_prio', and
'lru_deprio'), and the functionality is exposed via DAMON kernel API
(damon.h) the DAMON sysfs interface (/sys/kernel/mm/damon/admins/), and
DAMON_RECLAIM module parameters.
Patches Sequence
----------------
First patch implements DAMOS filter interface to DAMON kernel API.
Second patch makes the physical address space monitoring operations set
to support the filters from all supporting DAMOS actions. Third patch
adds anonymous pages filter support to DAMON_RECLAIM, and the fourth
patch documents the DAMON_RECLAIM's new feature. Fifth to seventh
patches implement DAMON sysfs files for support of the filters, and
eighth patch connects the file to use DAMOS filters feature. Ninth
patch adds simple self test cases for DAMOS filters of the sysfs
interface. Finally, following two patches (tenth and eleventh) document
the new features and interfaces.
SeongJae Park (11):
mm/damon/core: implement damos filter
mm/damon/paddr: support DAMOS filters
mm/damon/reclaim: add a parameter called skip_anon for avoiding
anonymous pages reclamation
Docs/admin-guide/damon/reclaim: document 'skip_anon' parameter
mm/damon/sysfs-schemes: implement filters directory
mm/damon/sysfs-schemes: implement filter directory
mm/damon/sysfs-schemes: connect filter directory and filters directory
mm/damon/sysfs-schemes: implement scheme filters
selftests/damon/sysfs: test filters directory
Docs/admin-guide/mm/damon/usage: document DAMOS filters of sysfs
Docs/ABI/damon: document scheme filters files
.../ABI/testing/sysfs-kernel-mm-damon | 29 ++
.../admin-guide/mm/damon/reclaim.rst | 9 +
Documentation/admin-guide/mm/damon/usage.rst | 48 ++-
include/linux/damon.h | 51 +++
mm/damon/core.c | 39 ++
mm/damon/paddr.c | 71 +++-
mm/damon/reclaim.c | 19 +
mm/damon/sysfs-schemes.c | 365 +++++++++++++++++-
tools/testing/selftests/damon/sysfs.sh | 29 ++
9 files changed, 647 insertions(+), 13 deletions(-)
--
2.25.1
On Wed, Nov 23, 2022 at 11:28 PM Maxime Ripard <maxime(a)cerno.tech> wrote:
>
> Hi,
>
> This series introduce Kunit tests to the vc4 KMS driver, but unlike what we
> have been doing so far in KMS, it actually tests the atomic modesetting code.
>
> In order to do so, I've had to improve a fair bit on the Kunit helpers already
> found in the tree in order to register a full blown and somewhat functional KMS
> driver.
>
> It's of course relying on a mock so that we can test it anywhere. The mocking
> approach created a number of issues, the main one being that we need to create
> a decent mock in the first place, see patch 22. The basic idea is that I
> created some structures to provide a decent approximation of the actual
> hardware, and that would support both major architectures supported by vc4.
>
> This is of course meant to evolve over time and support more tests, but I've
> focused on testing the HVS FIFO assignment code which is fairly tricky (and the
> tests have actually revealed one more bug with our current implementation). I
> used to have a userspace implementation of those tests, where I would copy and
> paste the kernel code and run the tests on a regular basis. It's was obviously
> fairly suboptimal, so it seemed like the perfect testbed for that series.
>
> Let me know what you think,
> Maxime
>
> To: David Airlie <airlied(a)gmail.com>
> To: Daniel Vetter <daniel(a)ffwll.ch>
> To: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com>
> To: Maxime Ripard <mripard(a)kernel.org>
> To: Thomas Zimmermann <tzimmermann(a)suse.de>
> Cc: Dave Stevenson <dave.stevenson(a)raspberrypi.com>
> Cc: Javier Martinez Canillas <javierm(a)redhat.com>
> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
> Cc: Maíra Canal <mairacanal(a)riseup.net>
> Cc: Brendan Higgins <brendan.higgins(a)linux.dev>
> Cc: David Gow <davidgow(a)google.com>
> Cc: linux-kselftest(a)vger.kernel.org
> Cc: kunit-dev(a)googlegroups.com
> Cc: dri-devel(a)lists.freedesktop.org
> Cc: linux-kernel(a)vger.kernel.org
> Cc: linux-media(a)vger.kernel.org
> Cc: linaro-mm-sig(a)lists.linaro.org
> Signed-off-by: Maxime Ripard <maxime(a)cerno.tech>
>
> ---
Hi Maxime,
Thanks very much for this! I'm really excited to see these sorts of
tests being written.
I was able to successfully run these under qemu with:
./tools/testing/kunit/kunit.py run --kunitconfig
drivers/gpu/drm/vc4/tests --arch arm64
--cross_compile=aarch64-linux-gnu-
(and also with clang, using --make_options LLVM=1 instead of the
--cross_compile flag)
On the other hand, they don't compile as a module:
ERROR: modpost: missing MODULE_LICENSE() in drivers/gpu/drm/vc4/tests/vc4_mock.o
ERROR: modpost: missing MODULE_LICENSE() in
drivers/gpu/drm/vc4/tests/vc4_mock_crtc.o
ERROR: modpost: missing MODULE_LICENSE() in
drivers/gpu/drm/vc4/tests/vc4_mock_output.o
ERROR: modpost: missing MODULE_LICENSE() in
drivers/gpu/drm/vc4/tests/vc4_mock_plane.o
ERROR: modpost: missing MODULE_LICENSE() in
drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.o
ERROR: modpost: missing MODULE_LICENSE() in
drivers/gpu/drm/tests/drm_managed_test.o
ERROR: modpost: "vc4_drm_driver"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "vc5_drm_driver"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "drm_kunit_helper_alloc_device"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "__drm_kunit_helper_alloc_drm_device_with_driver"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "__vc4_hvs_alloc"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "vc4_dummy_plane"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "vc4_mock_pv" [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "vc4_dummy_output"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
ERROR: modpost: "vc4_kms_load" [drivers/gpu/drm/vc4/tests/vc4_mock.ko]
undefined!
ERROR: modpost: "vc4_txp_crtc_data"
[drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
WARNING: modpost: suppressed 17 unresolved symbol warnings because
there were too many)
Most of those are just the need to export some symbols. There's some
work underway to support conditionally exporting symbols only if KUnit
is enabled, which may help:
https://lore.kernel.org/linux-kselftest/20221102175959.2921063-1-rmoar@goog…
Otherwise, I suspect the better short-term solution would just be to
require that the tests are built-in (or at least compiled into
whatever of the drm/vc4 modules makes most sense).
The only other thing which has me a little confused is the naming of
some of the functions, specifically with the __ prefix. Is it just for
internal functions (many of them aren't static, but maybe they could
use the VISIBLE_IF_KUNIT macro if that makes sense), or for versions
of functions which accept extra arguments? Not a big deal (and maybe
it's a DRM naming convention I'm ignorant of), but I couldn't quite
find a pattern on my first read through.
But on the whole, these look good from a KUnit point-of-view. It's
really to see some solid mocking and driver testing, too. There would
be ways to avoid passing the 'struct kunit' around in more places (or
to store extra data as a kunit_resource), but I think it's better
overall to pass it around like you have in this case -- it's certainly
more compatible with things which might span threads (e.g. the
workqueues).
Thanks a bunch,
-- David