Re: [PATCH v2 00/15] sysctl: Remove sentinel elements from drivers
On Mon, Oct 02, 2023 at 10:55:17AM +0200, Joel Granados via B4 Relay wrote: > Changes in v2: > - Left the dangling comma in the ctl_table arrays. > - Link to v1: > https://lore.kernel.org/r/20230928-jag-sysctl_remove_empty_elem_drivers-v1-0-e59120fca...@samsung.com Thanks! Pushed onto sysctl-next for wider testing. Luis
Re: [PATCH 2/2] modules/firmware: add a new option to denote a firmware group to choose one.
On Tue, Jul 04, 2023 at 12:50:50PM +1000, Dave Airlie wrote: > From: Dave Airlie > > This adds two tags that will go into the module info. > > The first denotes a group of firmwares, when that tag is present all > MODULE_FIRMWARE lines between the tags will be ignored by new versions of > dracut. > > The second makes an explicitly ordered group of firmwares to search for > inside a group setting. New dracut will pick the first available firmware > from this to put in the initramfs. > > Old dracut will just ignore these tags and fallback to installing all > the firmwares. > > The corresponding dracut code it at: > https://github.com/dracutdevs/dracut/pull/2309 > > Cc: Luis Chamberlain > Cc: linux-modu...@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Dave Airlie Lucas, did this end up working for you as well? Luis
Re: [PATCH 2/2] modules/firmware: add a new option to denote a firmware group to choose one.
On Thu, Jun 22, 2023 at 02:12:32PM -0700, Randy Dunlap wrote: > Is this going anywhere? It was posted about 2 months ago. Last I heard Dave was still working with Lucas on this? Luis
Re: [PATCH] modules/firmware: add a new option to denote a firmware group to choose one.
On Wed, May 24, 2023 at 03:35:41PM +1000, David Airlie wrote: > On Wed, May 24, 2023 at 3:26 PM Luis Chamberlain wrote: > > > > Hey Dave, just curious if there was going to be another follow up patch > > for this or if it was already posted. I don't see it clearly so just > > wanted to double check. > > I'm still considering the options here. > > I could leave the kernel patch as-is and add explicit sorting in > dracut for anything in the groups, but then we have to name/version > the firmware in a certain way, another option might be to emit the > group bounds and two records, one old, one new per-fw file, then have > some sort of explicit versioning by the driver over what order to load > them. Great thanks, just wanted to make sure I didn't neglect any pending patch. Luis
Re: [PATCH] modules/firmware: add a new option to denote a firmware group to choose one.
On Wed, May 03, 2023 at 01:19:31PM +1000, Dave Airlie wrote:
> >
> > >
> > >> > the GROUP until after the FIRMWARE, so this can't work, as it already
> > >> > will have included all the ones below, hence why I bracketed top and
> > >> > bottom with a group.
> > >>
> > >> well... that is something that can be adapted easily by using a 2 pass
> > >> approach, filtering out the list based on the groups.
> > >>
> > >> I agree that yours is simpler though. If we can rely on the
> > >> order produced by the compiler and we document the expectations of
> > >> MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
> > >> simpler approach.
> > >>
> > >> Luis, any thoughts here?
> > >
> > >I see the Dracut code indicates that the order says now that you should
> > >put the preferred firmware last, and that seems to match most coding
> > >conventions, ie, new firmwares likely get added last, so it's a nice
> >
> > not all. i915 for example keeps it newest first so when attempting
> > multiple firmware versions it starts from the preferred version. It
> > will be harder to convert since it also uses a x-macro to make sure the
> > MODULE_FIRMWARE() and the the platform mapping are actually using the same
> > firmware.
> >
> > >coincidence. Will this always work? I don't know. But if you like to
> >
> > short answer: it depends if your compiler is gcc *and* -O2 is used
> > Longer debug ahead. Simple test with the expansion of MODULE_FIRMWARE
> > baked in:
> >
> > $ cat /tmp/a.c
> > static const __attribute__((section("__modinfo_manual"), used,
> > aligned(1))) char foo[] = "modinfo_manual_foo";
> > static const __attribute__((section("__modinfo_manual"), used,
> > aligned(1))) char bar[] = "modinfo_manual_bar";
> > $ gcc -c -o /tmp/a.o /tmp/a.c
> > $ objcopy -O binary --only-section=__modinfo_manual /tmp/a.o
> > /tmp/modinfo_manual
> > $ strings /tmp/modinfo_manual
> > modinfo_manual_foo
> > modinfo_manual_bar
> >
> > However that doesn't match when building modules. In kmod:
> >
> > diff --git a/testsuite/module-playground/mod-simple.c
> > b/testsuite/module-playground/mod-simple.c
> > index 503e4d8..6dd5771 100644
> > --- a/testsuite/module-playground/mod-simple.c
> > +++ b/testsuite/module-playground/mod-simple.c
> > @@ -30,3 +30,9 @@ module_exit(test_module_exit);
> >
> > MODULE_AUTHOR("Lucas De Marchi ");
> > MODULE_LICENSE("GPL");
> > +
> > +
> > +static const char __UNIQUE_ID_firmware404[] __attribute__((__used__))
> > __attribute__((__section__("__modinfo_cpp")))
> > __attribute__((__aligned__(1))) = "modinfo_cpp_foo";
> > +static const char __UNIQUE_ID_firmware405[] __attribute__((__used__))
> > __attribute__((__section__("__modinfo_cpp")))
> > __attribute__((__aligned__(1))) = "modinfo_cpp_bar";
> >
> > $ make
> > $ objcopy -O binary --only-section=__modinfo_cpp
> > testsuite/module-playground/mod-simple.ko /tmp/modinfo_cpp
> > $ strings /tmp/modinfo_cpp
> > modinfo_cpp_bar
> > modinfo_cpp_foo
> >
> > It doesn't seem to be ./scripts/Makefile.modfinal neither as it's also
> > inverted in testsuite/module-playground/mod-simple.o
> >
> > After checking the options passed to gcc, here is the "culprit": -O2
> >
> > $ gcc -c -o /tmp/a.o /tmp/a.c && objcopy -O binary
> > --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings
> > /tmp/modinfo_manual
> > modinfo_manual_foo
> > modinfo_manual_bar
> > $ gcc -O2 -c -o /tmp/a.o /tmp/a.c && objcopy -O binary
> > --only-section=__modinfo_manual /tmp/a.o /tmp/modinfo_manual && strings
> > /tmp/modinfo_manual
> > modinfo_manual_bar
> > modinfo_manual_foo
> >
> > It seems anything but -O0 inverts the section.
> >
> > $ gcc --version
> > gcc (GCC) 12.2.1 20230201
> >
> > It doesn't match the behavior described in its man page though. Manually
> > specifying all the options that -O1 turns on doesn't invert it.
> >
> > $ gcc -fauto-inc-dec -fbranch-count-reg -fcombine-stack-adjustments
> > \
> > -fcompare-elim -fcprop-registers -fdce -fdefer-pop
> > -fdelayed-branch
> > -fdse -fforward-propagate -fguess-branch-probability
> > -fif-conversion \
> > -fif-conversion2 -finline-functions-called-once
> > -fipa-modref \
> > -fipa-profile -fipa-pure-const -fipa-reference
> > -fipa-reference-addressable \
> > -fmerge-constants -fmove-loop-stores -fomit-frame-pointer
> > -freorder-blocks \
> > -fshrink-wrap -fshrink-wrap-separate -fsplit-wide-types
> > -fssa-backprop \
> > -fssa-phiopt -ftree-bit-ccp -ftree-ccp -ftree-ch
> > -ftree-coalesce-vars \
> > -ftree-copy-prop -ftree-dce -ftree-dominator-opts
> > -ftree-dse -ftree-forwprop \
> > -ftree-fre -ftree-phiprop -ftree-pta -ftree-scev-cprop
> > -ftree-sink
Re: [PATCH] modules/firmware: add a new option to denote a firmware group to choose one.
On Tue, May 02, 2023 at 11:11:58AM -0700, Lucas De Marchi wrote:
> Based on the above and my previous reply, I think we should have
> something more explicit about the order rather than relying on the
> toolchain behavior.
You can open code ELF sections and provide SORT() but you can also use
helpers for all this. Long ago I provided low level ELF helpers to
provide the ability to easily sort through data / code using
linker-tables [0], to help with ELF section with explicit ordering,
perhaps this could be leveraged?
I *think* for instance, one could do, using the built-in firmware
conversion as a slightly relateed example [1], provide a firmware helper for
drivers which uses something like DECLARE_FIRMWARE_TABLE(acme_gpu_fw),
then that is declared as the ELF table for acme_gpu_fw, the firmware API
could then get the hint to use that table for iterating over with
linktable_for_each(fw, acme_gpu_fw). One would not be using the linker
table for the actual firmware but instead for the firmware odering.
The firmware loader could be extended with something like
#define DECLARE_FIRMWARE_TABLE(fw_table) DECLARE_LINKTABLE_RO(struct fw_foo,
fw_table)
struct fw_foo {
const char *opt_subfamily;
};
#define FW_NAME_ORDERED(__level, __family, __sub_family)\
static LINKTABLE_INIT_DATA(fw_foo, __level) \
__fw_ordered_##__family = { \
opt_subfamily = sub_family, \
};
Then firmware could would use
FW_NAME_ORDERED(01, acme_gpu_fw, coyote);
And helpers can use it to look for the firmware an firmare API call.
As to why linker-tables never got upstream? It promised / documented
too much, we need to just make the API conversion smooth and target
that. The ordering is a secondary win. The fact that we can simplify
init levels etc, is more futuristic and should only be documented once
we get there.
[0]
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170620-linker-tables-v8
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/commit/?h=20170620-linker-tables-v8=162698d2f1a2406c6a7a4d39f13113ca789fd2ec
Luis
Re: [PATCH] modules/firmware: add a new option to denote a firmware group to choose one.
On Mon, Apr 24, 2023 at 10:01:13AM -0700, Lucas De Marchi wrote:
> On Mon, Apr 24, 2023 at 03:44:18PM +1000, Dave Airlie wrote:
> > On Fri, 21 Apr 2023 at 05:09, Lucas De Marchi
> > wrote:
> > >
> > > On Wed, Apr 19, 2023 at 02:36:52PM +1000, Dave Airlie wrote:
> > > >From: Dave Airlie
> > > >
> > > >This adds a tag that will go into the module info, only one firmware from
> > > >the group given needs to be available for this driver to work. This
> > > >allows
> > > >dracut to avoid adding in firmware that aren't needed.
> > > >
> > > >This just brackets a module list in the modinfo, the modules in the list
> > > >will get entries in reversed order so the last module in the list is the
> > > >preferred one.
> > > >
> > > >The corresponding dracut code it at:
> > > >https://github.com/dracutdevs/dracut/pull/2309
> > >
> > > it would be good to have the example usage in the commit message here so
> > > it can be easily checked as reference for other drivers.
> >
> > Good point.
> >
> > >
> > > I don't think we ever had any ordering in modinfo being relevant for
> > > other things. Considering the use case and that we could also use a
> > > similar thing for i915 / xe modules wrt to the major version,
> > > couldn't we do something like below?
> > >
> > > MODULE_FIRMWARE_GROUP("nvidia/ga106/gsp/gsp");
> > > MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5258902.bin");
> > > MODULE_FIRMWARE("nvidia/ga106/gsp/gsp-5303002.bin");
> > >
> > > so the group is created by startswith() rather than by the order the
> > > modinfo appears in the elf section. In i915 we'd have:
> >
> > The way userspace parses these is reverse order, and it doesn't see
>
> the main issue I have with it is that it relies on a order that is
> implicit rather than intended. The order comes from how the .modinfo ELF
> section is assembled together... so the fact that your call to
> kmod_module_get_info() returns a list with the keys in the reverse order
> of the MODULE_FIRMWARE() definitions, is basically because the compiler
> toolchain did it did that way.
>
> It's worse when those sections come from different compilation units as
> the order then is not predictable and can easily break with changes to
> the build infra if the files are linked in different order.
>
> I think the grouping thing here would only be supported with firmware
> defined on the same compilation unit, but it's something to keep in mind
> and document.
I had provided a simple API to help with explicit linker order years ago:
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170620-linker-tables-v8
Other than that you have to rely on the order in the Makefile or way
in which they are declared.
> > the GROUP until after the FIRMWARE, so this can't work, as it already
> > will have included all the ones below, hence why I bracketed top and
> > bottom with a group.
>
> well... that is something that can be adapted easily by using a 2 pass
> approach, filtering out the list based on the groups.
>
> I agree that yours is simpler though. If we can rely on the
> order produced by the compiler and we document the expectations of
> MODULE_FIRMWARE_GROUP_ONLY_ONE, then I believe we can stay with the
> simpler approach.
>
> Luis, any thoughts here?
I see the Dracut code indicates that the order says now that you should
put the preferred firmware last, and that seems to match most coding
conventions, ie, new firmwares likely get added last, so it's a nice
coincidence. Will this always work? I don't know. But if you like to
hedge, then this seems fine so long as I'm folks follow up to fix issues
later. I think it should and the simplicity is preferred, worth a shot
I think.
But the examples on both sides are pretty terrible. I'd just like to ask
all this gets extended in proper kdoc form and we are able to get users
and developers to read this under "Module support" in:
https://docs.kernel.org/core-api/kernel-api.html
So go to town with a new section for:
Documentation/core-api/kernel-api.rst
Luis
Re: enhancing module info to allow grouping of firmwares
On Thu, Mar 16, 2023 at 07:18:11AM +1000, Dave Airlie wrote:
> MODULE_FIRMWARE_GROUP("g1")
> MODULE_FIRMWARE("fwv1.bin")
> MODULE_FIRMWARE("fwv2.bin")
> MODULE_FIRMWARE_GROUP_END("g2")
The way module namespaces were implemented could be used to leverage
something like this, just that you'd use it for firmware tags, not
symbols.
drivers/dma-buf/dma-buf.c:EXPORT_SYMBOL_NS_GPL(dma_buf_export, DMA_BUF);
Just that that would look like something like this, which might be
even nicer:
MODULE_FIRMWARE_GROUP("fwv1.bin", "nvidia/g1");
MODULE_FIRMWARE_GROUP("fwv2.bin", "nvidia/g1");
Luis
Re: [PATCH v4 1/4] video: fbdev: atyfb: only use ioremap_uc() on i386 and ia64
On Wed, Mar 08, 2023 at 09:07:07PM +0800, Baoquan He wrote: > From: Arnd Bergmann > > ioremap_uc() is only meaningful on old x86-32 systems with the PAT > extension, and on ia64 with its slightly unconventional ioremap() > behavior, everywhere else this is the same as ioremap() anyway. > > Change the only driver that still references ioremap_uc() to only do so > on x86-32/ia64 in order to allow removing that interface at some > point in the future for the other architectures. > > On some architectures, ioremap_uc() just returns NULL, changing > the driver to call ioremap() means that they now have a chance > of working correctly. > > Signed-off-by: Arnd Bergmann > Signed-off-by: Baoquan He > Cc: Helge Deller > Cc: Thomas Zimmermann > Cc: Christophe Leroy > Cc: linux-fb...@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org Reviewed-by: Luis Chamberlain Is anyone using this driver these days? How often do fbdev drivers get audited to see what can be nuked? Luis
Re: [PATCH 00/17] MODULE_LICENSE removals, sixth tranche
Stupid question, if you're removing MODULE_LICENSE() than why keep the other stupid MODULE_*() crap too? If its of no use, be gone! Luis
Re: [PATCH v6 0/4] Let userspace know when snd-hda-intel needs i915
On Tue, Sep 20, 2022 at 07:24:54AM +0200, Mauro Carvalho Chehab wrote: > Hi Luis, > > On Mon, 9 May 2022 13:38:28 -0700 > Luis Chamberlain wrote: > > > On Mon, May 09, 2022 at 06:23:35PM +0200, Mauro Carvalho Chehab wrote: > > > Currently, kernel/module annotates module dependencies when > > > request_symbol is used, but it doesn't cover more complex inter-driver > > > dependencies that are subsystem and/or driver-specific. > > > > > > > At this pount v5.18-rc7 is out and so it is too late to soak this > > in for the proper level of testing I'd like to see for modules-next. > > So I can review this after the next merge window. I'd want to beat > > the hell out of this and if possible I'd like to see if we can have > > some test coverage for the intended goal and how to break it. > > Any news with regards to this patch series? 0-day had a rant about a bug with it, it would be wonderful if you can fix that bug and rebase. Yet again we're now on v6.0-rc7 but it doesn't mean we can't start testing all this on linux-next. I can just get this merged to linux-next as soon as this is ready for a new spin, but we certainly will have to wait until 6.2 as we haven't yet gotten proper coverage for this on v6.1. Is there any testing situations you can think of using which can demo this a bit more separately from existing drivers, perhaps a new selftests or something? Luis
Re: [PATCH v6 0/4] Let userspace know when snd-hda-intel needs i915
On Mon, May 09, 2022 at 06:23:35PM +0200, Mauro Carvalho Chehab wrote: > Currently, kernel/module annotates module dependencies when > request_symbol is used, but it doesn't cover more complex inter-driver > dependencies that are subsystem and/or driver-specific. > At this pount v5.18-rc7 is out and so it is too late to soak this in for the proper level of testing I'd like to see for modules-next. So I can review this after the next merge window. I'd want to beat the hell out of this and if possible I'd like to see if we can have some test coverage for the intended goal and how to break it. Luis
Re: [PATCH v2 6/8] inotify: simplify subdirectory registration with register_sysctl()
On Wed, Nov 24, 2021 at 10:44:09AM +0100, Jan Kara wrote:
> On Tue 23-11-21 12:24:20, Luis Chamberlain wrote:
> > From: Xiaoming Ni
> >
> > There is no need to user boiler plate code to specify a set of base
> > directories we're going to stuff sysctls under. Simplify this by using
> > register_sysctl() and specifying the directory path directly.
> >
> > Move inotify_user sysctl to inotify_user.c while at it to remove clutter
> > from kernel/sysctl.c.
> >
> > Signed-off-by: Xiaoming Ni
> > [mcgrof: update commit log to reflect new path we decided to take]
> > Signed-off-by: Luis Chamberlain
>
> This looks fishy. You register inotify_table but not fanotify_table and
> remove both...
Indeed, the following was missing, I'll roll it in:
diff --git a/fs/notify/fanotify/fanotify_user.c
b/fs/notify/fanotify/fanotify_user.c
index 559bc1e9926d..a35693eb1f36 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -59,7 +59,7 @@ static int fanotify_max_queued_events __read_mostly;
static long ft_zero = 0;
static long ft_int_max = INT_MAX;
-struct ctl_table fanotify_table[] = {
+static struct ctl_table fanotify_table[] = {
{
.procname = "max_user_groups",
.data = _user_ns.ucount_max[UCOUNT_FANOTIFY_GROUPS],
@@ -88,6 +88,13 @@ struct ctl_table fanotify_table[] = {
},
{ }
};
+
+static void __init fanotify_sysctls_init(void)
+{
+ register_sysctl("fs/fanotify", fanotify_table);
+}
+#else
+#define fanotify_sysctls_init() do { } while (0)
#endif /* CONFIG_SYSCTL */
/*
@@ -1685,6 +1692,7 @@ static int __init fanotify_user_setup(void)
init_user_ns.ucount_max[UCOUNT_FANOTIFY_GROUPS] =
FANOTIFY_DEFAULT_MAX_GROUPS;
init_user_ns.ucount_max[UCOUNT_FANOTIFY_MARKS] = max_marks;
+ fanotify_sysctls_init();
return 0;
}
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index 616af2ea20f3..556cc63c88ee 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -5,8 +5,6 @@
#include
#include
-extern struct ctl_table fanotify_table[]; /* for sysctl */
-
#define FAN_GROUP_FLAG(group, flag) \
((group)->fanotify_data.flags & (flag))
[PATCH v2 3/8] macintosh/mac_hid.c: simplify subdirectory registration with register_sysctl()
There is no need to user boiler plate code to specify a set of base
directories we're going to stuff sysctls under. Simplify this by using
register_sysctl() and specifying the directory path directly.
// pycocci sysctl-subdir-register-sysctl-simplify.cocci PATH
@c1@
expression E1;
identifier subdir, sysctls;
@@
static struct ctl_table subdir[] = {
{
.procname = E1,
.maxlen = 0,
.mode = 0555,
.child = sysctls,
},
{ }
};
@c2@
identifier c1.subdir;
expression E2;
identifier base;
@@
static struct ctl_table base[] = {
{
.procname = E2,
.maxlen = 0,
.mode = 0555,
.child = subdir,
},
{ }
};
@c3@
identifier c2.base;
identifier header;
@@
header = register_sysctl_table(base);
@r1 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.subdir, c1.sysctls;
@@
-static struct ctl_table subdir[] = {
- {
- .procname = E1,
- .maxlen = 0,
- .mode = 0555,
- .child = sysctls,
- },
- { }
-};
@r2 depends on c1 && c2 && c3@
identifier c1.subdir;
expression c2.E2;
identifier c2.base;
@@
-static struct ctl_table base[] = {
- {
- .procname = E2,
- .maxlen = 0,
- .mode = 0555,
- .child = subdir,
- },
- { }
-};
@initialize:python@
@@
def make_my_fresh_expression(s1, s2):
return '"' + s1.strip('"') + "/" + s2.strip('"') + '"'
@r3 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.sysctls;
expression c2.E2;
identifier c2.base;
identifier c3.header;
fresh identifier E3 = script:python(E2, E1) { make_my_fresh_expression(E2, E1)
};
@@
header =
-register_sysctl_table(base);
+register_sysctl(E3, sysctls);
Generated-by: Coccinelle SmPL
Signed-off-by: Luis Chamberlain
---
drivers/macintosh/mac_hid.c | 24 +---
1 file changed, 1 insertion(+), 23 deletions(-)
diff --git a/drivers/macintosh/mac_hid.c b/drivers/macintosh/mac_hid.c
index 28b8581b44dd..d8c4d5664145 100644
--- a/drivers/macintosh/mac_hid.c
+++ b/drivers/macintosh/mac_hid.c
@@ -239,33 +239,11 @@ static struct ctl_table mac_hid_files[] = {
{ }
};
-/* dir in /proc/sys/dev */
-static struct ctl_table mac_hid_dir[] = {
- {
- .procname = "mac_hid",
- .maxlen = 0,
- .mode = 0555,
- .child = mac_hid_files,
- },
- { }
-};
-
-/* /proc/sys/dev itself, in case that is not there yet */
-static struct ctl_table mac_hid_root_dir[] = {
- {
- .procname = "dev",
- .maxlen = 0,
- .mode = 0555,
- .child = mac_hid_dir,
- },
- { }
-};
-
static struct ctl_table_header *mac_hid_sysctl_header;
static int __init mac_hid_init(void)
{
- mac_hid_sysctl_header = register_sysctl_table(mac_hid_root_dir);
+ mac_hid_sysctl_header = register_sysctl("dev/mac_hid", mac_hid_files);
if (!mac_hid_sysctl_header)
return -ENOMEM;
--
2.33.0
[PATCH v2 6/8] inotify: simplify subdirectory registration with register_sysctl()
From: Xiaoming Ni
There is no need to user boiler plate code to specify a set of base
directories we're going to stuff sysctls under. Simplify this by using
register_sysctl() and specifying the directory path directly.
Move inotify_user sysctl to inotify_user.c while at it to remove clutter
from kernel/sysctl.c.
Signed-off-by: Xiaoming Ni
[mcgrof: update commit log to reflect new path we decided to take]
Signed-off-by: Luis Chamberlain
---
fs/notify/inotify/inotify_user.c | 11 ++-
include/linux/inotify.h | 3 ---
kernel/sysctl.c | 21 -
3 files changed, 10 insertions(+), 25 deletions(-)
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 29fca3284bb5..54583f62dc44 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -58,7 +58,7 @@ struct kmem_cache *inotify_inode_mark_cachep __read_mostly;
static long it_zero = 0;
static long it_int_max = INT_MAX;
-struct ctl_table inotify_table[] = {
+static struct ctl_table inotify_table[] = {
{
.procname = "max_user_instances",
.data =
_user_ns.ucount_max[UCOUNT_INOTIFY_INSTANCES],
@@ -87,6 +87,14 @@ struct ctl_table inotify_table[] = {
},
{ }
};
+
+static void __init inotify_sysctls_init(void)
+{
+ register_sysctl("fs/inotify", inotify_table);
+}
+
+#else
+#define inotify_sysctls_init() do { } while (0)
#endif /* CONFIG_SYSCTL */
static inline __u32 inotify_arg_to_mask(struct inode *inode, u32 arg)
@@ -849,6 +857,7 @@ static int __init inotify_user_setup(void)
inotify_max_queued_events = 16384;
init_user_ns.ucount_max[UCOUNT_INOTIFY_INSTANCES] = 128;
init_user_ns.ucount_max[UCOUNT_INOTIFY_WATCHES] = watches_max;
+ inotify_sysctls_init();
return 0;
}
diff --git a/include/linux/inotify.h b/include/linux/inotify.h
index 6a24905f6e1e..8d20caa1b268 100644
--- a/include/linux/inotify.h
+++ b/include/linux/inotify.h
@@ -7,11 +7,8 @@
#ifndef _LINUX_INOTIFY_H
#define _LINUX_INOTIFY_H
-#include
#include
-extern struct ctl_table inotify_table[]; /* for sysctl */
-
#define ALL_INOTIFY_BITS (IN_ACCESS | IN_MODIFY | IN_ATTRIB | IN_CLOSE_WRITE |
\
IN_CLOSE_NOWRITE | IN_OPEN | IN_MOVED_FROM | \
IN_MOVED_TO | IN_CREATE | IN_DELETE | \
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 7a90a12b9ea4..6aa67c737e4e 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -125,13 +125,6 @@ static const int maxolduid = 65535;
static const int ngroups_max = NGROUPS_MAX;
static const int cap_last_cap = CAP_LAST_CAP;
-#ifdef CONFIG_INOTIFY_USER
-#include
-#endif
-#ifdef CONFIG_FANOTIFY
-#include
-#endif
-
#ifdef CONFIG_PROC_SYSCTL
/**
@@ -3099,20 +3092,6 @@ static struct ctl_table fs_table[] = {
.proc_handler = proc_dointvec,
},
#endif
-#ifdef CONFIG_INOTIFY_USER
- {
- .procname = "inotify",
- .mode = 0555,
- .child = inotify_table,
- },
-#endif
-#ifdef CONFIG_FANOTIFY
- {
- .procname = "fanotify",
- .mode = 0555,
- .child = fanotify_table,
- },
-#endif
#ifdef CONFIG_EPOLL
{
.procname = "epoll",
--
2.33.0
[PATCH v2 1/8] hpet: simplify subdirectory registration with register_sysctl()
There is no need to user boiler plate code to specify a set of base
directories we're going to stuff sysctls under. Simplify this by using
register_sysctl() and specifying the directory path directly.
// pycocci sysctl-subdir-register-sysctl-simplify.cocci drivers/char/hpet.c
@c1@
expression E1;
identifier subdir, sysctls;
@@
static struct ctl_table subdir[] = {
{
.procname = E1,
.maxlen = 0,
.mode = 0555,
.child = sysctls,
},
{ }
};
@c2@
identifier c1.subdir;
expression E2;
identifier base;
@@
static struct ctl_table base[] = {
{
.procname = E2,
.maxlen = 0,
.mode = 0555,
.child = subdir,
},
{ }
};
@c3@
identifier c2.base;
identifier header;
@@
header = register_sysctl_table(base);
@r1 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.subdir, c1.sysctls;
@@
-static struct ctl_table subdir[] = {
- {
- .procname = E1,
- .maxlen = 0,
- .mode = 0555,
- .child = sysctls,
- },
- { }
-};
@r2 depends on c1 && c2 && c3@
identifier c1.subdir;
expression c2.E2;
identifier c2.base;
@@
-static struct ctl_table base[] = {
- {
- .procname = E2,
- .maxlen = 0,
- .mode = 0555,
- .child = subdir,
- },
- { }
-};
@initialize:python@
@@
def make_my_fresh_expression(s1, s2):
return '"' + s1.strip('"') + "/" + s2.strip('"') + '"'
@r3 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.sysctls;
expression c2.E2;
identifier c2.base;
identifier c3.header;
fresh identifier E3 = script:python(E2, E1) { make_my_fresh_expression(E2, E1)
};
@@
header =
-register_sysctl_table(base);
+register_sysctl(E3, sysctls);
Generated-by: Coccinelle SmPL
Signed-off-by: Luis Chamberlain
---
drivers/char/hpet.c | 22 +-
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index 4e5431f01450..563dfae3b8da 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -746,26 +746,6 @@ static struct ctl_table hpet_table[] = {
{}
};
-static struct ctl_table hpet_root[] = {
- {
-.procname = "hpet",
-.maxlen = 0,
-.mode = 0555,
-.child = hpet_table,
-},
- {}
-};
-
-static struct ctl_table dev_root[] = {
- {
-.procname = "dev",
-.maxlen = 0,
-.mode = 0555,
-.child = hpet_root,
-},
- {}
-};
-
static struct ctl_table_header *sysctl_header;
/*
@@ -1061,7 +1041,7 @@ static int __init hpet_init(void)
if (result < 0)
return -ENODEV;
- sysctl_header = register_sysctl_table(dev_root);
+ sysctl_header = register_sysctl("dev/hpet", hpet_table);
result = acpi_bus_register_driver(_acpi_driver);
if (result < 0) {
--
2.33.0
[PATCH v2 7/8] cdrom: simplify subdirectory registration with register_sysctl()
There is no need to user boiler plate code to specify a set of base
directories we're going to stuff sysctls under. Simplify this by using
register_sysctl() and specifying the directory path directly.
// pycocci sysctl-subdir-register-sysctl-simplify.cocci PATH
@c1@
expression E1;
identifier subdir, sysctls;
@@
static struct ctl_table subdir[] = {
{
.procname = E1,
.maxlen = 0,
.mode = 0555,
.child = sysctls,
},
{ }
};
@c2@
identifier c1.subdir;
expression E2;
identifier base;
@@
static struct ctl_table base[] = {
{
.procname = E2,
.maxlen = 0,
.mode = 0555,
.child = subdir,
},
{ }
};
@c3@
identifier c2.base;
identifier header;
@@
header = register_sysctl_table(base);
@r1 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.subdir, c1.sysctls;
@@
-static struct ctl_table subdir[] = {
- {
- .procname = E1,
- .maxlen = 0,
- .mode = 0555,
- .child = sysctls,
- },
- { }
-};
@r2 depends on c1 && c2 && c3@
identifier c1.subdir;
expression c2.E2;
identifier c2.base;
@@
-static struct ctl_table base[] = {
- {
- .procname = E2,
- .maxlen = 0,
- .mode = 0555,
- .child = subdir,
- },
- { }
-};
@initialize:python@
@@
def make_my_fresh_expression(s1, s2):
return '"' + s1.strip('"') + "/" + s2.strip('"') + '"'
@r3 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.sysctls;
expression c2.E2;
identifier c2.base;
identifier c3.header;
fresh identifier E3 = script:python(E2, E1) { make_my_fresh_expression(E2, E1)
};
@@
header =
-register_sysctl_table(base);
+register_sysctl(E3, sysctls);
Generated-by: Coccinelle SmPL
Signed-off-by: Luis Chamberlain
---
drivers/cdrom/cdrom.c | 23 +--
1 file changed, 1 insertion(+), 22 deletions(-)
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 9877e413fce3..1b57d4666e43 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -3691,27 +3691,6 @@ static struct ctl_table cdrom_table[] = {
},
{ }
};
-
-static struct ctl_table cdrom_cdrom_table[] = {
- {
- .procname = "cdrom",
- .maxlen = 0,
- .mode = 0555,
- .child = cdrom_table,
- },
- { }
-};
-
-/* Make sure that /proc/sys/dev is there */
-static struct ctl_table cdrom_root_table[] = {
- {
- .procname = "dev",
- .maxlen = 0,
- .mode = 0555,
- .child = cdrom_cdrom_table,
- },
- { }
-};
static struct ctl_table_header *cdrom_sysctl_header;
static void cdrom_sysctl_register(void)
@@ -3721,7 +3700,7 @@ static void cdrom_sysctl_register(void)
if (!atomic_add_unless(, 1, 1))
return;
- cdrom_sysctl_header = register_sysctl_table(cdrom_root_table);
+ cdrom_sysctl_header = register_sysctl("dev/cdrom", cdrom_table);
/* set the defaults */
cdrom_sysctl_settings.autoclose = autoclose;
--
2.33.0
[PATCH v2 8/8] eventpoll: simplify sysctl declaration with register_sysctl()
From: Xiaoming Ni
The kernel/sysctl.c is a kitchen sink where everyone leaves
their dirty dishes, this makes it very difficult to maintain.
To help with this maintenance let's start by moving sysctls to
places where they actually belong. The proc sysctl maintainers
do not want to know what sysctl knobs you wish to add for your own
piece of code, we just care about the core logic.
So move the epoll_table sysctl to fs/eventpoll.c and use
use register_sysctl().
Signed-off-by: Xiaoming Ni
Signed-off-by: Luis Chamberlain
---
fs/eventpoll.c | 10 +-
include/linux/poll.h | 2 --
include/linux/sysctl.h | 1 -
kernel/sysctl.c| 7 ---
4 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 06f4c5ae1451..e2daa940ebce 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -307,7 +307,7 @@ static void unlist_file(struct epitems_head *head)
static long long_zero;
static long long_max = LONG_MAX;
-struct ctl_table epoll_table[] = {
+static struct ctl_table epoll_table[] = {
{
.procname = "max_user_watches",
.data = _user_watches,
@@ -319,6 +319,13 @@ struct ctl_table epoll_table[] = {
},
{ }
};
+
+static void __init epoll_sysctls_init(void)
+{
+ register_sysctl("fs/epoll", epoll_table);
+}
+#else
+#define epoll_sysctls_init() do { } while (0)
#endif /* CONFIG_SYSCTL */
static const struct file_operations eventpoll_fops;
@@ -2378,6 +2385,7 @@ static int __init eventpoll_init(void)
/* Allocates slab cache used to allocate "struct eppoll_entry" */
pwq_cache = kmem_cache_create("eventpoll_pwq",
sizeof(struct eppoll_entry), 0, SLAB_PANIC|SLAB_ACCOUNT, NULL);
+ epoll_sysctls_init();
ephead_cache = kmem_cache_create("ep_head",
sizeof(struct epitems_head), 0, SLAB_PANIC|SLAB_ACCOUNT, NULL);
diff --git a/include/linux/poll.h b/include/linux/poll.h
index 1cdc32b1f1b0..a9e0e1c2d1f2 100644
--- a/include/linux/poll.h
+++ b/include/linux/poll.h
@@ -8,12 +8,10 @@
#include
#include
#include
-#include
#include
#include
#include
-extern struct ctl_table epoll_table[]; /* for sysctl */
/* ~832 bytes of stack space used max in sys_select/sys_poll before allocating
additional memory. */
#ifdef __clang__
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 718492057c70..5e0428a71899 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -218,7 +218,6 @@ extern int no_unaligned_warning;
extern struct ctl_table sysctl_mount_point[];
extern struct ctl_table random_table[];
extern struct ctl_table firmware_config_table[];
-extern struct ctl_table epoll_table[];
#else /* CONFIG_SYSCTL */
static inline struct ctl_table_header *register_sysctl_table(struct ctl_table
* table)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 6aa67c737e4e..b09ff41720e3 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -3092,13 +3092,6 @@ static struct ctl_table fs_table[] = {
.proc_handler = proc_dointvec,
},
#endif
-#ifdef CONFIG_EPOLL
- {
- .procname = "epoll",
- .mode = 0555,
- .child = epoll_table,
- },
-#endif
#endif
{
.procname = "protected_symlinks",
--
2.33.0
[PATCH v2 4/8] ocfs2: simplify subdirectory registration with register_sysctl()
There is no need to user boiler plate code to specify a set of base
directories we're going to stuff sysctls under. Simplify this by using
register_sysctl() and specifying the directory path directly.
// pycocci sysctl-subdir-register-sysctl-simplify.cocci PATH
@c1@
expression E1;
identifier subdir, sysctls;
@@
static struct ctl_table subdir[] = {
{
.procname = E1,
.maxlen = 0,
.mode = 0555,
.child = sysctls,
},
{ }
};
@c2@
identifier c1.subdir;
expression E2;
identifier base;
@@
static struct ctl_table base[] = {
{
.procname = E2,
.maxlen = 0,
.mode = 0555,
.child = subdir,
},
{ }
};
@c3@
identifier c2.base;
identifier header;
@@
header = register_sysctl_table(base);
@r1 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.subdir, c1.sysctls;
@@
-static struct ctl_table subdir[] = {
- {
- .procname = E1,
- .maxlen = 0,
- .mode = 0555,
- .child = sysctls,
- },
- { }
-};
@r2 depends on c1 && c2 && c3@
identifier c1.subdir;
expression c2.E2;
identifier c2.base;
@@
-static struct ctl_table base[] = {
- {
- .procname = E2,
- .maxlen = 0,
- .mode = 0555,
- .child = subdir,
- },
- { }
-};
@initialize:python@
@@
def make_my_fresh_expression(s1, s2):
return '"' + s1.strip('"') + "/" + s2.strip('"') + '"'
@r3 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.sysctls;
expression c2.E2;
identifier c2.base;
identifier c3.header;
fresh identifier E3 = script:python(E2, E1) { make_my_fresh_expression(E2, E1)
};
@@
header =
-register_sysctl_table(base);
+register_sysctl(E3, sysctls);
Generated-by: Coccinelle SmPL
Signed-off-by: Luis Chamberlain
---
fs/ocfs2/stackglue.c | 25 +
1 file changed, 1 insertion(+), 24 deletions(-)
diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c
index 16f1bfc407f2..731558a6f27d 100644
--- a/fs/ocfs2/stackglue.c
+++ b/fs/ocfs2/stackglue.c
@@ -672,31 +672,8 @@ static struct ctl_table ocfs2_mod_table[] = {
{ }
};
-static struct ctl_table ocfs2_kern_table[] = {
- {
- .procname = "ocfs2",
- .data = NULL,
- .maxlen = 0,
- .mode = 0555,
- .child = ocfs2_mod_table
- },
- { }
-};
-
-static struct ctl_table ocfs2_root_table[] = {
- {
- .procname = "fs",
- .data = NULL,
- .maxlen = 0,
- .mode = 0555,
- .child = ocfs2_kern_table
- },
- { }
-};
-
static struct ctl_table_header *ocfs2_table_header;
-
/*
* Initialization
*/
@@ -705,7 +682,7 @@ static int __init ocfs2_stack_glue_init(void)
{
strcpy(cluster_stack_name, OCFS2_STACK_PLUGIN_O2CB);
- ocfs2_table_header = register_sysctl_table(ocfs2_root_table);
+ ocfs2_table_header = register_sysctl("fs/ocfs2", ocfs2_mod_table);
if (!ocfs2_table_header) {
printk(KERN_ERR
"ocfs2 stack glue: unable to register sysctl\n");
--
2.33.0
[PATCH v2 2/8] i915: simplify subdirectory registration with register_sysctl()
There is no need to user boiler plate code to specify a set of base
directories we're going to stuff sysctls under. Simplify this by using
register_sysctl() and specifying the directory path directly.
// pycocci sysctl-subdir-register-sysctl-simplify.cocci PATH
@c1@
expression E1;
identifier subdir, sysctls;
@@
static struct ctl_table subdir[] = {
{
.procname = E1,
.maxlen = 0,
.mode = 0555,
.child = sysctls,
},
{ }
};
@c2@
identifier c1.subdir;
expression E2;
identifier base;
@@
static struct ctl_table base[] = {
{
.procname = E2,
.maxlen = 0,
.mode = 0555,
.child = subdir,
},
{ }
};
@c3@
identifier c2.base;
identifier header;
@@
header = register_sysctl_table(base);
@r1 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.subdir, c1.sysctls;
@@
-static struct ctl_table subdir[] = {
- {
- .procname = E1,
- .maxlen = 0,
- .mode = 0555,
- .child = sysctls,
- },
- { }
-};
@r2 depends on c1 && c2 && c3@
identifier c1.subdir;
expression c2.E2;
identifier c2.base;
@@
-static struct ctl_table base[] = {
- {
- .procname = E2,
- .maxlen = 0,
- .mode = 0555,
- .child = subdir,
- },
- { }
-};
@initialize:python@
@@
def make_my_fresh_expression(s1, s2):
return '"' + s1.strip('"') + "/" + s2.strip('"') + '"'
@r3 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.sysctls;
expression c2.E2;
identifier c2.base;
identifier c3.header;
fresh identifier E3 = script:python(E2, E1) { make_my_fresh_expression(E2, E1)
};
@@
header =
-register_sysctl_table(base);
+register_sysctl(E3, sysctls);
Generated-by: Coccinelle SmPL
Signed-off-by: Luis Chamberlain
---
drivers/gpu/drm/i915/i915_perf.c | 22 +-
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 2f01b8c0284c..5979e3258647 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -4273,26 +4273,6 @@ static struct ctl_table oa_table[] = {
{}
};
-static struct ctl_table i915_root[] = {
- {
-.procname = "i915",
-.maxlen = 0,
-.mode = 0555,
-.child = oa_table,
-},
- {}
-};
-
-static struct ctl_table dev_root[] = {
- {
-.procname = "dev",
-.maxlen = 0,
-.mode = 0555,
-.child = i915_root,
-},
- {}
-};
-
static void oa_init_supported_formats(struct i915_perf *perf)
{
struct drm_i915_private *i915 = perf->i915;
@@ -4488,7 +4468,7 @@ static int destroy_config(int id, void *p, void *data)
int i915_perf_sysctl_register(void)
{
- sysctl_header = register_sysctl_table(dev_root);
+ sysctl_header = register_sysctl("dev/i915", oa_table);
return 0;
}
--
2.33.0
[PATCH v2 5/8] test_sysctl: simplify subdirectory registration with register_sysctl()
There is no need to user boiler plate code to specify a set of base
directories we're going to stuff sysctls under. Simplify this by using
register_sysctl() and specifying the directory path directly.
// pycocci sysctl-subdir-register-sysctl-simplify.cocci lib/test_sysctl.c
@c1@
expression E1;
identifier subdir, sysctls;
@@
static struct ctl_table subdir[] = {
{
.procname = E1,
.maxlen = 0,
.mode = 0555,
.child = sysctls,
},
{ }
};
@c2@
identifier c1.subdir;
expression E2;
identifier base;
@@
static struct ctl_table base[] = {
{
.procname = E2,
.maxlen = 0,
.mode = 0555,
.child = subdir,
},
{ }
};
@c3@
identifier c2.base;
identifier header;
@@
header = register_sysctl_table(base);
@r1 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.subdir, c1.sysctls;
@@
-static struct ctl_table subdir[] = {
- {
- .procname = E1,
- .maxlen = 0,
- .mode = 0555,
- .child = sysctls,
- },
- { }
-};
@r2 depends on c1 && c2 && c3@
identifier c1.subdir;
expression c2.E2;
identifier c2.base;
@@
-static struct ctl_table base[] = {
- {
- .procname = E2,
- .maxlen = 0,
- .mode = 0555,
- .child = subdir,
- },
- { }
-};
@initialize:python@
@@
def make_my_fresh_expression(s1, s2):
return '"' + s1.strip('"') + "/" + s2.strip('"') + '"'
@r3 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.sysctls;
expression c2.E2;
identifier c2.base;
identifier c3.header;
fresh identifier E3 = script:python(E2, E1) { make_my_fresh_expression(E2, E1)
};
@@
header =
-register_sysctl_table(base);
+register_sysctl(E3, sysctls);
Generated-by: Coccinelle SmPL
Signed-off-by: Luis Chamberlain
---
lib/test_sysctl.c | 22 +-
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index 3750323973f4..a5a3d6c27e1f 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -128,26 +128,6 @@ static struct ctl_table test_table[] = {
{ }
};
-static struct ctl_table test_sysctl_table[] = {
- {
- .procname = "test_sysctl",
- .maxlen = 0,
- .mode = 0555,
- .child = test_table,
- },
- { }
-};
-
-static struct ctl_table test_sysctl_root_table[] = {
- {
- .procname = "debug",
- .maxlen = 0,
- .mode = 0555,
- .child = test_sysctl_table,
- },
- { }
-};
-
static struct ctl_table_header *test_sysctl_header;
static int __init test_sysctl_init(void)
@@ -155,7 +135,7 @@ static int __init test_sysctl_init(void)
test_data.bitmap_0001 = kzalloc(SYSCTL_TEST_BITMAP_SIZE/8, GFP_KERNEL);
if (!test_data.bitmap_0001)
return -ENOMEM;
- test_sysctl_header = register_sysctl_table(test_sysctl_root_table);
+ test_sysctl_header = register_sysctl("debug/test_sysctl", test_table);
if (!test_sysctl_header) {
kfree(test_data.bitmap_0001);
return -ENOMEM;
--
2.33.0
[PATCH v2 0/8] sysctl: second set of kernel/sysctl cleanups
This is the 2nd set of kernel/sysctl.c cleanups. The diff stat should reflect how this is a much better way to deal with theses. Fortunately coccinelle can be used to ensure correctness for most of these and/or future merge conflicts. Note that since this is part of a larger effort to cleanup kernel/sysctl.c I think we have no other option but to go with merging these patches in either Andrew's tree or keep them staged in a separate tree and send a merge request later. Otherwise kernel/sysctl.c will end up becoming a sore spot for the next merge window. Changes in this v2: * As suggested by Eric W. Biederman I dropped the subdir new call and just used the register_sysctl() by specifying the parent directory. * 0-day cleanups, commit log enhancements * Updated the coccinelle patch with register_sysctl() Luis Chamberlain (6): hpet: simplify subdirectory registration with register_sysctl() i915: simplify subdirectory registration with register_sysctl() macintosh/mac_hid.c: simplify subdirectory registration with register_sysctl() ocfs2: simplify subdirectory registration with register_sysctl() test_sysctl: simplify subdirectory registration with register_sysctl() cdrom: simplify subdirectory registration with register_sysctl() Xiaoming Ni (2): inotify: simplify subdirectory registration with register_sysctl() eventpoll: simplify sysctl declaration with register_sysctl() drivers/cdrom/cdrom.c| 23 +-- drivers/char/hpet.c | 22 +- drivers/gpu/drm/i915/i915_perf.c | 22 +- drivers/macintosh/mac_hid.c | 24 +--- fs/eventpoll.c | 10 +- fs/notify/inotify/inotify_user.c | 11 ++- fs/ocfs2/stackglue.c | 25 + include/linux/inotify.h | 3 --- include/linux/poll.h | 2 -- include/linux/sysctl.h | 1 - kernel/sysctl.c | 28 lib/test_sysctl.c| 22 +- 12 files changed, 25 insertions(+), 168 deletions(-) -- 2.33.0
Re: [PATCH 12/13] sysctl: add helper to register empty subdir
On Fri, May 29, 2020 at 08:03:02AM -0500, Eric W. Biederman wrote:
> Luis Chamberlain writes:
>
> > The way to create a subdirectory from the base set of directories
> > is a bit obscure, so provide a helper which makes this clear, and
> > also helps remove boiler plate code required to do this work.
>
> I agreee calling:
> register_sysctl("fs/binfmt_misc", sysctl_mount_point)
> is a bit obscure but if you are going to make a wrapper
> please make it the trivial one liner above.
>
> Say something that looks like:
> struct sysctl_header *register_sysctl_mount_point(const char *path)
> {
> return register_sysctl(path, sysctl_mount_point);
> }
>
> And yes please talk about a mount point and not an empty dir, as these
> are permanently empty directories to serve as mount points. There are
> some subtle but important permission checks this allows in the case of
> unprivileged mounts.
>
> Further code like this belong in proc_sysctl.c next to all of the code
> it is related to so that it is easier to see how to refactor the code if
> necessary.
Alrighty, it's been a while since this kernel/sysctl.c kitchen sink
cleanup... so it's time to respin this now that the merge window is
open. I already rebased patches, addressed all input and now just
waiting to fix any compilation errors. I'm going to split the patches
up into real small sets so to ensure we just get this through becauase
getting this in otherwise is going to be hard.
I'd appreciate folk's review once the patches start going out. I think
a hard part will be deciding what tree this should got through.
Luis
Re: refactor the i915 GVT support
mpletes and if this means having kvmgt's init routine complete, that
could end up in some longer chain or in the worst case a sort of
circular dependency which is only implicated by module loading. It'd be
really odd... but I cannot rule it out.
This is one reason I hinted that you should strive to not do much on a
module's init. If you can punt work off for later that's best.
Luis
>
> Author: Christoph Hellwig
> Date: Wed Jul 21 17:53:38 2021 +0200
>
> drm/i915/gvt: move the gvt code into kvmgt.ko
>
> Instead of having an option to build the gvt code into the main i915
> module, just move it into the kvmgt.ko module. This only requires
> a new struct with three entries that the main i915 module needs to
> request before enabling VGPU passthrough operations.
>
> This also conveniently streamlines the GVT initialization and avoids
> the need for the global device pointer.
>
> Signed-off-by: Christoph Hellwig
> Signed-off-by: Zhenyu Wang
> Link:
> http://patchwork.freedesktop.org/patch/msgid/20210721155355.173183-5-...@lst.de
> Acked-by: Zhenyu Wang
>
> On 8/26/21 6:12 AM, Zhenyu Wang wrote:
> > On 2021.08.20 12:56:34 -0700, Luis Chamberlain wrote:
> >> On Fri, Aug 20, 2021 at 04:17:24PM +0200, Christoph Hellwig wrote:
> >>> On Thu, Aug 19, 2021 at 04:29:29PM +0800, Zhenyu Wang wrote:
> >>>> I'm working on below patch to resolve this. But I met a weird issue in
> >>>> case when building i915 as module and also kvmgt module, it caused
> >>>> busy wait on request_module("kvmgt") when boot, it doesn't happen if
> >>>> building i915 into kernel. I'm not sure what could be the reason?
> >>> Luis, do you know if there is a problem with a request_module from
> >>> a driver ->probe routine that is probably called by a module_init
> >>> function itself?
> >> Generally no, but you can easily foot yourself in the feet by creating
> >> cross dependencies and not dealing with them properly. I'd make sure
> >> to keep module initialization as simple as possible, and run whatever
> >> takes more time asynchronously, then use a state machine to allow
> >> you to verify where you are in the initialization phase or query it
> >> or wait for a completion with a timeout.
> >>
> >> It seems the code in question is getting some spring cleaning, and its
> >> unclear where the code is I can inspect. If there's a tree somewhere I
> >> can take a peak I'd be happy to review possible oddities that may stick
> >> out.
> > I tried to put current patches under test here:
> > https://github.com/intel/gvt-linux/tree/gvt-staging
> > The issue can be produced with CONFIG_DRM_I915=m and
> > CONFIG_DRM_I915_GVT_KVMGT=m.
> >
> >> My goto model for these sorts of problems is to abstract the issue
> >> *outside* of the driver in question and implement new selftests to
> >> try to reproduce. This serves two purposes, 1) helps with testing
> >> 2) may allow you to see the problem more clearly.
> >>
> > I'll see if can abstract that.
> >
> > Thanks, Luis.
>
>
Re: refactor the i915 GVT support
On Fri, Aug 20, 2021 at 04:17:24PM +0200, Christoph Hellwig wrote:
> On Thu, Aug 19, 2021 at 04:29:29PM +0800, Zhenyu Wang wrote:
> > I'm working on below patch to resolve this. But I met a weird issue in
> > case when building i915 as module and also kvmgt module, it caused
> > busy wait on request_module("kvmgt") when boot, it doesn't happen if
> > building i915 into kernel. I'm not sure what could be the reason?
>
> Luis, do you know if there is a problem with a request_module from
> a driver ->probe routine that is probably called by a module_init
> function itself?
Generally no, but you can easily foot yourself in the feet by creating
cross dependencies and not dealing with them properly. I'd make sure
to keep module initialization as simple as possible, and run whatever
takes more time asynchronously, then use a state machine to allow
you to verify where you are in the initialization phase or query it
or wait for a completion with a timeout.
It seems the code in question is getting some spring cleaning, and its
unclear where the code is I can inspect. If there's a tree somewhere I
can take a peak I'd be happy to review possible oddities that may stick
out.
My goto model for these sorts of problems is to abstract the issue
*outside* of the driver in question and implement new selftests to
try to reproduce. This serves two purposes, 1) helps with testing
2) may allow you to see the problem more clearly.
Luis
Re: [PATCH v2 1/1] kernel.h: Split out panic and oops helpers
On Fri, Apr 09, 2021 at 01:02:50PM +0300, Andy Shevchenko wrote: > kernel.h is being used as a dump for all kinds of stuff for a long time. > Here is the attempt to start cleaning it up by splitting out panic and > oops helpers. > > There are several purposes of doing this: > - dropping dependency in bug.h > - dropping a loop by moving out panic_notifier.h > - unload kernel.h from something which has its own domain > > At the same time convert users tree-wide to use new headers, although > for the time being include new header back to kernel.h to avoid twisted > indirected includes for existing users. > > Signed-off-by: Andy Shevchenko > Reviewed-by: Bjorn Andersson > Acked-by: Mike Rapoport > Acked-by: Corey Minyard > Acked-by: Christian Brauner > Acked-by: Arnd Bergmann > Acked-by: Kees Cook > Acked-by: Wei Liu > Acked-by: Rasmus Villemoes > Signed-off-by: Andrew Morton Acked-by: Luis Chamberlain Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 01/13] sysctl: add new register_sysctl_subdir() helper
On Fri, May 29, 2020 at 11:13:21AM +0300, Jani Nikula wrote: > On Fri, 29 May 2020, Luis Chamberlain wrote: > > Often enough all we need to do is create a subdirectory so that > > we can stuff sysctls underneath it. However, *if* that directory > > was already created early on the boot sequence we really have no > > need to use the full boiler plate code for it, we can just use > > local variables to help us guide sysctl to place the new leaf files. > > I find it hard to figure out the lifetime requirements for the tables > passed in; when it's okay to use local variables and when you need > longer lifetimes. It's not documented, everyone appears to be using > static tables for this. It's far from obvious. I agree 2000% that it is not obvious. What made me consider it was that I *knew* that the base directory would already exist, so it wouldn't make sense for the code to rely on earlier parts of a table if part of the hierarchy already existed. In fact, a *huge* part of the due dilligence on this and futre series on this cleanup will be to be 100% sure that the base path is already created. And so this use is obviously dangerous, you just *need* to know that the base path is created before. Non-posted changes also deal with link order to help address this in other places, given that link order controls how *initcalls() (early_initcall(), late_initcall(), etc) are ordered if you have multiple of these. I had a link order series long ago which augmented our ability to make things clearer at a link order. Eventually I believe this will become more important, specially as we end up wanting to async more code. For now, we can only rely on manual code inspection for ensuring proper ordering. Part of the implicit aspects of this cleanup is to slowly make these things clearer for each base path. So... the "fs" base path will actually end up being created in fs/sysctl.c after we are *fully* done with the fs sysctl cleanups. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 09/13] firmware_loader: simplify sysctl declaration with register_sysctl_subdir()
On Fri, May 29, 2020 at 12:26:13PM +0200, Greg KH wrote: > On Fri, May 29, 2020 at 07:41:04AM +0000, Luis Chamberlain wrote: > > From: Xiaoming Ni > > > > Move the firmware config sysctl table to fallback_table.c and use the > > new register_sysctl_subdir() helper. This removes the clutter from > > kernel/sysctl.c. > > > > Signed-off-by: Xiaoming Ni > > Signed-off-by: Luis Chamberlain > > --- > > drivers/base/firmware_loader/fallback.c | 4 > > drivers/base/firmware_loader/fallback.h | 11 ++ > > drivers/base/firmware_loader/fallback_table.c | 22 +-- > > include/linux/sysctl.h| 1 - > > kernel/sysctl.c | 7 -- > > 5 files changed, 35 insertions(+), 10 deletions(-) > > So it now takes more lines than the old stuff? :( Pretty much agreed with the other changes, thanks for the review! But this diff-stat change, indeed, it is unfortunate that we end up with more code here than before. We'll try to reduce it instead somehow, however in some cases during this spring-cleaning, since the goal is to move code from one file to another, it *may* require more code. So it won't always be negative. But we'll try! Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 06/13] ocfs2: use new sysctl subdir helper register_sysctl_subdir()
On Fri, May 29, 2020 at 01:23:19AM -0700, Kees Cook wrote:
> On Fri, May 29, 2020 at 07:41:01AM +0000, Luis Chamberlain wrote:
> > This simplifies the code considerably. The following coccinelle
> > SmPL grammar rule was used to transform this code.
> >
> > // pycocci sysctl-subdir.cocci fs/ocfs2/stackglue.c
> >
> > @c1@
> > expression E1;
> > identifier subdir, sysctls;
> > @@
> >
> > static struct ctl_table subdir[] = {
> > {
> > .procname = E1,
> > .maxlen = 0,
> > .mode = 0555,
> > .child = sysctls,
> > },
> > { }
> > };
> >
> > @c2@
> > identifier c1.subdir;
> >
> > expression E2;
> > identifier base;
> > @@
> >
> > static struct ctl_table base[] = {
> > {
> > .procname = E2,
> > .maxlen = 0,
> > .mode = 0555,
> > .child = subdir,
> > },
> > { }
> > };
> >
> > @c3@
> > identifier c2.base;
> > identifier header;
> > @@
> >
> > header = register_sysctl_table(base);
> >
> > @r1 depends on c1 && c2 && c3@
> > expression c1.E1;
> > identifier c1.subdir, c1.sysctls;
> > @@
> >
> > -static struct ctl_table subdir[] = {
> > - {
> > - .procname = E1,
> > - .maxlen = 0,
> > - .mode = 0555,
> > - .child = sysctls,
> > - },
> > - { }
> > -};
> >
> > @r2 depends on c1 && c2 && c3@
> > identifier c1.subdir;
> >
> > expression c2.E2;
> > identifier c2.base;
> > @@
> > -static struct ctl_table base[] = {
> > - {
> > - .procname = E2,
> > - .maxlen = 0,
> > - .mode = 0555,
> > - .child = subdir,
> > - },
> > - { }
> > -};
> >
> > @r3 depends on c1 && c2 && c3@
> > expression c1.E1;
> > identifier c1.sysctls;
> > expression c2.E2;
> > identifier c2.base;
> > identifier c3.header;
> > @@
> >
> > header =
> > -register_sysctl_table(base);
> > +register_sysctl_subdir(E2, E1, sysctls);
> >
> > Generated-by: Coccinelle SmPL
> >
> > Signed-off-by: Luis Chamberlain
> > ---
> > fs/ocfs2/stackglue.c | 27 ---
> > 1 file changed, 4 insertions(+), 23 deletions(-)
> >
> > diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c
> > index a191094694c6..addafced7f59 100644
> > --- a/fs/ocfs2/stackglue.c
> > +++ b/fs/ocfs2/stackglue.c
> > @@ -677,28 +677,8 @@ static struct ctl_table ocfs2_mod_table[] = {
> > },
> > { }
> > };
> > -
> > -static struct ctl_table ocfs2_kern_table[] = {
> > - {
> > - .procname = "ocfs2",
> > - .data = NULL,
> > - .maxlen = 0,
> > - .mode = 0555,
> > - .child = ocfs2_mod_table
> > - },
> > - { }
> > -};
> > -
> > -static struct ctl_table ocfs2_root_table[] = {
> > - {
> > - .procname = "fs",
> > - .data = NULL,
> > - .maxlen = 0,
> > - .mode = 0555,
> > - .child = ocfs2_kern_table
> > - },
> > - { }
> > -};
> > + .data = NULL,
> > + .data = NULL,
>
> The conversion script doesn't like the .data field assignments. ;)
>
> Was this series built with allmodconfig? I would have expected this to
> blow up very badly. :)
Yikes, sense, you're right. Nope, I left the random config tests to
0day. Will fix, thanks!
Luis
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 09/13] firmware_loader: simplify sysctl declaration with register_sysctl_subdir()
From: Xiaoming Ni
Move the firmware config sysctl table to fallback_table.c and use the
new register_sysctl_subdir() helper. This removes the clutter from
kernel/sysctl.c.
Signed-off-by: Xiaoming Ni
Signed-off-by: Luis Chamberlain
---
drivers/base/firmware_loader/fallback.c | 4
drivers/base/firmware_loader/fallback.h | 11 ++
drivers/base/firmware_loader/fallback_table.c | 22 +--
include/linux/sysctl.h| 1 -
kernel/sysctl.c | 7 --
5 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index d9ac7296205e..8190653ae9a3 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -200,12 +200,16 @@ static struct class firmware_class = {
int register_sysfs_loader(void)
{
+ int ret = register_firmware_config_sysctl();
+ if (ret != 0)
+ return ret;
return class_register(_class);
}
void unregister_sysfs_loader(void)
{
class_unregister(_class);
+ unregister_firmware_config_sysctl();
}
static ssize_t firmware_loading_show(struct device *dev,
diff --git a/drivers/base/firmware_loader/fallback.h
b/drivers/base/firmware_loader/fallback.h
index 06f4577733a8..7d2cb5f6ceb8 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -42,6 +42,17 @@ void fw_fallback_set_default_timeout(void);
int register_sysfs_loader(void);
void unregister_sysfs_loader(void);
+#ifdef CONFIG_SYSCTL
+extern int register_firmware_config_sysctl(void);
+extern void unregister_firmware_config_sysctl(void);
+#else
+static inline int register_firmware_config_sysctl(void)
+{
+ return 0;
+}
+static inline void unregister_firmware_config_sysctl(void) { }
+#endif /* CONFIG_SYSCTL */
+
#else /* CONFIG_FW_LOADER_USER_HELPER */
static inline int firmware_fallback_sysfs(struct firmware *fw, const char
*name,
struct device *device,
diff --git a/drivers/base/firmware_loader/fallback_table.c
b/drivers/base/firmware_loader/fallback_table.c
index 46a731dede6f..4234aa5ee5df 100644
--- a/drivers/base/firmware_loader/fallback_table.c
+++ b/drivers/base/firmware_loader/fallback_table.c
@@ -24,7 +24,7 @@ struct firmware_fallback_config fw_fallback_config = {
EXPORT_SYMBOL_NS_GPL(fw_fallback_config, FIRMWARE_LOADER_PRIVATE);
#ifdef CONFIG_SYSCTL
-struct ctl_table firmware_config_table[] = {
+static struct ctl_table firmware_config_table[] = {
{
.procname = "force_sysfs_fallback",
.data = _fallback_config.force_sysfs_fallback,
@@ -45,4 +45,22 @@ struct ctl_table firmware_config_table[] = {
},
{ }
};
-#endif
+
+static struct ctl_table_header *hdr;
+int register_firmware_config_sysctl(void)
+{
+ if (hdr)
+ return -EEXIST;
+ hdr = register_sysctl_subdir("kernel", "firmware_config",
+firmware_config_table);
+ if (!hdr)
+ return -ENOMEM;
+ return 0;
+}
+
+void unregister_firmware_config_sysctl(void)
+{
+ if (hdr)
+ unregister_sysctl_table(hdr);
+}
+#endif /* CONFIG_SYSCTL */
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 58bc978d4f03..aa01f54d0442 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -217,7 +217,6 @@ extern int no_unaligned_warning;
extern struct ctl_table sysctl_mount_point[];
extern struct ctl_table random_table[];
-extern struct ctl_table firmware_config_table[];
extern struct ctl_table epoll_table[];
#else /* CONFIG_SYSCTL */
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 30c2d521502a..e007375c8a11 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2088,13 +2088,6 @@ static struct ctl_table kern_table[] = {
.mode = 0555,
.child = usermodehelper_table,
},
-#ifdef CONFIG_FW_LOADER_USER_HELPER
- {
- .procname = "firmware_config",
- .mode = 0555,
- .child = firmware_config_table,
- },
-#endif
{
.procname = "overflowuid",
.data = ,
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 12/13] sysctl: add helper to register empty subdir
The way to create a subdirectory from the base set of directories
is a bit obscure, so provide a helper which makes this clear, and
also helps remove boiler plate code required to do this work.
Signed-off-by: Luis Chamberlain
---
include/linux/sysctl.h | 7 +++
kernel/sysctl.c| 16 +---
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 33a471b56345..89c92390e6de 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -208,6 +208,8 @@ extern void register_sysctl_init(const char *path, struct
ctl_table *table,
extern struct ctl_table_header *register_sysctl_subdir(const char *base,
const char *subdir,
struct ctl_table *table);
+extern void register_sysctl_empty_subdir(const char *base, const char *subdir);
+
void do_sysctl_args(void);
extern int pwrsw_enabled;
@@ -231,6 +233,11 @@ inline struct ctl_table_header
*register_sysctl_subdir(const char *base,
return NULL;
}
+static inline void register_sysctl_empty_subdir(const char *base,
+ const char *subdir)
+{
+}
+
static inline struct ctl_table_header *register_sysctl_paths(
const struct ctl_path *path, struct ctl_table *table)
{
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f9a35325d5d5..460532cd5ac8 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -3188,13 +3188,17 @@ struct ctl_table_header *register_sysctl_subdir(const
char *base,
{ }
};
- if (!table->procname)
+ if (table != sysctl_mount_point && !table->procname)
goto out;
hdr = register_sysctl_table(base_table);
if (unlikely(!hdr)) {
- pr_err("failed when creating subdirectory sysctl %s/%s/%s\n",
- base, subdir, table->procname);
+ if (table != sysctl_mount_point)
+ pr_err("failed when creating subdirectory sysctl
%s/%s/%s\n",
+ base, subdir, table->procname);
+ else
+ pr_err("failed when creating empty subddirectory
%s/%s\n",
+ base, subdir);
goto out;
}
kmemleak_not_leak(hdr);
@@ -3202,6 +3206,12 @@ struct ctl_table_header *register_sysctl_subdir(const
char *base,
return hdr;
}
EXPORT_SYMBOL_GPL(register_sysctl_subdir);
+
+void register_sysctl_empty_subdir(const char *base,
+ const char *subdir)
+{
+ register_sysctl_subdir(base, subdir, sysctl_mount_point);
+}
#endif /* CONFIG_SYSCTL */
/*
* No sense putting this after each symbol definition, twice,
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 02/13] cdrom: use new sysctl subdir helper register_sysctl_subdir()
This simplifies the code considerably. The following coccinelle
SmPL grammar rule was used to transform this code.
// pycocci sysctl-subdir.cocci drivers/cdrom/cdrom.c
@c1@
expression E1;
identifier subdir, sysctls;
@@
static struct ctl_table subdir[] = {
{
.procname = E1,
.maxlen = 0,
.mode = 0555,
.child = sysctls,
},
{ }
};
@c2@
identifier c1.subdir;
expression E2;
identifier base;
@@
static struct ctl_table base[] = {
{
.procname = E2,
.maxlen = 0,
.mode = 0555,
.child = subdir,
},
{ }
};
@c3@
identifier c2.base;
identifier header;
@@
header = register_sysctl_table(base);
@r1 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.subdir, c1.sysctls;
@@
-static struct ctl_table subdir[] = {
- {
- .procname = E1,
- .maxlen = 0,
- .mode = 0555,
- .child = sysctls,
- },
- { }
-};
@r2 depends on c1 && c2 && c3@
identifier c1.subdir;
expression c2.E2;
identifier c2.base;
@@
-static struct ctl_table base[] = {
- {
- .procname = E2,
- .maxlen = 0,
- .mode = 0555,
- .child = subdir,
- },
- { }
-};
@r3 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.sysctls;
expression c2.E2;
identifier c2.base;
identifier c3.header;
@@
header =
-register_sysctl_table(base);
+register_sysctl_subdir(E2, E1, sysctls);
Generated-by: Coccinelle SmPL
Signed-off-by: Luis Chamberlain
---
drivers/cdrom/cdrom.c | 23 ++-
1 file changed, 2 insertions(+), 21 deletions(-)
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index a0a7ae705de8..3c638f464cef 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -3719,26 +3719,6 @@ static struct ctl_table cdrom_table[] = {
{ }
};
-static struct ctl_table cdrom_cdrom_table[] = {
- {
- .procname = "cdrom",
- .maxlen = 0,
- .mode = 0555,
- .child = cdrom_table,
- },
- { }
-};
-
-/* Make sure that /proc/sys/dev is there */
-static struct ctl_table cdrom_root_table[] = {
- {
- .procname = "dev",
- .maxlen = 0,
- .mode = 0555,
- .child = cdrom_cdrom_table,
- },
- { }
-};
static struct ctl_table_header *cdrom_sysctl_header;
static void cdrom_sysctl_register(void)
@@ -3748,7 +3728,8 @@ static void cdrom_sysctl_register(void)
if (!atomic_add_unless(, 1, 1))
return;
- cdrom_sysctl_header = register_sysctl_table(cdrom_root_table);
+ cdrom_sysctl_header = register_sysctl_subdir("dev", "cdrom",
+cdrom_table);
/* set the defaults */
cdrom_sysctl_settings.autoclose = autoclose;
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 11/13] random: simplify sysctl declaration with register_sysctl_subdir()
From: Xiaoming Ni
Move random_table sysctl from kernel/sysctl.c to drivers/char/random.c
and use register_sysctl_subdir() to help remove the clutter out of
kernel/sysctl.c.
Signed-off-by: Xiaoming Ni
Signed-off-by: Luis Chamberlain
---
drivers/char/random.c | 14 --
include/linux/sysctl.h | 1 -
kernel/sysctl.c| 5 -
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index a7cf6aa65908..73fd4b6e9c18 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -2101,8 +2101,7 @@ static int proc_do_entropy(struct ctl_table *table, int
write,
}
static int sysctl_poolsize = INPUT_POOL_WORDS * 32;
-extern struct ctl_table random_table[];
-struct ctl_table random_table[] = {
+static struct ctl_table random_table[] = {
{
.procname = "poolsize",
.data = _poolsize,
@@ -2164,6 +2163,17 @@ struct ctl_table random_table[] = {
#endif
{ }
};
+
+/*
+ * rand_initialize() is called before sysctl_init(),
+ * so we cannot call register_sysctl_init() in rand_initialize()
+ */
+static int __init random_sysctls_init(void)
+{
+ register_sysctl_subdir("kernel", "random", random_table);
+ return 0;
+}
+device_initcall(random_sysctls_init);
#endif /* CONFIG_SYSCTL */
struct batched_entropy {
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index e5364b69dd95..33a471b56345 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -216,7 +216,6 @@ extern int unaligned_dump_stack;
extern int no_unaligned_warning;
extern struct ctl_table sysctl_mount_point[];
-extern struct ctl_table random_table[];
#else /* CONFIG_SYSCTL */
static inline struct ctl_table_header *register_sysctl_table(struct ctl_table
* table)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 5c116904feb7..f9a35325d5d5 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2078,11 +2078,6 @@ static struct ctl_table kern_table[] = {
.mode = 0644,
.proc_handler = sysctl_max_threads,
},
- {
- .procname = "random",
- .mode = 0555,
- .child = random_table,
- },
{
.procname = "usermodehelper",
.mode = 0555,
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 13/13] fs: move binfmt_misc sysctl to its own file
This moves the binfmt_misc sysctl to its own file to help remove
clutter from kernel/sysctl.c.
Signed-off-by: Luis Chamberlain
---
fs/binfmt_misc.c | 1 +
kernel/sysctl.c | 7 ---
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index f69a043f562b..656b3f5f3bbf 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -821,6 +821,7 @@ static int __init init_misc_binfmt(void)
int err = register_filesystem(_fs_type);
if (!err)
insert_binfmt(_format);
+ register_sysctl_empty_subdir("fs", "binfmt_misc");
return err;
}
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 460532cd5ac8..7714e7b476c2 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -3042,13 +3042,6 @@ static struct ctl_table fs_table[] = {
.extra1 = SYSCTL_ZERO,
.extra2 = SYSCTL_TWO,
},
-#if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE)
- {
- .procname = "binfmt_misc",
- .mode = 0555,
- .child = sysctl_mount_point,
- },
-#endif
{
.procname = "pipe-max-size",
.data = _max_size,
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 00/13] sysctl: spring cleaning
Me and Xiaoming are working on some kernel/sysctl.c spring cleaning. During a recent linux-next merge conflict it became clear that the kitchen sink on kernel/sysctl.c creates too many conflicts, and so we need to do away with stuffing everyone's knobs on this one file. This is part of that work. This is not expected to get merged yet, but since our delta is pretty considerable at this point, we need to piece meal this and collect reviews for what we have so far. This follows up on some of his recent work. This series focuses on a new helper to deal with subdirectories and empty subdirectories. The terminology that we will embrace will be that things like "fs", "kernel", "debug" are based directories, and directories underneath this are subdirectories. In this case, the cleanup ends up also trimming the amount of code we have for sysctls. If this seems reasonable we'll kdocify this a bit too. This code has been boot tested without issues, and I'm letting 0day do its thing to test against many kconfig builds. If you however spot any issues please let us know. Luis Chamberlain (9): sysctl: add new register_sysctl_subdir() helper cdrom: use new sysctl subdir helper register_sysctl_subdir() hpet: use new sysctl subdir helper register_sysctl_subdir() i915: use new sysctl subdir helper register_sysctl_subdir() macintosh/mac_hid.c: use new sysctl subdir helper register_sysctl_subdir() ocfs2: use new sysctl subdir helper register_sysctl_subdir() test_sysctl: use new sysctl subdir helper register_sysctl_subdir() sysctl: add helper to register empty subdir fs: move binfmt_misc sysctl to its own file Xiaoming Ni (4): inotify: simplify sysctl declaration with register_sysctl_subdir() firmware_loader: simplify sysctl declaration with register_sysctl_subdir() eventpoll: simplify sysctl declaration with register_sysctl_subdir() random: simplify sysctl declaration with register_sysctl_subdir() drivers/base/firmware_loader/fallback.c | 4 + drivers/base/firmware_loader/fallback.h | 11 +++ drivers/base/firmware_loader/fallback_table.c | 22 - drivers/cdrom/cdrom.c | 23 + drivers/char/hpet.c | 22 + drivers/char/random.c | 14 +++- drivers/gpu/drm/i915/i915_perf.c | 22 + drivers/macintosh/mac_hid.c | 25 +- fs/binfmt_misc.c | 1 + fs/eventpoll.c| 10 ++- fs/notify/inotify/inotify_user.c | 11 ++- fs/ocfs2/stackglue.c | 27 +- include/linux/inotify.h | 3 - include/linux/poll.h | 2 - include/linux/sysctl.h| 21 - kernel/sysctl.c | 84 +++ lib/test_sysctl.c | 23 + 17 files changed, 144 insertions(+), 181 deletions(-) -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 10/13] eventpoll: simplify sysctl declaration with register_sysctl_subdir()
From: Xiaoming Ni
Move epoll_table sysctl to fs/eventpoll.c and remove the
clutter out of kernel/sysctl.c by using register_sysctl_subdir()..
Signed-off-by: Xiaoming Ni
Signed-off-by: Luis Chamberlain
---
fs/eventpoll.c | 10 +-
include/linux/poll.h | 2 --
include/linux/sysctl.h | 1 -
kernel/sysctl.c| 7 ---
4 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 12eebcdea9c8..957ebc9700e3 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -299,7 +299,7 @@ static LIST_HEAD(tfile_check_list);
static long long_zero;
static long long_max = LONG_MAX;
-struct ctl_table epoll_table[] = {
+static struct ctl_table epoll_table[] = {
{
.procname = "max_user_watches",
.data = _user_watches,
@@ -311,6 +311,13 @@ struct ctl_table epoll_table[] = {
},
{ }
};
+
+static void __init epoll_sysctls_init(void)
+{
+ register_sysctl_subdir("fs", "epoll", epoll_table);
+}
+#else
+#define epoll_sysctls_init() do { } while (0)
#endif /* CONFIG_SYSCTL */
static const struct file_operations eventpoll_fops;
@@ -2422,6 +2429,7 @@ static int __init eventpoll_init(void)
/* Allocates slab cache used to allocate "struct eppoll_entry" */
pwq_cache = kmem_cache_create("eventpoll_pwq",
sizeof(struct eppoll_entry), 0, SLAB_PANIC|SLAB_ACCOUNT, NULL);
+ epoll_sysctls_init();
return 0;
}
diff --git a/include/linux/poll.h b/include/linux/poll.h
index 1cdc32b1f1b0..a9e0e1c2d1f2 100644
--- a/include/linux/poll.h
+++ b/include/linux/poll.h
@@ -8,12 +8,10 @@
#include
#include
#include
-#include
#include
#include
#include
-extern struct ctl_table epoll_table[]; /* for sysctl */
/* ~832 bytes of stack space used max in sys_select/sys_poll before allocating
additional memory. */
#ifdef __clang__
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index aa01f54d0442..e5364b69dd95 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -217,7 +217,6 @@ extern int no_unaligned_warning;
extern struct ctl_table sysctl_mount_point[];
extern struct ctl_table random_table[];
-extern struct ctl_table epoll_table[];
#else /* CONFIG_SYSCTL */
static inline struct ctl_table_header *register_sysctl_table(struct ctl_table
* table)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index e007375c8a11..5c116904feb7 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -3001,13 +3001,6 @@ static struct ctl_table fs_table[] = {
.proc_handler = proc_dointvec,
},
#endif
-#ifdef CONFIG_EPOLL
- {
- .procname = "epoll",
- .mode = 0555,
- .child = epoll_table,
- },
-#endif
#endif
{
.procname = "protected_symlinks",
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 03/13] hpet: use new sysctl subdir helper register_sysctl_subdir()
This simplifies the code considerably. The following coccinelle
SmPL grammar rule was used to transform this code.
// pycocci sysctl-subdir.cocci drivers/char/hpet.c
@c1@
expression E1;
identifier subdir, sysctls;
@@
static struct ctl_table subdir[] = {
{
.procname = E1,
.maxlen = 0,
.mode = 0555,
.child = sysctls,
},
{ }
};
@c2@
identifier c1.subdir;
expression E2;
identifier base;
@@
static struct ctl_table base[] = {
{
.procname = E2,
.maxlen = 0,
.mode = 0555,
.child = subdir,
},
{ }
};
@c3@
identifier c2.base;
identifier header;
@@
header = register_sysctl_table(base);
@r1 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.subdir, c1.sysctls;
@@
-static struct ctl_table subdir[] = {
- {
- .procname = E1,
- .maxlen = 0,
- .mode = 0555,
- .child = sysctls,
- },
- { }
-};
@r2 depends on c1 && c2 && c3@
identifier c1.subdir;
expression c2.E2;
identifier c2.base;
@@
-static struct ctl_table base[] = {
- {
- .procname = E2,
- .maxlen = 0,
- .mode = 0555,
- .child = subdir,
- },
- { }
-};
@r3 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.sysctls;
expression c2.E2;
identifier c2.base;
identifier c3.header;
@@
header =
-register_sysctl_table(base);
+register_sysctl_subdir(E2, E1, sysctls);
Generated-by: Coccinelle SmPL
Signed-off-by: Luis Chamberlain
---
drivers/char/hpet.c | 22 +-
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index ed3b7dab678d..169c970d5ff8 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -746,26 +746,6 @@ static struct ctl_table hpet_table[] = {
{}
};
-static struct ctl_table hpet_root[] = {
- {
-.procname = "hpet",
-.maxlen = 0,
-.mode = 0555,
-.child = hpet_table,
-},
- {}
-};
-
-static struct ctl_table dev_root[] = {
- {
-.procname = "dev",
-.maxlen = 0,
-.mode = 0555,
-.child = hpet_root,
-},
- {}
-};
-
static struct ctl_table_header *sysctl_header;
/*
@@ -1059,7 +1039,7 @@ static int __init hpet_init(void)
if (result < 0)
return -ENODEV;
- sysctl_header = register_sysctl_table(dev_root);
+ sysctl_header = register_sysctl_subdir("dev", "hpet", hpet_table);
result = acpi_bus_register_driver(_acpi_driver);
if (result < 0) {
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 06/13] ocfs2: use new sysctl subdir helper register_sysctl_subdir()
This simplifies the code considerably. The following coccinelle
SmPL grammar rule was used to transform this code.
// pycocci sysctl-subdir.cocci fs/ocfs2/stackglue.c
@c1@
expression E1;
identifier subdir, sysctls;
@@
static struct ctl_table subdir[] = {
{
.procname = E1,
.maxlen = 0,
.mode = 0555,
.child = sysctls,
},
{ }
};
@c2@
identifier c1.subdir;
expression E2;
identifier base;
@@
static struct ctl_table base[] = {
{
.procname = E2,
.maxlen = 0,
.mode = 0555,
.child = subdir,
},
{ }
};
@c3@
identifier c2.base;
identifier header;
@@
header = register_sysctl_table(base);
@r1 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.subdir, c1.sysctls;
@@
-static struct ctl_table subdir[] = {
- {
- .procname = E1,
- .maxlen = 0,
- .mode = 0555,
- .child = sysctls,
- },
- { }
-};
@r2 depends on c1 && c2 && c3@
identifier c1.subdir;
expression c2.E2;
identifier c2.base;
@@
-static struct ctl_table base[] = {
- {
- .procname = E2,
- .maxlen = 0,
- .mode = 0555,
- .child = subdir,
- },
- { }
-};
@r3 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.sysctls;
expression c2.E2;
identifier c2.base;
identifier c3.header;
@@
header =
-register_sysctl_table(base);
+register_sysctl_subdir(E2, E1, sysctls);
Generated-by: Coccinelle SmPL
Signed-off-by: Luis Chamberlain
---
fs/ocfs2/stackglue.c | 27 ---
1 file changed, 4 insertions(+), 23 deletions(-)
diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c
index a191094694c6..addafced7f59 100644
--- a/fs/ocfs2/stackglue.c
+++ b/fs/ocfs2/stackglue.c
@@ -677,28 +677,8 @@ static struct ctl_table ocfs2_mod_table[] = {
},
{ }
};
-
-static struct ctl_table ocfs2_kern_table[] = {
- {
- .procname = "ocfs2",
- .data = NULL,
- .maxlen = 0,
- .mode = 0555,
- .child = ocfs2_mod_table
- },
- { }
-};
-
-static struct ctl_table ocfs2_root_table[] = {
- {
- .procname = "fs",
- .data = NULL,
- .maxlen = 0,
- .mode = 0555,
- .child = ocfs2_kern_table
- },
- { }
-};
+ .data = NULL,
+ .data = NULL,
static struct ctl_table_header *ocfs2_table_header;
@@ -711,7 +691,8 @@ static int __init ocfs2_stack_glue_init(void)
{
strcpy(cluster_stack_name, OCFS2_STACK_PLUGIN_O2CB);
- ocfs2_table_header = register_sysctl_table(ocfs2_root_table);
+ ocfs2_table_header = register_sysctl_subdir("fs", "ocfs2",
+ ocfs2_mod_table);
if (!ocfs2_table_header) {
printk(KERN_ERR
"ocfs2 stack glue: unable to register sysctl\n");
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 05/13] macintosh/mac_hid.c: use new sysctl subdir helper register_sysctl_subdir()
This simplifies the code considerably. The following coccinelle
SmPL grammar rule was used to transform this code.
// pycocci sysctl-subdir.cocci drivers/macintosh/mac_hid.c
@c1@
expression E1;
identifier subdir, sysctls;
@@
static struct ctl_table subdir[] = {
{
.procname = E1,
.maxlen = 0,
.mode = 0555,
.child = sysctls,
},
{ }
};
@c2@
identifier c1.subdir;
expression E2;
identifier base;
@@
static struct ctl_table base[] = {
{
.procname = E2,
.maxlen = 0,
.mode = 0555,
.child = subdir,
},
{ }
};
@c3@
identifier c2.base;
identifier header;
@@
header = register_sysctl_table(base);
@r1 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.subdir, c1.sysctls;
@@
-static struct ctl_table subdir[] = {
- {
- .procname = E1,
- .maxlen = 0,
- .mode = 0555,
- .child = sysctls,
- },
- { }
-};
@r2 depends on c1 && c2 && c3@
identifier c1.subdir;
expression c2.E2;
identifier c2.base;
@@
-static struct ctl_table base[] = {
- {
- .procname = E2,
- .maxlen = 0,
- .mode = 0555,
- .child = subdir,
- },
- { }
-};
@r3 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.sysctls;
expression c2.E2;
identifier c2.base;
identifier c3.header;
@@
header =
-register_sysctl_table(base);
+register_sysctl_subdir(E2, E1, sysctls);
Generated-by: Coccinelle SmPL
Signed-off-by: Luis Chamberlain
---
drivers/macintosh/mac_hid.c | 25 ++---
1 file changed, 2 insertions(+), 23 deletions(-)
diff --git a/drivers/macintosh/mac_hid.c b/drivers/macintosh/mac_hid.c
index 28b8581b44dd..736d0e151716 100644
--- a/drivers/macintosh/mac_hid.c
+++ b/drivers/macintosh/mac_hid.c
@@ -239,33 +239,12 @@ static struct ctl_table mac_hid_files[] = {
{ }
};
-/* dir in /proc/sys/dev */
-static struct ctl_table mac_hid_dir[] = {
- {
- .procname = "mac_hid",
- .maxlen = 0,
- .mode = 0555,
- .child = mac_hid_files,
- },
- { }
-};
-
-/* /proc/sys/dev itself, in case that is not there yet */
-static struct ctl_table mac_hid_root_dir[] = {
- {
- .procname = "dev",
- .maxlen = 0,
- .mode = 0555,
- .child = mac_hid_dir,
- },
- { }
-};
-
static struct ctl_table_header *mac_hid_sysctl_header;
static int __init mac_hid_init(void)
{
- mac_hid_sysctl_header = register_sysctl_table(mac_hid_root_dir);
+ mac_hid_sysctl_header = register_sysctl_subdir("dev", "mac_hid",
+ mac_hid_files);
if (!mac_hid_sysctl_header)
return -ENOMEM;
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 04/13] i915: use new sysctl subdir helper register_sysctl_subdir()
This simplifies the code considerably. The following coccinelle
SmPL grammar rule was used to transform this code.
// pycocci sysctl-subdir.cocci drivers/gpu/drm/i915/i915_perf.c
@c1@
expression E1;
identifier subdir, sysctls;
@@
static struct ctl_table subdir[] = {
{
.procname = E1,
.maxlen = 0,
.mode = 0555,
.child = sysctls,
},
{ }
};
@c2@
identifier c1.subdir;
expression E2;
identifier base;
@@
static struct ctl_table base[] = {
{
.procname = E2,
.maxlen = 0,
.mode = 0555,
.child = subdir,
},
{ }
};
@c3@
identifier c2.base;
identifier header;
@@
header = register_sysctl_table(base);
@r1 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.subdir, c1.sysctls;
@@
-static struct ctl_table subdir[] = {
- {
- .procname = E1,
- .maxlen = 0,
- .mode = 0555,
- .child = sysctls,
- },
- { }
-};
@r2 depends on c1 && c2 && c3@
identifier c1.subdir;
expression c2.E2;
identifier c2.base;
@@
-static struct ctl_table base[] = {
- {
- .procname = E2,
- .maxlen = 0,
- .mode = 0555,
- .child = subdir,
- },
- { }
-};
@r3 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.sysctls;
expression c2.E2;
identifier c2.base;
identifier c3.header;
@@
header =
-register_sysctl_table(base);
+register_sysctl_subdir(E2, E1, sysctls);
Generated-by: Coccinelle SmPL
Signed-off-by: Luis Chamberlain
---
drivers/gpu/drm/i915/i915_perf.c | 22 +-
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 665bb076e84d..52509b573794 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -4203,26 +4203,6 @@ static struct ctl_table oa_table[] = {
{}
};
-static struct ctl_table i915_root[] = {
- {
-.procname = "i915",
-.maxlen = 0,
-.mode = 0555,
-.child = oa_table,
-},
- {}
-};
-
-static struct ctl_table dev_root[] = {
- {
-.procname = "dev",
-.maxlen = 0,
-.mode = 0555,
-.child = i915_root,
-},
- {}
-};
-
/**
* i915_perf_init - initialize i915-perf state on module bind
* @i915: i915 device instance
@@ -4383,7 +4363,7 @@ static int destroy_config(int id, void *p, void *data)
void i915_perf_sysctl_register(void)
{
- sysctl_header = register_sysctl_table(dev_root);
+ sysctl_header = register_sysctl_subdir("dev", "i915", oa_table);
}
void i915_perf_sysctl_unregister(void)
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 08/13] inotify: simplify sysctl declaration with register_sysctl_subdir()
From: Xiaoming Ni
move inotify_user sysctl to inotify_user.c and use the new
register_sysctl_subdir() helper.
Signed-off-by: Xiaoming Ni
Signed-off-by: Luis Chamberlain
---
fs/notify/inotify/inotify_user.c | 11 ++-
include/linux/inotify.h | 3 ---
kernel/sysctl.c | 11 ---
3 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index f88bbcc9efeb..64859fbf8463 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -46,7 +46,7 @@ struct kmem_cache *inotify_inode_mark_cachep __read_mostly;
#include
-struct ctl_table inotify_table[] = {
+static struct ctl_table inotify_table[] = {
{
.procname = "max_user_instances",
.data =
_user_ns.ucount_max[UCOUNT_INOTIFY_INSTANCES],
@@ -73,6 +73,14 @@ struct ctl_table inotify_table[] = {
},
{ }
};
+
+static void __init inotify_sysctls_init(void)
+{
+ register_sysctl_subdir("fs", "inotify", inotify_table);
+}
+
+#else
+#define inotify_sysctls_init() do { } while (0)
#endif /* CONFIG_SYSCTL */
static inline __u32 inotify_arg_to_mask(u32 arg)
@@ -826,6 +834,7 @@ static int __init inotify_user_setup(void)
inotify_max_queued_events = 16384;
init_user_ns.ucount_max[UCOUNT_INOTIFY_INSTANCES] = 128;
init_user_ns.ucount_max[UCOUNT_INOTIFY_WATCHES] = 8192;
+ inotify_sysctls_init();
return 0;
}
diff --git a/include/linux/inotify.h b/include/linux/inotify.h
index 6a24905f6e1e..8d20caa1b268 100644
--- a/include/linux/inotify.h
+++ b/include/linux/inotify.h
@@ -7,11 +7,8 @@
#ifndef _LINUX_INOTIFY_H
#define _LINUX_INOTIFY_H
-#include
#include
-extern struct ctl_table inotify_table[]; /* for sysctl */
-
#define ALL_INOTIFY_BITS (IN_ACCESS | IN_MODIFY | IN_ATTRIB | IN_CLOSE_WRITE |
\
IN_CLOSE_NOWRITE | IN_OPEN | IN_MOVED_FROM | \
IN_MOVED_TO | IN_CREATE | IN_DELETE | \
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 04ff032f2863..30c2d521502a 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -123,10 +123,6 @@ static const int maxolduid = 65535;
static int ngroups_max = NGROUPS_MAX;
static const int cap_last_cap = CAP_LAST_CAP;
-#ifdef CONFIG_INOTIFY_USER
-#include
-#endif
-
#ifdef CONFIG_PROC_SYSCTL
/**
@@ -3012,13 +3008,6 @@ static struct ctl_table fs_table[] = {
.proc_handler = proc_dointvec,
},
#endif
-#ifdef CONFIG_INOTIFY_USER
- {
- .procname = "inotify",
- .mode = 0555,
- .child = inotify_table,
- },
-#endif
#ifdef CONFIG_EPOLL
{
.procname = "epoll",
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 01/13] sysctl: add new register_sysctl_subdir() helper
Often enough all we need to do is create a subdirectory so that
we can stuff sysctls underneath it. However, *if* that directory
was already created early on the boot sequence we really have no
need to use the full boiler plate code for it, we can just use
local variables to help us guide sysctl to place the new leaf files.
So use a helper to do precisely this.
Signed-off-by: Luis Chamberlain
---
include/linux/sysctl.h | 11 +++
kernel/sysctl.c| 37 +
2 files changed, 48 insertions(+)
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index ddaa06ddd852..58bc978d4f03 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -205,6 +205,9 @@ void unregister_sysctl_table(struct ctl_table_header *
table);
extern int sysctl_init(void);
extern void register_sysctl_init(const char *path, struct ctl_table *table,
const char *table_name);
+extern struct ctl_table_header *register_sysctl_subdir(const char *base,
+ const char *subdir,
+ struct ctl_table *table);
void do_sysctl_args(void);
extern int pwrsw_enabled;
@@ -223,6 +226,14 @@ static inline struct ctl_table_header
*register_sysctl_table(struct ctl_table *
return NULL;
}
+static
+inline struct ctl_table_header *register_sysctl_subdir(const char *base,
+ const char *subdir,
+ struct ctl_table *table)
+{
+ return NULL;
+}
+
static inline struct ctl_table_header *register_sysctl_paths(
const struct ctl_path *path, struct ctl_table *table)
{
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 008ac0576ae5..04ff032f2863 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -3195,6 +3195,43 @@ void __init register_sysctl_init(const char *path,
struct ctl_table *table,
}
kmemleak_not_leak(hdr);
}
+
+struct ctl_table_header *register_sysctl_subdir(const char *base,
+ const char *subdir,
+ struct ctl_table *table)
+{
+ struct ctl_table_header *hdr = NULL;
+ struct ctl_table subdir_table[] = {
+ {
+ .procname = subdir,
+ .mode = 0555,
+ .child = table,
+ },
+ { }
+ };
+ struct ctl_table base_table[] = {
+ {
+ .procname = base,
+ .mode = 0555,
+ .child = subdir_table,
+ },
+ { }
+ };
+
+ if (!table->procname)
+ goto out;
+
+ hdr = register_sysctl_table(base_table);
+ if (unlikely(!hdr)) {
+ pr_err("failed when creating subdirectory sysctl %s/%s/%s\n",
+ base, subdir, table->procname);
+ goto out;
+ }
+ kmemleak_not_leak(hdr);
+out:
+ return hdr;
+}
+EXPORT_SYMBOL_GPL(register_sysctl_subdir);
#endif /* CONFIG_SYSCTL */
/*
* No sense putting this after each symbol definition, twice,
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 07/13] test_sysctl: use new sysctl subdir helper register_sysctl_subdir()
This simplifies the code considerably. The following coccinelle
SmPL grammar rule was used to transform this code.
// pycocci sysctl-subdir.cocci lib/test_sysctl.c
@c1@
expression E1;
identifier subdir, sysctls;
@@
static struct ctl_table subdir[] = {
{
.procname = E1,
.maxlen = 0,
.mode = 0555,
.child = sysctls,
},
{ }
};
@c2@
identifier c1.subdir;
expression E2;
identifier base;
@@
static struct ctl_table base[] = {
{
.procname = E2,
.maxlen = 0,
.mode = 0555,
.child = subdir,
},
{ }
};
@c3@
identifier c2.base;
identifier header;
@@
header = register_sysctl_table(base);
@r1 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.subdir, c1.sysctls;
@@
-static struct ctl_table subdir[] = {
- {
- .procname = E1,
- .maxlen = 0,
- .mode = 0555,
- .child = sysctls,
- },
- { }
-};
@r2 depends on c1 && c2 && c3@
identifier c1.subdir;
expression c2.E2;
identifier c2.base;
@@
-static struct ctl_table base[] = {
- {
- .procname = E2,
- .maxlen = 0,
- .mode = 0555,
- .child = subdir,
- },
- { }
-};
@r3 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.sysctls;
expression c2.E2;
identifier c2.base;
identifier c3.header;
@@
header =
-register_sysctl_table(base);
+register_sysctl_subdir(E2, E1, sysctls);
Generated-by: Coccinelle SmPL
Signed-off-by: Luis Chamberlain
---
lib/test_sysctl.c | 23 ++-
1 file changed, 2 insertions(+), 21 deletions(-)
diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index 84eaae22d3a6..b17581307756 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -128,26 +128,6 @@ static struct ctl_table test_table[] = {
{ }
};
-static struct ctl_table test_sysctl_table[] = {
- {
- .procname = "test_sysctl",
- .maxlen = 0,
- .mode = 0555,
- .child = test_table,
- },
- { }
-};
-
-static struct ctl_table test_sysctl_root_table[] = {
- {
- .procname = "debug",
- .maxlen = 0,
- .mode = 0555,
- .child = test_sysctl_table,
- },
- { }
-};
-
static struct ctl_table_header *test_sysctl_header;
static int __init test_sysctl_init(void)
@@ -155,7 +135,8 @@ static int __init test_sysctl_init(void)
test_data.bitmap_0001 = kzalloc(SYSCTL_TEST_BITMAP_SIZE/8, GFP_KERNEL);
if (!test_data.bitmap_0001)
return -ENOMEM;
- test_sysctl_header = register_sysctl_table(test_sysctl_root_table);
+ test_sysctl_header = register_sysctl_subdir("debug", "test_sysctl",
+ test_table);
if (!test_sysctl_header) {
kfree(test_data.bitmap_0001);
return -ENOMEM;
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/8] Simplefs: group and simplify linux fs code
On Tue, Apr 14, 2020 at 02:42:54PM +0200, Emanuele Giuseppe Esposito wrote: > This series of patches introduce wrappers for functions, > arguments simplification in functions calls and most importantly > groups duplicated code in a single header, simplefs, to avoid redundancy > in the linux fs, especially debugfs and tracefs. The general goal seems worthy, but here I don't see explained why hasn't this gone through libfs, and what the intention was long term. For instance, you added some other generalizations which you have found. It was not clear that this was the goal here, to expand on these paths. What if common code on fs is found which are not part of debugfs and tracefs, how does one decide if to move to libfs or simplefs? Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/8] fs: extract simple_pin/release_fs to separate files
On Tue, Apr 14, 2020 at 02:42:56PM +0200, Emanuele Giuseppe Esposito wrote: > We will augment this family of functions with inode management. To avoid > littering include/linux/fs.h and fs/libfs.c, move them to a separate header, > with a Kconfig symbol to enable them. If there are no functional changes, indicating that on the commit log will make the review much easier. > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index d1398cef3b18..fc38a6f0fc11 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -288,12 +288,16 @@ config STRIP_ASM_SYMS > > config READABLE_ASM > bool "Generate readable assembler code" > - depends on DEBUG_KERNEL > - help > - Disable some compiler optimizations that tend to generate human > unreadable > - assembler output. This may make the kernel slightly slower, but it > helps > - to keep kernel developers who have to stare a lot at assembler > listings > - sane. > +depends on DEBUG_KERNEL > +help > + Disable some compiler optimizations that tend to generate human > unreadable > + assembler output. This may make the kernel slightly slower, but it > helps > + to keep kernel developers who have to stare a lot at assembler listings > + sane. > + This minor change above should just be a separate patch. Its just noise otherwise. > +config DEBUG_FS > + bool "Debug Filesystem" > + select SIMPLEFS I'm at a loss reviewing this, my lib/Kconfig.debug already has a config DEBUG_FS. But above I see it is being added for the very first time. I'm sure there is some odd conditional which is obscuring this, can this be explained in the commit log? Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/8] apparmor: just use vfs_kern_mount to make .null
On Tue, Apr 14, 2020 at 02:42:55PM +0200, Emanuele Giuseppe Esposito wrote:
> aa_mk_null_file is using simple_pin_fs/simple_release_fs with local
> variables as arguments, for what would amount to a simple
> vfs_kern_mount/mntput pair if everything was inlined. Just use
> the normal filesystem API since the reference counting is not needed
> here.
*Why* is refcounting not needed here?
Luis
>
> Signed-off-by: Emanuele Giuseppe Esposito
> ---
> security/apparmor/apparmorfs.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 280741fc0f5f..828bb1eb77ea 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -2525,14 +2525,14 @@ struct path aa_null;
>
> static int aa_mk_null_file(struct dentry *parent)
> {
> - struct vfsmount *mount = NULL;
> + struct file_system_type *type = parent->d_sb->s_type;
> + struct vfsmount *mount;
> struct dentry *dentry;
> struct inode *inode;
> - int count = 0;
> - int error = simple_pin_fs(parent->d_sb->s_type, , );
>
> - if (error)
> - return error;
> + mount = vfs_kern_mount(type, SB_KERNMOUNT, type->name, NULL);
> + if (IS_ERR(mount))
> + return PTR_ERR(mount);
>
> inode_lock(d_inode(parent));
> dentry = lookup_one_len(NULL_FILE_NAME, parent, strlen(NULL_FILE_NAME));
> @@ -2561,7 +2561,7 @@ static int aa_mk_null_file(struct dentry *parent)
> dput(dentry);
> out:
> inode_unlock(d_inode(parent));
> - simple_release_fs(, );
> + mntput(mount);
> return error;
> }
>
> --
> 2.25.2
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] video: fbdev: atyfb: only use ioremap_uc() on i386 and ia64
On Wed, Nov 13, 2019 at 11:31:54AM +0200, Andy Shevchenko wrote: > On Wed, Nov 13, 2019 at 08:38:15AM +0100, Arnd Bergmann wrote: > > On Wed, Nov 13, 2019 at 8:27 AM Christoph Hellwig wrote: > > > > > > On Tue, Nov 12, 2019 at 10:24:23PM +, Luis Chamberlain wrote: > > > > I think this would be possible if we could flop ioremap_nocache() to UC > > > > instead of UC- on x86. Otherwise, I can't see how we can remove this by > > > > still not allowing direct MTRR calls. > > > > > > If everything goes well ioremap_nocache will be gone as of 5.5. > > > > As ioremap_nocache() just an alias for ioremap(), I suppose the idea would > > then be to make x86 ioremap be UC instead of UC-, again matching what the > > other architectures do already. > > I think it's right thing to do, i.e. assume that ioremap() always does strong > UC independently on MTRR settings. Agreed wholeheartedly. What are the blockers from making that happen? Do we have any left? Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] video: fbdev: atyfb: only use ioremap_uc() on i386 and ia64
On Tue, Nov 12, 2019 at 03:26:35PM +0100, Daniel Vetter wrote: > On Tue, Nov 12, 2019 at 3:06 PM Christoph Hellwig wrote: > > On Tue, Nov 12, 2019 at 02:04:16PM +0100, Daniel Vetter wrote: > > > Wut ... Maybe I'm missing something, but from how we use mtrr in other > > > gpu drivers it's a) either you use MTRR because that's all you got or > > > b) you use pat. Mixing both sounds like a pretty bad idea, since if > > > you need MTRR for performance (because you dont have PAT) then you > > > can't fix the wc with the PAT-based ioremap_uc. And if you have PAT, > > > then you don't really need an MTRR to get wc. > > > > > > So I'd revert this patch from Luis and ... > > > > Sounds great to me.. > > > > > ... apply this one. Since the same reasoning should apply to anything > > > that's running on any cpu with PAT. > > > > Can you take a look at "mfd: intel-lpss: Use devm_ioremap_uc for MMIO" > > in linux-next, which also looks rather fishy to me? Can't we use > > the MTRR APIs to override the broken BIOS MTRR setup there as well? > > Hm so that's way out of my knowledge, but I think mtrr_cleanup() was > supposed to fix up messy/broken MTRR setups by the bios. So maybe they > simply didn't enable that in their .config with CONFIG_MTRR_SANITIZER. I had originally suggested to just make the driver build on x86, but an atlternative was to provide the call for the missing architecture. > An explicit cleanup is currently not possible for drivers, since the > only interface exported to drivers is arch_phys_wc_add/del (which > short-circuits if pat works since you don't need mtrr in that case). Right, the goal was to not call MTRR directly. > Adding everyone from that commit, plus Luis. Drivers really shouldn't > assume/work around the bios setting up superflous/wrong MTRR. Such things are needed, otherwise some systems may not boot... > > With that we could kill ioremap_uc entirely. > > So yeah removing that seems definitely like the right thing. I think this would be possible if we could flop ioremap_nocache() to UC instead of UC- on x86. Otherwise, I can't see how we can remove this by still not allowing direct MTRR calls. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] video: fbdev: atyfb: only use ioremap_uc() on i386 and ia64
On Tue, Nov 12, 2019 at 03:06:31PM +0100, Christoph Hellwig wrote: > On Tue, Nov 12, 2019 at 02:04:16PM +0100, Daniel Vetter wrote: > > Wut ... Maybe I'm missing something, but from how we use mtrr in other > > gpu drivers it's a) either you use MTRR because that's all you got or > > b) you use pat. Mixing both sounds like a pretty bad idea, You misread the patch. And indeed there is a bit of complexity involved here folks should be aware of as .. well, its been a while. A mix of both MTRR and PAT is not effectively done on the code patch for the atyb driver. If you have PAT only PAT is used. If you don't have PAT a solution is provided to use MTRR. The goal of the patch really was to help finally avoid direct calls to MTRR. *This* driver was the *one* crazy exception where we needed to adddress this with a solution which would work effectively for both non-PAT and PAT world which had crazy constraints. So with this out of the way, no direct calls of MTRRs was possible and there are future possible gains with this for x86. The biggest two were: 1) Xen didn't have to implement MTRR hypervisor calls out for Linux guests. This means Xen guests don't have to enable MTRRs. Any code path avoiding such craziness as stop_machine() on each CPU during bootup, resume, CPU online and whenever an MTRR is set is a blessing. 2) We may be closer in the future to getting ioremap_nocache to use UC isntead of UC-, this would be a win. x86 ioremap_nocache() does not use UC (strong UC), it just uses UC-. Note though that BIOSes can *only* enable UC by using MTRR directly, fan control for a system was one use case example that can come up, just as an example. Ideally your BIOS won't need this. When and how this is done is platform and BIOS specific though. So effectively, if a BIOS enables MTRRs the Linux must keep them enabled. If the BIOS disables MTRRs the kernel keeps them disabled. > Can you take a look at "mfd: intel-lpss: Use devm_ioremap_uc for MMIO" > in linux-next, which also looks rather fishy to me? Can't we use > the MTRR APIs to override the broken BIOS MTRR setup there as well? The call there was put to allow precisely for such work around but also allow the code to work on PAT / non-PAT systems by using the same API. > With that we could kill ioremap_uc entirely. ioremap_uc() is a compromise to avoid direct calls to MTRRs, since ioremap_nocache() is not effectively yet using UC. Whether or not other archs carry it. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 18/18] MAINTAINERS: add proc sysctl KUnit test to PROC SYSCTL section
On Wed, Jul 03, 2019 at 05:36:15PM -0700, Brendan Higgins wrote: > Add entry for the new proc sysctl KUnit test to the PROC SYSCTL section. > > Signed-off-by: Brendan Higgins > Reviewed-by: Greg Kroah-Hartman > Reviewed-by: Logan Gunthorpe > Acked-by: Luis Chamberlain Come to think of it, I'd welcome Iurii to be added as a maintainer, with the hope Iurii would be up to review only the kunit changes. Of course if Iurii would be up to also help review future proc changes, even better. 3 pair of eyeballs is better than 2 pairs. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
On Wed, Jul 03, 2019 at 05:36:14PM -0700, Brendan Higgins wrote: > From: Iurii Zaikin > > KUnit tests for initialized data behavior of proc_dointvec that is > explicitly checked in the code. Includes basic parsing tests including > int min/max overflow. > > Signed-off-by: Iurii Zaikin > Signed-off-by: Brendan Higgins > Reviewed-by: Greg Kroah-Hartman > Reviewed-by: Logan Gunthorpe Acked-by: Luis Chamberlain Luis
Re: [PATCH v6 02/18] kunit: test: add test resource management API
On Wed, Jul 03, 2019 at 05:35:59PM -0700, Brendan Higgins wrote:
> diff --git a/kunit/test.c b/kunit/test.c
> index c030ba5a43e40..a70fbe449e922 100644
> --- a/kunit/test.c
> +++ b/kunit/test.c
> @@ -122,7 +122,8 @@ static void kunit_print_test_case_ok_not_ok(struct
> kunit_case *test_case,
>
> void kunit_init_test(struct kunit *test, const char *name)
> {
> - spin_lock_init(>lock);
Once you re-spin, this above line should be removed.
> + mutex_init(>lock);
> + INIT_LIST_HEAD(>resources);
> test->name = name;
> test->success = true;
> }
Luis
Re: [PATCH v6 01/18] kunit: test: add KUnit test runner core
On Wed, Jul 03, 2019 at 05:35:58PM -0700, Brendan Higgins wrote:
> +struct kunit {
> + void *priv;
> +
> + /* private: internal use only. */
> + const char *name; /* Read only after initialization! */
> + bool success; /* Read only after test_case finishes! */
> +};
No lock attribute above.
> +void kunit_init_test(struct kunit *test, const char *name)
> +{
> + spin_lock_init(>lock);
> + test->name = name;
> + test->success = true;
> +}
And yet here you initialize a spin lock... This won't compile. Seems
you forgot to remove this line. So I guess a re-spin is better.
Luis
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 01/18] kunit: test: add KUnit test runner core
On Wed, Jul 03, 2019 at 05:35:58PM -0700, Brendan Higgins wrote: > Add core facilities for defining unit tests; this provides a common way > to define test cases, functions that execute code which is under test > and determine whether the code under test behaves as expected; this also > provides a way to group together related test cases in test suites (here > we call them test_modules). > > Just define test cases and how to execute them for now; setting > expectations on code will be defined later. > > Signed-off-by: Brendan Higgins > Reviewed-by: Greg Kroah-Hartman > Reviewed-by: Logan Gunthorpe Reviewed-by: Luis Chamberlain But a nitpick below, I think that can be fixed later with a follow up patch. > +/** > + * struct kunit - represents a running instance of a test. > + * @priv: for user to store arbitrary data. Commonly used to pass data > created > + * in the init function (see kunit_suite). > + * > + * Used to store information about the current context under which the test > is > + * running. Most of this data is private and should only be accessed > indirectly > + * via public functions; the one exception is @priv which can be used by the > + * test writer to store arbitrary data. > + * > + * A brief note on locking: > + * > + * First off, we need to lock because in certain cases a user may want to > use an > + * expectation in a thread other than the thread that the test case is > running > + * in. This as a prefix to the struct without a lock seems odd. It would be clearer I think if you'd explain here what locking mechanism we decided to use and why it suffices today. > +/** > + * suite_test() - used to register a kunit_suite with KUnit. You mean kunit_test_suite()? > + * @suite: a statically allocated kunit_suite. > + * > + * Registers @suite with the test framework. See kunit_suite for more > + * information. > + * > + * NOTE: Currently KUnit tests are all run as late_initcalls; this means that > + * they cannot test anything where tests must run at a different init phase. > One > + * significant restriction resulting from this is that KUnit cannot reliably > + * test anything that is initialize in the late_init phase. initialize prior to the late init phase. That is, this is useless to test things running early. > + * > + * TODO(brendanhigg...@google.com): Don't run all KUnit tests as > late_initcalls. > + * I have some future work planned to dispatch all KUnit tests from the same > + * place, and at the very least to do so after everything else is definitely > + * initialized. TODOs are odd to be adding to documentation, this is just not common place practice. The NOTE should suffice for you. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 07/18] kunit: test: add initial tests
On Tue, Jul 02, 2019 at 10:52:50AM -0700, Brendan Higgins wrote: > On Wed, Jun 26, 2019 at 12:53 AM Brendan Higgins > wrote: > > > > On Tue, Jun 25, 2019 at 4:22 PM Luis Chamberlain wrote: > > > > > > On Mon, Jun 17, 2019 at 01:26:02AM -0700, Brendan Higgins wrote: > > > > diff --git a/kunit/example-test.c b/kunit/example-test.c > > > > new file mode 100644 > > > > index 0..f44b8ece488bb > > > > --- /dev/null > > > > +++ b/kunit/example-test.c > > > > > > <-- snip --> > > > > > > > +/* > > > > + * This defines a suite or grouping of tests. > > > > + * > > > > + * Test cases are defined as belonging to the suite by adding them to > > > > + * `kunit_cases`. > > > > + * > > > > + * Often it is desirable to run some function which will set up things > > > > which > > > > + * will be used by every test; this is accomplished with an `init` > > > > function > > > > + * which runs before each test case is invoked. Similarly, an `exit` > > > > function > > > > + * may be specified which runs after every test case and can be used > > > > to for > > > > + * cleanup. For clarity, running tests in a test module would behave > > > > as follows: > > > > + * > > > > > > To be clear this is not the kernel module init, but rather the kunit > > > module init. I think using kmodule would make this clearer to a reader. > > > > Seems reasonable. Will fix in next revision. > > > > > > + * module.init(test); > > > > + * module.test_case[0](test); > > > > + * module.exit(test); > > > > + * module.init(test); > > > > + * module.test_case[1](test); > > > > + * module.exit(test); > > > > + * ...; > > > > + */ > > Do you think it might be clearer yet to rename `struct kunit_module > *module;` to `struct kunit_suite *suite;`? Yes. Definitely. Or struct kunit_test. Up to you. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
On Fri, Jun 28, 2019 at 01:01:54AM -0700, Brendan Higgins wrote:
> On Wed, Jun 26, 2019 at 11:10 PM Luis Chamberlain wrote:
> >
> > On Wed, Jun 26, 2019 at 09:07:43PM -0700, Iurii Zaikin wrote:
> > > On Tue, Jun 25, 2019 at 7:17 PM Luis Chamberlain
> > > wrote:
> > > > > +static void sysctl_test_dointvec_table_maxlen_unset(struct kunit
> > > > > *test)
> > > > > +{
> > > > > + struct ctl_table table = {
> > > > > + .procname = "foo",
> > > > > + .data = _data.int_0001,
> > > > > + .maxlen = 0,
> > > > > + .mode = 0644,
> > > > > + .proc_handler = proc_dointvec,
> > > > > + .extra1 = _zero,
> > > > > + .extra2 = _one_hundred,
> > > > > + };
> > > > > + void *buffer = kunit_kzalloc(test, sizeof(int), GFP_USER);
> > > > > + size_t len;
> > > > > + loff_t pos;
> > > > > +
> > > > > + len = 1234;
> > > > > + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(, 0, buffer, ,
> > > > > ));
> > > > > + KUNIT_EXPECT_EQ(test, (size_t)0, len);
> > > > > + len = 1234;
> > > > > + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(, 1, buffer, ,
> > > > > ));
> > > > > + KUNIT_EXPECT_EQ(test, (size_t)0, len);
> > > > > +}
> > > >
> > > > In a way this is also testing for general kernel API changes. This is
> > > > and the
> > > > last one were good examples. So this is not just testing functionality
> > > > here. There is no wrong or write answer if 0 or -EINVAL was returned
> > > > other than the fact that we have been doing this for years.
> > > >
> > > > Its a perhaps small but important difference for some of these tests. I
> > > > *do* think its worth clarifying through documentation which ones are
> > > > testing for API consistency Vs proper correctness.
> > >
> > > You make a good point that the test codifies the existing behavior of
> > > the function in lieu of formal documentation. However, the test cases
> > > were derived from examining the source code of the function under test
> > > and attempting to cover all branches. The assertions were added only
> > > for the values that appeared to be set deliberately in the
> > > implementation. And it makes sense to me to test that the code does
> > > exactly what the implementation author intended.
> >
> > I'm not arguing against adding them. I'm suggesting that it is different
> > to test for API than for correctness of intended functionality, and
> > it would be wise to make it clear which test cases are for API and which
> > for correctness.
>
> I see later on that some of the API stuff you are talking about is
> public APIs from the standpoint of user (outside of LInux) visible.
Right, UAPI.
> To
> be clear, is that what you mean by public APIs throughout, or would
> you distinguish between correctness tests, internal API tests, and
> external API tests?
I would definitely recommend distingishing between all of these.
Kernel API (or just call it API), UAPI, and correctness.
> > This will come up later for other kunit tests and it would be great
> > to set precendent so that other kunit tests can follow similar
> > practices to ensure its clear what is API realted Vs correctness of
> > intended functionality.
> >
> > In fact, I'm not yet sure if its possible to test public kernel API to
> > userspace with kunit, but if it is possible... well, that could make
> > linux-api folks happy as they could enable us to codify interpreation of
> > what is expected into kunit test cases, and we'd ensure that the
> > codified interpretation is not only documented in man pages but also
> > through formal kunit test cases.
> >
> > A regression in linux-api then could be formalized through a proper
> > kunit tests case. And if an API evolves, it would force developers to
> > update the respective kunit which codifies that contract.
>
> Yep, I think that is long term hope. Some of the file system interface
> stuff that requires a filesystem to be mounted somewhere might get a
> little weird/difficult, but I suspect we should be able to do it
> eventually. I mean it's all just C code right? Should mostly boil down
> to someone figuring out how to do it the first time.
There used to be hacks in the kernel the call syscalls in a few places.
This was cleaned up and addressed via Dominik Brodowski's series last
year in March:
http://lkml.kernel.org/r/20180325162527.ga17...@light.dominikbrodowski.net
An example commit: d300b610812f3 ("kernel: use kernel_wait4() instead of
sys_wait4()").
So it would seem the work is done, and you'd just have to use the
respective exposed kernel_syscallname() calls, or add some if you
want to test a specific syscall in kernel space.
Luis
Re: [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
On Wed, Jun 26, 2019 at 09:07:43PM -0700, Iurii Zaikin wrote:
> On Tue, Jun 25, 2019 at 7:17 PM Luis Chamberlain wrote:
> > > +static void sysctl_test_dointvec_table_maxlen_unset(struct kunit *test)
> > > +{
> > > + struct ctl_table table = {
> > > + .procname = "foo",
> > > + .data = _data.int_0001,
> > > + .maxlen = 0,
> > > + .mode = 0644,
> > > + .proc_handler = proc_dointvec,
> > > + .extra1 = _zero,
> > > + .extra2 = _one_hundred,
> > > + };
> > > + void *buffer = kunit_kzalloc(test, sizeof(int), GFP_USER);
> > > + size_t len;
> > > + loff_t pos;
> > > +
> > > + len = 1234;
> > > + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(, 0, buffer, ,
> > > ));
> > > + KUNIT_EXPECT_EQ(test, (size_t)0, len);
> > > + len = 1234;
> > > + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(, 1, buffer, ,
> > > ));
> > > + KUNIT_EXPECT_EQ(test, (size_t)0, len);
> > > +}
> >
> > In a way this is also testing for general kernel API changes. This is and
> > the
> > last one were good examples. So this is not just testing functionality
> > here. There is no wrong or write answer if 0 or -EINVAL was returned
> > other than the fact that we have been doing this for years.
> >
> > Its a perhaps small but important difference for some of these tests. I
> > *do* think its worth clarifying through documentation which ones are
> > testing for API consistency Vs proper correctness.
>
> You make a good point that the test codifies the existing behavior of
> the function in lieu of formal documentation. However, the test cases
> were derived from examining the source code of the function under test
> and attempting to cover all branches. The assertions were added only
> for the values that appeared to be set deliberately in the
> implementation. And it makes sense to me to test that the code does
> exactly what the implementation author intended.
I'm not arguing against adding them. I'm suggesting that it is different
to test for API than for correctness of intended functionality, and
it would be wise to make it clear which test cases are for API and which
for correctness.
This will come up later for other kunit tests and it would be great
to set precendent so that other kunit tests can follow similar
practices to ensure its clear what is API realted Vs correctness of
intended functionality.
In fact, I'm not yet sure if its possible to test public kernel API to
userspace with kunit, but if it is possible... well, that could make
linux-api folks happy as they could enable us to codify interpreation of
what is expected into kunit test cases, and we'd ensure that the
codified interpretation is not only documented in man pages but also
through formal kunit test cases.
A regression in linux-api then could be formalized through a proper
kunit tests case. And if an API evolves, it would force developers to
update the respective kunit which codifies that contract.
> > > +static void sysctl_test_dointvec_single_less_int_min(struct kunit *test)
> > > +{
> > > + struct ctl_table table = {
> > > + .procname = "foo",
> > > + .data = _data.int_0001,
> > > + .maxlen = sizeof(int),
> > > + .mode = 0644,
> > > + .proc_handler = proc_dointvec,
> > > + .extra1 = _zero,
> > > + .extra2 = _one_hundred,
> > > + };
> > > + char input[32];
> > > + size_t len = sizeof(input) - 1;
> > > + loff_t pos = 0;
> > > + unsigned long abs_of_less_than_min = (unsigned long)INT_MAX
> > > + - (INT_MAX + INT_MIN) + 1;
> > > +
> > > + KUNIT_EXPECT_LT(test,
> > > + (size_t)snprintf(input, sizeof(input), "-%lu",
> > > + abs_of_less_than_min),
> > > + sizeof(input));
> > > +
> > > + table.data = kunit_kzalloc(test, sizeof(int), GFP_USER);
> > > + KUNIT_EXPECT_EQ(test, -EINVAL,
> > > + proc_dointvec(, 1, input, , ));
> > > + KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len);
> > > + KUNIT_EXPECT_EQ(test, 0, ((int *)table.data)[0]);
> > > +}
> >
> > API test.
> >
> Not sure why.
Because you are codifying tha
Re: [PATCH v5 13/18] kunit: tool: add Python wrappers for running KUnit tests
On Wed, Jun 26, 2019 at 01:02:55AM -0700, Brendan Higgins wrote: > On Tue, Jun 25, 2019 at 5:01 PM Luis Chamberlain wrote: > > > > On Mon, Jun 17, 2019 at 01:26:08AM -0700, Brendan Higgins wrote: > > > create mode 100644 > > > tools/testing/kunit/test_data/test_is_test_passed-all_passed.log > > > create mode 100644 > > > tools/testing/kunit/test_data/test_is_test_passed-crash.log > > > create mode 100644 > > > tools/testing/kunit/test_data/test_is_test_passed-failure.log > > > create mode 100644 > > > tools/testing/kunit/test_data/test_is_test_passed-no_tests_run.log > > > create mode 100644 > > > tools/testing/kunit/test_data/test_output_isolated_correctly.log > > > create mode 100644 > > > tools/testing/kunit/test_data/test_read_from_file.kconfig > > > > Why are these being added upstream? The commit log does not explain > > this. > > Oh sorry, those are for testing purposes. I thought that was clear > from being in the test_data directory. I will reference it in the > commit log in the next revision. Still, I don't get it. They seem to be results from a prior run. Why do we need them for testing purposes? Luis
Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
On Tue, Jun 25, 2019 at 11:41:47PM -0700, Brendan Higgins wrote: > On Tue, Jun 25, 2019 at 4:02 PM Luis Chamberlain wrote: > > > > On Tue, Jun 25, 2019 at 03:14:45PM -0700, Brendan Higgins wrote: > > > On Tue, Jun 25, 2019 at 2:44 PM Luis Chamberlain > > > wrote: > > > > Since its a new architecture and since you seem to imply most tests > > > > don't require locking or even IRQs disabled, I think its worth to > > > > consider the impact of adding such extreme locking requirements for > > > > an initial ramp up. > > > > > > Fair enough, I can see the point of not wanting to use irq disabled > > > until we get someone complaining about it, but I think making it > > > thread safe is reasonable. It means there is one less thing to confuse > > > a KUnit user and the only penalty paid is some very minor performance. > > > > One reason I'm really excited about kunit is speed... so by all means I > > think we're at a good point to analyze performance optimizationsm if > > they do make sense. > > Yeah, but I think there are much lower hanging fruit than this (as you > point out below). I am all for making/keeping KUnit super fast, but I > also don't want to waste time with premature optimizations and I think > having thread safe expectations and non-thread safe expectations hurts > usability. > > Still, I am on board with making this a mutex instead of a spinlock for now. > > > While on the topic of parallization, what about support for running > > different test cases in parallel? Or at the very least different kunit > > modules in parallel. Few questions come up based on this prospect: > > > > * Why not support parallelism from the start? > > Just because it is more work and there isn't much to gain from it right now. > > Some numbers: > I currently have a collection of 86 test cases in the branch that this > patchset is from. Impressive, imagine 86 tests and kunit is not even *merged yet*. > I turned on PRINTK_TIME and looked at the first > KUnit output and the last. On UML, start time was 0.09, and end > time was 0.09. Looks like sched_clock is not very good on UML. Since you have a python thing that kicks tests, what if you just run time on it? > Still it seems quite likely that all of these tests run around 0.01 > seconds or less on UML: I ran KUnit with only 2 test cases enabled > three times and got an average runtime of 1.55867 seconds with a > standard deviation of 0.0346747. I then ran it another three times > with all test cases enabled and got an average runtime of 1.535 > seconds with a standard deviation of 0.0150997. The second average is > less, but that doesn't really mean anything because it is well within > one standard deviation with a very small sample size. Nevertheless, we > can conclude that the actual runtime of those 84 test cases is most > likely within one standard deviation, so on the order of 0.01 seconds. > > On x86 running on QEMU, first message from KUnit was printed at > 0.194251 and the last KUnit message was printed at 0.340915, meaning > that all 86 test cases ran in about 0.146664 seconds. Pretty impressive numbers. But can you blame me for expressing the desire to possibly being able to do better? I am not saying -- let's definitely have parallelism in place *now*. Just wanted to hear out tangibles of why we *don't* want it now. And.. also, since we are reviewing now, try to target so that the code can later likely get a face lift to support parallelism without requiring much changes. > In any case, running KUnit tests in parallel is definitely something I > plan on adding it eventually, but it just doesn't really seem worth it > right now. Makes sense! > I find the incremental build time of the kernel to > typically be between 3 and 30 seconds, and a clean build to be between > 30 seconds to several minutes, depending on the number of available > cores, so I don't think most users would even notice the amount of > runtime contributed by the actual unit tests until we start getting > into the 1000s of test cases. I don't suspect it will become an issue > until we get into the 10,000s of test cases. I think we are a pretty > long way off from that. All makes sense, and agreed based on the numbers you are providing. Thanks for the details! > > * Are you opposed to eventually having this added? For instance, there is > > enough code on lib/test_kmod.c for batching tons of kthreads each > > one running its own thing for testing purposes which could be used > > as template. > > I am not opposed to adding it eventually at all. I actually plan on > doing so, just not in this patchset. There are
Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
On Tue, Jun 25, 2019 at 05:07:32PM -0700, Brendan Higgins wrote:
> On Tue, Jun 25, 2019 at 3:33 PM Luis Chamberlain wrote:
> >
> > On Mon, Jun 17, 2019 at 01:25:56AM -0700, Brendan Higgins wrote:
> > > +/**
> > > + * module_test() - used to register a kunit_module with KUnit.
> > > + * @module: a statically allocated kunit_module.
> > > + *
> > > + * Registers @module with the test framework. See kunit_module
> > > for more
> > > + * information.
> > > + */
> > > +#define module_test(module) \
> > > + static int module_kunit_init##module(void) \
> > > + { \
> > > + return kunit_run_tests(); \
> > > + } \
> > > + late_initcall(module_kunit_init##module)
> >
> > Becuase late_initcall() is used, if these modules are built-in, this
> > would preclude the ability to test things prior to this part of the
> > kernel under UML or whatever architecture runs the tests. So, this
> > limits the scope of testing. Small detail but the scope whould be
> > documented.
>
> You aren't the first person to complain about this (and I am not sure
> it is the first time you have complained about it). Anyway, I have
> some follow on patches that will improve the late_initcall thing, and
> people seemed okay with discussing the follow on patches as part of a
> subsequent patchset after this gets merged.
>
> I will nevertheless document the restriction until then.
To be clear, I am not complaining about it. I just find it simply
critical to document its limitations, so folks don't try to invest
time and energy on kunit right away for an early init test, if it
cannot support it.
If support for that requires some work, it may be worth mentioning
as well.
> > > +static void kunit_print_tap_version(void)
> > > +{
> > > + if (!kunit_has_printed_tap_version) {
> > > + kunit_printk_emit(LOGLEVEL_INFO, "TAP version 14\n");
> >
> > What is this TAP thing? Why should we care what version it is on?
> > Why are we printing this?
>
> It's part of the TAP specification[1]. Greg and Frank asked me to make
> the intermediate format conform to TAP. Seems like something else I
> should probable document...
Yes thanks!
> > > + kunit_has_printed_tap_version = true;
> > > + }
> > > +}
> > > +
> > > +static size_t kunit_test_cases_len(struct kunit_case *test_cases)
> > > +{
> > > + struct kunit_case *test_case;
> > > + size_t len = 0;
> > > +
> > > + for (test_case = test_cases; test_case->run_case; test_case++)
> >
> > If we make the last test case NULL, we'd just check for test_case here,
> > and save ourselves an extra few bytes per test module. Any reason why
> > the last test case cannot be NULL?
>
> Is there anyway to make that work with a statically defined array?
No you're right.
> Basically, I want to be able to do something like:
>
> static struct kunit_case example_test_cases[] = {
> KUNIT_CASE(example_simple_test),
> KUNIT_CASE(example_mock_test),
> {}
> };
>
> FYI,
> #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
>
> In order to do what you are proposing, I think I need an array of
> pointers to test cases, which is not ideal.
Yeah, you're right. The only other alternative is to have a:
struct kunit_module {
const char name[256];
int (*init)(struct kunit *test);
void (*exit)(struct kunit *test);
struct kunit_case *test_cases;
+ unsigned int num_cases;
};
And then something like:
#define KUNIT_MODULE(name, init, exit, cases) { \
.name = name, \
.init = init, \
.exit = exit, \
.test_cases = cases,
num_cases = ARRAY_SIZE(cases), \
}
Let's evaluate which is better: one extra test case per all test cases, or
an extra unsigned int for each kunit module.
Luis
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 00/18] kunit: introduce KUnit, the Linux kernel unit testing framework
On Wed, Jun 19, 2019 at 06:17:51PM -0700, Frank Rowand wrote: > It does not matter whether KUnit provides additional things, relative > to kselftest. The point I was making is that there appears to be > _some_ overlap between kselftest and KUnit, and if there is overlap > then it is worth considering whether the overlap can be unified instead > of duplicated. From my experience as an author of several kselftests drivers, one faily complex, and after reviewing the sysctl kunit test module, I disagree with this. Even if there were an overlap, I'd say let the best test harness win. Just as we have different LSMs that do something similar. But this is not about that though. Although both are testing code, they do so in *very* different ways. The design philosophy and architecture are fundamentally different. The *only* thing I can think of where there is overlap is that both can test similar code paths. Beyond that, the layout of how one itemizes tests could be borrowed, but that would be up to each kselftest author to decide, in fact some ksefltests do exist which follow similar pattern of itemizing test cases and running them. Kunit just provides a proper framework to do this easily but also with a focus on UML. This last aspect makes kselftests fundamentally orthogonal from an architecture / design perspective. After careful review, I cannot personally identify what could be shared at this point. Can you? If you did identify one part, how do you recommend to share it? Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 18/18] MAINTAINERS: add proc sysctl KUnit test to PROC SYSCTL section
On Mon, Jun 17, 2019 at 01:26:13AM -0700, Brendan Higgins wrote: > Add entry for the new proc sysctl KUnit test to the PROC SYSCTL section. > > Signed-off-by: Brendan Higgins > Reviewed-by: Greg Kroah-Hartman > Reviewed-by: Logan Gunthorpe Acked-by: Luis Chamberlain Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
On Mon, Jun 17, 2019 at 01:26:12AM -0700, Brendan Higgins wrote:
> From: Iurii Zaikin
>
> KUnit tests for initialized data behavior of proc_dointvec that is
> explicitly checked in the code. Includes basic parsing tests including
> int min/max overflow.
First, thanks for this work! My review below.
>
> Signed-off-by: Iurii Zaikin
> Signed-off-by: Brendan Higgins
> Reviewed-by: Greg Kroah-Hartman
> Reviewed-by: Logan Gunthorpe
> ---
> Changes Since Last Revision:
> - Iurii did some clean up (thanks Iurii!) as suggested by Stephen Boyd.
> ---
> kernel/Makefile | 2 +
> kernel/sysctl-test.c | 242 +++
> lib/Kconfig.debug| 10 ++
> 3 files changed, 254 insertions(+)
> create mode 100644 kernel/sysctl-test.c
>
> diff --git a/kernel/Makefile b/kernel/Makefile
> index a8d923b5481ba..50fd511cd0ee0 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -114,6 +114,8 @@ obj-$(CONFIG_HAS_IOMEM) += iomem.o
> obj-$(CONFIG_ZONE_DEVICE) += memremap.o
> obj-$(CONFIG_RSEQ) += rseq.o
>
> +obj-$(CONFIG_SYSCTL_KUNIT_TEST) += sysctl-test.o
And we have lib/test_sysctl.c of selftests.
I'm fine with this going in as-is to its current place, but if we have
to learn from selftests I'd say we try to stick to a convention so
folks know what framework a test is for, and to ensure folks can
easily tell if its test code or not.
Perhaps simply a directory for kunit tests would suffice alone.
> +
> obj-$(CONFIG_GCC_PLUGIN_STACKLEAK) += stackleak.o
> KASAN_SANITIZE_stackleak.o := n
> KCOV_INSTRUMENT_stackleak.o := n
> diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
> new file mode 100644
> index 0..cb61ad3c7db63
> --- /dev/null
> +++ b/kernel/sysctl-test.c
> @@ -0,0 +1,242 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit test of proc sysctl.
> + */
> +
> +#include
> +#include
> +
> +static int i_zero;
> +static int i_one_hundred = 100;
> +
> +struct test_sysctl_data {
> + int int_0001;
> + int int_0002;
> + int int_0003[4];
> +
> + unsigned int uint_0001;
> +
> + char string_0001[65];
> +};
> +
> +static struct test_sysctl_data test_data = {
> + .int_0001 = 60,
> + .int_0002 = 1,
> +
> + .int_0003[0] = 0,
> + .int_0003[1] = 1,
> + .int_0003[2] = 2,
> + .int_0003[3] = 3,
> +
> + .uint_0001 = 314,
> +
> + .string_0001 = "(none)",
> +};
> +
> +static void sysctl_test_dointvec_null_tbl_data(struct kunit *test)
> +{
> + struct ctl_table table = {
> + .procname = "foo",
> + .data = NULL,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + .extra1 = _zero,
> + .extra2 = _one_hundred,
> + };
> + void *buffer = kunit_kzalloc(test, sizeof(int), GFP_USER);
> + size_t len;
> + loff_t pos;
> +
> + len = 1234;
> + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(, 0, buffer, , ));
> + KUNIT_EXPECT_EQ(test, (size_t)0, len);
It is a bit odd, but it does happen, for a developer to be calling
proc_dointvec() directly, instead typically folks just register a table
and let it do its thing. That said, someone not too familiar with proc
code would see this and really have no clue exactly what is being
tested.
Even as a maintainer, I had to read the code for proc_dointvec() a bit
to understand that the above is a *read* attempt to the .data field
being allocated. Because its a write, the len set to a bogus does not
matter as we are expecting the proc_dointvec() to update len for us.
If a test fails, it would be good to for anyone to easily grasp what is
being tested. So... a few words documenting each test case would be nice.
> + len = 1234;
> + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(, 1, buffer, , ));
> + KUNIT_EXPECT_EQ(test, (size_t)0, len);
And this is a write...
A nice tests given the data on the table allocated is not assigned.
I don't see any other areas in the kernel where we open code a
proc_dointvec() call where the second argument is a digit, it
always is with a variable. As such there would be no need for
us to expose helpers to make it clear if one is a read or write.
But for *this* case, I think it would be useful to add two wrappers
inside this kunit test module which sprinkles the 0 or 1, this way
a reader can easily know what mode is being tested.
kunit_proc_dointvec_read()
kunit_proc_dointvec_write()
Or just use #define KUNIT_PROC_READ 0, #define KUNIT_PROC_WRITE 1.
Whatever makes this code more legible.
> +}
> +
> +static void sysctl_test_dointvec_table_maxlen_unset(struct kunit *test)
> +{
> + struct ctl_table table = {
> + .procname = "foo",
> + .data = _data.int_0001,
> + .maxlen = 0,
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + .extra1 = _zero,
> +
Re: [PATCH v5 13/18] kunit: tool: add Python wrappers for running KUnit tests
On Mon, Jun 17, 2019 at 01:26:08AM -0700, Brendan Higgins wrote: > create mode 100644 > tools/testing/kunit/test_data/test_is_test_passed-all_passed.log > create mode 100644 > tools/testing/kunit/test_data/test_is_test_passed-crash.log > create mode 100644 > tools/testing/kunit/test_data/test_is_test_passed-failure.log > create mode 100644 > tools/testing/kunit/test_data/test_is_test_passed-no_tests_run.log > create mode 100644 > tools/testing/kunit/test_data/test_output_isolated_correctly.log > create mode 100644 tools/testing/kunit/test_data/test_read_from_file.kconfig Why are these being added upstream? The commit log does not explain this. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 07/18] kunit: test: add initial tests
On Mon, Jun 17, 2019 at 01:26:02AM -0700, Brendan Higgins wrote: > diff --git a/kunit/example-test.c b/kunit/example-test.c > new file mode 100644 > index 0..f44b8ece488bb > --- /dev/null > +++ b/kunit/example-test.c <-- snip --> > +/* > + * This defines a suite or grouping of tests. > + * > + * Test cases are defined as belonging to the suite by adding them to > + * `kunit_cases`. > + * > + * Often it is desirable to run some function which will set up things which > + * will be used by every test; this is accomplished with an `init` function > + * which runs before each test case is invoked. Similarly, an `exit` function > + * may be specified which runs after every test case and can be used to for > + * cleanup. For clarity, running tests in a test module would behave as > follows: > + * To be clear this is not the kernel module init, but rather the kunit module init. I think using kmodule would make this clearer to a reader. > + * module.init(test); > + * module.test_case[0](test); > + * module.exit(test); > + * module.init(test); > + * module.test_case[1](test); > + * module.exit(test); > + * ...; > + */ Luis
Re: [PATCH v5 06/18] kbuild: enable building KUnit
On Tue, Jun 25, 2019 at 03:41:29PM -0700, Brendan Higgins wrote: > On Tue, Jun 25, 2019 at 3:13 PM Luis Chamberlain wrote: > > > > On Mon, Jun 17, 2019 at 01:26:01AM -0700, Brendan Higgins wrote: > > > diff --git a/Kconfig b/Kconfig > > > index 48a80beab6853..10428501edb78 100644 > > > --- a/Kconfig > > > +++ b/Kconfig > > > @@ -30,3 +30,5 @@ source "crypto/Kconfig" > > > source "lib/Kconfig" > > > > > > source "lib/Kconfig.debug" > > > + > > > +source "kunit/Kconfig" > > > > This patch would break compilation as kunit/Kconfig is not introduced. This > > would would also break bisectability on this commit. This change should > > either be folded in to the next patch, or just be a separate patch after > > the next one. > > Maybe my brain isn't working right now, but I am pretty darn sure that > I introduce kunit/Kconfig in the very first patch of this series. > Quoting from the change summary from the first commit: Indeed, my mistake, thanks! Luis
Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
On Tue, Jun 25, 2019 at 03:14:45PM -0700, Brendan Higgins wrote: > On Tue, Jun 25, 2019 at 2:44 PM Luis Chamberlain wrote: > > Since its a new architecture and since you seem to imply most tests > > don't require locking or even IRQs disabled, I think its worth to > > consider the impact of adding such extreme locking requirements for > > an initial ramp up. > > Fair enough, I can see the point of not wanting to use irq disabled > until we get someone complaining about it, but I think making it > thread safe is reasonable. It means there is one less thing to confuse > a KUnit user and the only penalty paid is some very minor performance. One reason I'm really excited about kunit is speed... so by all means I think we're at a good point to analyze performance optimizationsm if they do make sense. While on the topic of parallization, what about support for running different test cases in parallel? Or at the very least different kunit modules in parallel. Few questions come up based on this prospect: * Why not support parallelism from the start? * Are you opposed to eventually having this added? For instance, there is enough code on lib/test_kmod.c for batching tons of kthreads each one running its own thing for testing purposes which could be used as template. * If we eventually *did* support it: - Would logs be skewed? - Could we have a way to query: give me log for only kunit module named "foo"? Luis
Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
On Mon, Jun 17, 2019 at 01:25:56AM -0700, Brendan Higgins wrote:
> +/**
> + * module_test() - used to register a kunit_module with KUnit.
> + * @module: a statically allocated kunit_module.
> + *
> + * Registers @module with the test framework. See kunit_module for
> more
> + * information.
> + */
> +#define module_test(module) \
> + static int module_kunit_init##module(void) \
> + { \
> + return kunit_run_tests(); \
> + } \
> + late_initcall(module_kunit_init##module)
Becuase late_initcall() is used, if these modules are built-in, this
would preclude the ability to test things prior to this part of the
kernel under UML or whatever architecture runs the tests. So, this
limits the scope of testing. Small detail but the scope whould be
documented.
> +static void kunit_print_tap_version(void)
> +{
> + if (!kunit_has_printed_tap_version) {
> + kunit_printk_emit(LOGLEVEL_INFO, "TAP version 14\n");
What is this TAP thing? Why should we care what version it is on?
Why are we printing this?
> + kunit_has_printed_tap_version = true;
> + }
> +}
> +
> +static size_t kunit_test_cases_len(struct kunit_case *test_cases)
> +{
> + struct kunit_case *test_case;
> + size_t len = 0;
> +
> + for (test_case = test_cases; test_case->run_case; test_case++)
If we make the last test case NULL, we'd just check for test_case here,
and save ourselves an extra few bytes per test module. Any reason why
the last test case cannot be NULL?
> +void kunit_init_test(struct kunit *test, const char *name)
> +{
> + spin_lock_init(>lock);
> + test->name = name;
> + test->success = true;
> +}
> +
> +/*
> + * Performs all logic to run a test case.
> + */
> +static void kunit_run_case(struct kunit_module *module,
> +struct kunit_case *test_case)
> +{
> + struct kunit test;
> + int ret = 0;
> +
> + kunit_init_test(, test_case->name);
> +
> + if (module->init) {
> + ret = module->init();
I believe if we used struct kunit_module *kmodule it would be much
clearer who's init this is.
Luis
Re: [PATCH v5 06/18] kbuild: enable building KUnit
On Mon, Jun 17, 2019 at 01:26:01AM -0700, Brendan Higgins wrote: > diff --git a/Kconfig b/Kconfig > index 48a80beab6853..10428501edb78 100644 > --- a/Kconfig > +++ b/Kconfig > @@ -30,3 +30,5 @@ source "crypto/Kconfig" > source "lib/Kconfig" > > source "lib/Kconfig.debug" > + > +source "kunit/Kconfig" This patch would break compilation as kunit/Kconfig is not introduced. This would would also break bisectability on this commit. This change should either be folded in to the next patch, or just be a separate patch after the next one. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
On Tue, Jun 25, 2019 at 01:28:25PM -0700, Brendan Higgins wrote:
> On Wed, Jun 19, 2019 at 5:15 PM Stephen Boyd wrote:
> >
> > Quoting Brendan Higgins (2019-06-17 01:25:56)
> > > diff --git a/kunit/test.c b/kunit/test.c
> > > new file mode 100644
> > > index 0..d05d254f1521f
> > > --- /dev/null
> > > +++ b/kunit/test.c
> > > @@ -0,0 +1,210 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Base unit test (KUnit) API.
> > > + *
> > > + * Copyright (C) 2019, Google LLC.
> > > + * Author: Brendan Higgins
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +
> > > +static bool kunit_get_success(struct kunit *test)
> > > +{
> > > + unsigned long flags;
> > > + bool success;
> > > +
> > > + spin_lock_irqsave(>lock, flags);
> > > + success = test->success;
> > > + spin_unlock_irqrestore(>lock, flags);
> >
> > I still don't understand the locking scheme in this code. Is the
> > intention to make getter and setter APIs that are "safe" by adding in a
> > spinlock that is held around getting and setting various members in the
> > kunit structure?
>
> Yes, your understanding is correct. It is possible for a user to write
> a test such that certain elements may be updated in different threads;
> this would most likely happen in the case where someone wants to make
> an assertion or an expectation in a thread created by a piece of code
> under test. Although this should generally be avoided, it is possible,
> and there are occasionally good reasons to do so, so it is
> functionality that we should support.
>
> Do you think I should add a comment to this effect?
>
> > In what situation is there more than one thread reading or writing the
> > kunit struct? Isn't it only a single process that is going to be
>
> As I said above, it is possible that the code under test may spawn a
> new thread that may make an expectation or an assertion. It is not a
> super common use case, but it is possible.
I wonder if it is worth to have then different types of tests based on
locking requirements. One with no locking, since it seems you imply
most tests would fall under this category, then locking, and another
with IRQ context.
If no locking is done at all for all tests which do not require locking,
is there any gains at run time? I'm sure it might be minimum but
curious.
> > operating on this structure? And why do we need to disable irqs? Are we
> > expecting to be modifying the unit tests from irq contexts?
>
> There are instances where someone may want to test a driver which has
> an interrupt handler in it. I actually have (not the greatest) example
> here. Now in these cases, I expect someone to use a mock irqchip or
> some other fake mechanism to trigger the interrupt handler and not
> actual hardware; technically speaking in this case, it is not going to
> be accessed from a "real" irq context; however, the code under test
> should think that it is in an irq context; given that, I figured it is
> best to just treat it as a real irq context. Does that make sense?
Since its a new architecture and since you seem to imply most tests
don't require locking or even IRQs disabled, I think its worth to
consider the impact of adding such extreme locking requirements for
an initial ramp up.
Luis
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 14/19] Documentation: kunit: add documentation for KUnit
On Wed, Feb 13, 2019 at 04:17:13PM -0800, Brendan Higgins wrote: > On Wed, Feb 13, 2019 at 1:55 PM Kieran Bingham > wrote: > Oh, yep, you are right. Does that mean we should bother at all with a > defconfig? If one wanted a qemu enabled type of kernel and also for kuniut one could imply run: make kvmconfig make kunitconfig That would get what you suggest above of default "bells and whistles" and keep the kuniut as a fragment. Hm, actually the kvmconfig doesn't really enable the required fragments for qemu, so perhaps one would be good. It would have the serial stuff for instance. > Luis, I know you said you wanted one. I am thinking just stick with > the UML one? The downside there is we then get stuck having to > maintain the fragment and the defconfig. I right now (in the new > revision I am working on) have the Python kunit_tool copy the > defconfig if no kunitconfig is provided and a flag is set. It would be > pretty straightforward to make it merge in the fragment instead. Up to you in the end. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 11/19] kunit: add Python libraries for handing KUnit config and kernel
On Thu, Dec 06, 2018 at 12:32:47PM +, Kieran Bingham wrote: > My main initial idea for a libumlinux is to provide infrastructure such > as our linked-lists and other kernel formatting so that we can take > kernel code directly to userspace for test and debug (assuming that > there are no hardware dependencies or things that we can't mock out) The tools/ directory already does this for a tons of things. Its where I ended up placing some API I tested a long time ago when I wanted to test it in userspace, and provide the unit test in userspace (for my linker table patches). > Now we just have to start the race to see who can tweak the kernel build > system to produce an output library first :) Should be relatively easy if the tools directory used. Yes, there is an inherent risk of duplication, but that was decided long ago. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 11/19] kunit: add Python libraries for handing KUnit config and kernel
On Mon, Dec 03, 2018 at 03:50:48PM -0800, Brendan Higgins wrote: > On Thu, Nov 29, 2018 at 7:44 PM Luis Chamberlain wrote: > > > > On Wed, Nov 28, 2018 at 11:36:28AM -0800, Brendan Higgins wrote: > > > The ultimate goal is to create minimal isolated test binaries; in the > > > meantime we are using UML to provide the infrastructure to run tests, so > > > define an abstract way to configure and run tests that allow us to > > > change the context in which tests are built without affecting the user. > > > This also makes pretty and dynamic error reporting, and a lot of other > > > nice features easier. > > > > > > kunit_config.py: > > > - parse .config and Kconfig files. > > > > > > > > > kunit_kernel.py: provides helper functions to: > > > - configure the kernel using kunitconfig. > > > > We get the tools to run the config stuff, build, etc, but not a top > > level 'make kunitconfig' or whatever. We have things like 'make > > kvmconfig' and 'make xenconfig', I think it would be reasonable to > > add similar for this. > > Are you just asking for a defconfig for KUnit, or are you asking for a > way to run KUnit from make? At least the first. The later seems intrusive as a top level Makefile thing. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 11/19] kunit: add Python libraries for handing KUnit config and kernel
On Mon, Dec 03, 2018 at 03:48:15PM -0800, Brendan Higgins wrote: > On Thu, Nov 29, 2018 at 5:54 AM Kieran Bingham > wrote: > > > > Hi Brendan, > > > > Thanks again for this series! > > > > On 28/11/2018 19:36, Brendan Higgins wrote: > > > The ultimate goal is to create minimal isolated test binaries; in the > > > meantime we are using UML to provide the infrastructure to run tests, so > > > define an abstract way to configure and run tests that allow us to > > > change the context in which tests are built without affecting the user. > > > This also makes pretty and dynamic error reporting, and a lot of other > > > nice features easier. > > > > > > I wonder if we could somehow generate a shared library object > > 'libkernel' or 'libumlinux' from a UM configured set of headers and > > objects so that we could create binary targets directly ? > > That's an interesting idea. I think it would be difficult to figure > out exactly where to draw the line of what goes in there and what > needs to be built specific to a test a priori. Of course, that leads > into the biggest problem in general, needed to know what I need to > build to test the thing that I want to test. > > Nevertheless, I could definitely imagine that being useful in a lot of cases. Whether or not we can abstract away the kernel into such a mechanism with uml libraries is a good question worth exploring. Developers working upstream do modify their kernels a lot, so we'd have to update such libraries quite a bit, but I think that's fine too. The *real* value I think from the above suggestion would be enterprise / mobile distros or stable kernel maintainers which have a static kernel they need to support for a relatively *long time*, consider a 10 year time frame. Running unit tests without qemu with uml and libraries for respective kernels seems real worthy. The overhead for testing a unit test for said targets, *ideally*, would just be to to reboot into the system with such libraries available, a unit test would just look for the respective uname -r library and mimic that kernel, much the same way enterprise distributions today rely on having debugging symbols available to run against crash / gdb. Having debug modules / kernel for crash requires such effort already, so this would just be an extra layer of other prospect tests. All ideaware for now, but the roadmap seems to be paving itself. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 08/19] arch: um: add shim to trap to allow installing a fault catcher for tests
On Mon, Dec 03, 2018 at 03:34:57PM -0800, Brendan Higgins wrote:
> On Thu, Nov 29, 2018 at 7:34 PM Luis Chamberlain wrote:
> >
> > On Wed, Nov 28, 2018 at 11:36:25AM -0800, Brendan Higgins wrote:
> > > diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
> > > index cced829460427..bf90e678b3d71 100644
> > > --- a/arch/um/kernel/trap.c
> > > +++ b/arch/um/kernel/trap.c
> > > @@ -201,6 +201,12 @@ void segv_handler(int sig, struct siginfo
> > > *unused_si, struct uml_pt_regs *regs)
> > > segv(*fi, UPT_IP(regs), UPT_IS_USER(regs), regs);
> > > }
> > >
> > > +static void segv_run_catcher(jmp_buf *catcher, void *fault_addr)
> > > +{
> > > + current->thread.fault_addr = fault_addr;
> > > + UML_LONGJMP(catcher, 1);
> > > +}
> > > +
> > > /*
> > > * We give a *copy* of the faultinfo in the regs to segv.
> > > * This must be done, since nesting SEGVs could overwrite
> > > @@ -219,7 +225,10 @@ unsigned long segv(struct faultinfo fi, unsigned
> > > long ip, int is_user,
> > > if (!is_user && regs)
> > > current->thread.segv_regs = container_of(regs, struct
> > > pt_regs, regs);
> > >
> > > - if (!is_user && (address >= start_vm) && (address < end_vm)) {
> > > + catcher = current->thread.fault_catcher;
> >
> > This and..
> >
> > > + if (catcher && current->thread.is_running_test)
> > > + segv_run_catcher(catcher, (void *) address);
> > > + else if (!is_user && (address >= start_vm) && (address < end_vm)) {
> > > flush_tlb_kernel_vm();
> > > goto out;
> > > }
> >
> > *not this*
>
> I don't understand. Are you saying the previous block of code is good
> and this one is bad?
No, I was saying that the above block of code is a functional change,
but I was also pointing out other areas which were not and could be
folded into a separate atomic patch where no functionality changes.
> > > @@ -246,12 +255,10 @@ unsigned long segv(struct faultinfo fi, unsigned
> > > long ip, int is_user,
> > > address = 0;
> > > }
> > >
> > > - catcher = current->thread.fault_catcher;
> > > if (!err)
> > > goto out;
> > > else if (catcher != NULL) {
> > > - current->thread.fault_addr = (void *) address;
> > > - UML_LONGJMP(catcher, 1);
> > > + segv_run_catcher(catcher, (void *) address);
> > > }
> > > else if (current->thread.fault_addr != NULL)
> > > panic("fault_addr set but no fault catcher");
> >
> > But with this seems one atomic change which should be submitted
> > separately, its just a helper. Think it would make the actual
> > change needed easier to review, ie, your needed changes would
> > be smaller and clearer for what you need.
>
> Are you suggesting that I pull out the bits needed to implement abort
> in the next patch and squash it into this one?
No, I'm suggesting you can probably split this patch in 2, one which
wraps things with no functional changes, and another which adds your
changes.
Luis
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 07/19] kunit: test: add initial tests
On Mon, Dec 03, 2018 at 03:26:26PM -0800, Brendan Higgins wrote: > On Thu, Nov 29, 2018 at 7:40 PM Luis Chamberlain wrote: > > > > On Wed, Nov 28, 2018 at 11:36:24AM -0800, Brendan Higgins wrote: > > > Add a test for string stream along with a simpler example. > > > > > > Signed-off-by: Brendan Higgins > > > --- > > > kunit/Kconfig | 12 ++ > > > kunit/Makefile | 4 ++ > > > kunit/example-test.c | 88 ++ > > > > BTW if you need another more concrete but very simple example I think it > > may be possible to port tools/testing/selftests/sysctl/sysctl.sh + > > lib/test_sysctl.c into a kunit test. Correct me if I'm wrong. > > I think that is pretty doable. I don't know that I want to shoot for > that on the next revision. But I can definitely do it in a later > revision, or a later patchset, unless you would strongly prefer it > now, that is. No rush on my end, just figured I'd mention a simple candidate in case you needed another one to evaluate. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 03/19] kunit: test: add string_stream a std::stream like string builder
On Fri, Nov 30, 2018 at 06:14:17PM -0800, Brendan Higgins wrote: > On Thu, Nov 29, 2018 at 7:29 PM Luis Chamberlain wrote: > > > > On Wed, Nov 28, 2018 at 11:36:20AM -0800, Brendan Higgins wrote: > > > A number of test features need to do pretty complicated string printing > > > where it may not be possible to rely on a single preallocated string > > > with parameters. > > > > > > So provide a library for constructing the string as you go similar to > > > C++'s std::string. > > > > Hrm, what's the potential for such thing actually being eventually > > generically useful for printk folks, I wonder? Petr? > > Are you saying you think this is applicable for other things? Yes. > This doesn't belong here. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 01/19] kunit: test: add KUnit test runner core
On Fri, Nov 30, 2018 at 06:08:36PM -0800, Brendan Higgins wrote:
> On Thu, Nov 29, 2018 at 7:28 PM Luis Chamberlain wrote:
> >
> > > +static void kunit_run_case_internal(struct kunit *test,
> > > + struct kunit_module *module,
> > > + struct kunit_case *test_case)
> > > +{
> > > + int ret;
> > > +
> > > + if (module->init) {
> > > + ret = module->init(test);
> > > + if (ret) {
> > > + kunit_err(test, "failed to initialize: %d", ret);
> > > + kunit_set_success(test, false);
> > > + return;
> > > + }
> > > + }
> > > +
> > > + test_case->run_case(test);
> > > +}
> >
> > <-- snip -->
> >
> > > +static bool kunit_run_case(struct kunit *test,
> > > +struct kunit_module *module,
> > > +struct kunit_case *test_case)
> > > +{
> > > + kunit_set_success(test, true);
> > > +
> > > + kunit_run_case_internal(test, module, test_case);
> > > + kunit_run_case_cleanup(test, module, test_case);
> > > +
> > > + return kunit_get_success(test);
> > > +}
> >
> > So we are running the module->init() for each test case... is that
> > correct? Shouldn't the init run once? Also, typically init calls are
>
> Yep, it's correct. `module->init()` should run once before every test
> case, reason being that the kunit_module serves as a test fixture in
> which each test cases should be run completely independently of every
> other.
Shouldn't the init be test_case specific as well? Right now we just
past the struct kunit, but not the struct kunit_case. I though that
that the struct kunit_case was where we'd customize each specific
test case as we see fit for each test case. If not, how would we
do say, a different type of initialization for a different type of
test (for the same unit)?
> init and exit is supposed to allow code common to all test
> cases to run since it is so common to have dependencies needed for a
> test to be common to every test case.
Sure things in common make sense, however the differntiating aspects
seem important as well on init? Or should the author be doing all
custom specific initializations on run_case() instead?
Luis
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 01/19] kunit: test: add KUnit test runner core
On Wed, Nov 28, 2018 at 11:36:18AM -0800, Brendan Higgins wrote:
> +int kunit_run_tests(struct kunit_module *module)
> +{
> + bool all_passed = true, success;
> + struct kunit_case *test_case;
> + struct kunit test;
> + int ret;
> +
> + ret = kunit_init_test(, module->name);
> + if (ret)
> + return ret;
> +
> + for (test_case = module->test_cases; test_case->run_case; test_case++) {
> + success = kunit_run_case(, module, test_case);
We are running test cases serially, why not address testing
asynchronously, this way tests can also be paralellized when possible,
therefore decreasing test time even further.
Would that mess up the printing/log stuff somehow?
Luis
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 01/19] kunit: test: add KUnit test runner core
On Fri, Nov 30, 2018 at 05:51:11PM -0800, Brendan Higgins wrote:
> On Thu, Nov 29, 2018 at 7:14 PM Luis Chamberlain wrote:
> >
> > On Wed, Nov 28, 2018 at 11:36:18AM -0800, Brendan Higgins wrote:
> > > +#define module_test(module) \
> > > + static int module_kunit_init##module(void) \
> > > + { \
> > > + return kunit_run_tests(); \
> > > + } \
> > > + late_initcall(module_kunit_init##module)
> >
> > Here in lies an assumption that suffices. I'm inclined to believe we
> > need new initcall level here so to ensure we *do* run after all the
> > respective kernels iniut calls. Otherwise we're left at the whims of
> > link order for kunit. For instance if a kunit test relies on frameworks
> > which are also late_initcall() we'd have complete incompatibility with
> > anything linked *after* kunit.
>
> Yep, I have some patches that address this, but I thought this is
> sufficient for the initial patchset (I figured that's the type of
> thing that people will have opinions about so best to get it out of
> the critical path). Do you want me to add those in the next revision?
>
> >
> > > diff --git a/kunit/Kconfig b/kunit/Kconfig
> > > new file mode 100644
> > > index 0..49b44c4f6630a
> > > --- /dev/null
> > > +++ b/kunit/Kconfig
> > > @@ -0,0 +1,17 @@
> > > +#
> > > +# KUnit base configuration
> > > +#
> > > +
> > > +menu "KUnit support"
> > > +
> > > +config KUNIT
> > > + bool "Enable support for unit tests (KUnit)"
> > > + depends on UML
> >
> > Consider using:
> >
> > if UML
> >...
> > endif
> >
> > That allows the depends to be done once.
>
> If you want to eliminate depends, wouldn't it be best to have KUNIT
> depend on whatever it needs, and then do `if KUNIT` below that? That
> seems cleaner over the long term. Anyway, Kees actually asked me to
> change it to the way it is now; I really don't care either way.
Yes, that works better. The idea is to just avoid having to write in
depends on over and over again.
> > I'm a bit conflicted here. This currently depends on UML but yet you
> > noted on RFC v2 that your intention is to liberate kunit from UML and
> > ideally allow unit tests to depend only on userspace. I've addressed
> > tests using both selftests kernels drivers and also re-written kernel
> > APIs to userspace to test there. I think we may need to live with both.
>
> I am not entirely opposed. The greater isolation we can achieve, the
> fewer dependencies, and barriers to setting up test fixtures the
> better. I think the best way to do that in most cases is allowing
> minimal test binaries to be built that have the absolute minimum
> amount of code necessary to test the desired property. That being
> said, integration tests are a thing and drawing a line between them
> and unit tests is not always possible, so supporting other
> architectures might be necessary.
Then lets pave the way for it to be done easily.
> > Then for the UML stuff, I think if we *really* accept that UML will
> > always be a viable option we should probably consider now throwing these
> > things under drivers/platform/uml/. This follows the pattern of arch
> > specific drivers. Whether or not we end up with a complete userspace
> > component independent of UML may implicate having a shared component
> > somewhere else.
>
> Fair enough. What specifically are you suggesting should go in
> `drivers/platform/uml`? Just the bits that are completely tied to UML
> or a concrete architecture?
The bits that are UML specific. As I see it, with the above intention
clarified, kunit is a framework for architectures and UML is supported
first. The code doesn't currently reflect this.
> > Likewise, I realize the goal is to *avoid* using a virtual machine for
> > these tests, but would it in any way make sense to share kunit to be
> > supported for other architectures to allow easier-to-write tests as
> > well?
>
> You are not the first person to ask for this.
>
> For the vast majority of tests, I think we can (and consequently
> should) make them run without any external dependencies. Doing so
> makes it such that someone can run a test without knowing anything
> about it, which allows you to do a lot of things. For one, I, as a
> developer, don't have to hunt down somebody's QEMU patches, or
> whatever. But it also means I, as someone maintaining part of the
> kernel, can make nice test runners and build things like presubmit
> servers on top of them.
>
> Nevertheless, I accept that there are things which are just easier to
> do with hardware or a VM (for integration tests it is necessary).
> Still, I think we need to make sure the vast majority of unit tests do
> not depend on real hardware or a VM.
When possible, sure.
Luis
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 06/19] arch: um: enable running kunit from User Mode Linux
On Fri, Nov 30, 2018 at 08:05:34AM -0600, Rob Herring wrote: > On Thu, Nov 29, 2018 at 9:37 PM Luis Chamberlain wrote: > > > > On Wed, Nov 28, 2018 at 03:26:03PM -0600, Rob Herring wrote: > > > On Wed, Nov 28, 2018 at 1:37 PM Brendan Higgins > > > wrote: > > > > > > > > Make minimum number of changes outside of the KUnit directories for > > > > KUnit to build and run using UML. > > > > > > There's nothing in this patch limiting this to UML. > > > > Not that one, but the abort thing segv thing is, eventually. > > To support other architectures we'd need to make a wrapper to that > > hack which Brendan added, and then allow each os to implement > > its own call, and add an asm-generic helper. > > I've not looked into why this is needed, but can't you make the abort > support optional and arches can select it when they support it. Its why I have asked for it to be properly documented. The patches in no way illustrate *why* such thing is done. And if we are going to potentially have other archs do something similar best to make it explicit. > At > least before, the DT unittests didn't need this to run and shouldn't > depend on it after converting to kunit. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 16/19] arch: um: make UML unflatten device tree when testing
On Wed, Nov 28, 2018 at 11:36:33AM -0800, Brendan Higgins wrote: > Make UML unflatten any present device trees when running KUnit tests. > > Signed-off-by: Brendan Higgins > --- > arch/um/kernel/um_arch.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c > index a818ccef30ca2..bd58ae3bf4148 100644 > --- a/arch/um/kernel/um_arch.c > +++ b/arch/um/kernel/um_arch.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -347,6 +348,9 @@ void __init setup_arch(char **cmdline_p) > read_initrd(); > > paging_init(); > +#if IS_ENABLED(CONFIG_OF_UNITTEST) > + unflatten_device_tree(); > +#endif *Why?* Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 14/19] Documentation: kunit: add documentation for KUnit
On Thu, Nov 29, 2018 at 01:56:37PM +, Kieran Bingham wrote: > Hi Brendan, > > Please excuse the top posting, but I'm replying here as I'm following > the section "Creating a kunitconfig" in Documentation/kunit/start.rst. > > Could the three line kunitconfig file live under say >arch/um/configs/kunit_defconfig? > > So that it's always provided? And could even be extended with tests > which people would expect to be run by default? (say in distributions) Indeed, and then a top level 'make kunitconfig' could use it as well. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 11/19] kunit: add Python libraries for handing KUnit config and kernel
On Wed, Nov 28, 2018 at 11:36:28AM -0800, Brendan Higgins wrote: > The ultimate goal is to create minimal isolated test binaries; in the > meantime we are using UML to provide the infrastructure to run tests, so > define an abstract way to configure and run tests that allow us to > change the context in which tests are built without affecting the user. > This also makes pretty and dynamic error reporting, and a lot of other > nice features easier. > > kunit_config.py: > - parse .config and Kconfig files. > > > kunit_kernel.py: provides helper functions to: > - configure the kernel using kunitconfig. We get the tools to run the config stuff, build, etc, but not a top level 'make kunitconfig' or whatever. We have things like 'make kvmconfig' and 'make xenconfig', I think it would be reasonable to add similar for this. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 08/19] arch: um: add shim to trap to allow installing a fault catcher for tests
On Wed, Nov 28, 2018 at 11:36:25AM -0800, Brendan Higgins wrote:
> +static void segv_run_catcher(jmp_buf *catcher, void *fault_addr)
> +{
> + current->thread.fault_addr = fault_addr;
> + UML_LONGJMP(catcher, 1);
> +}
Some documentation about what this does exactly would be appreciated.
With the goal it may be useful to others wanting to consider support
for other archs -- if that actually ends up being desirable.
Luis
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 07/19] kunit: test: add initial tests
On Wed, Nov 28, 2018 at 11:36:24AM -0800, Brendan Higgins wrote: > Add a test for string stream along with a simpler example. > > Signed-off-by: Brendan Higgins > --- > kunit/Kconfig | 12 ++ > kunit/Makefile | 4 ++ > kunit/example-test.c | 88 ++ BTW if you need another more concrete but very simple example I think it may be possible to port tools/testing/selftests/sysctl/sysctl.sh + lib/test_sysctl.c into a kunit test. Correct me if I'm wrong. I think that would show the differences clearly between selftests and kunit as well. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 06/19] arch: um: enable running kunit from User Mode Linux
On Wed, Nov 28, 2018 at 03:26:03PM -0600, Rob Herring wrote: > On Wed, Nov 28, 2018 at 1:37 PM Brendan Higgins > wrote: > > > > Make minimum number of changes outside of the KUnit directories for > > KUnit to build and run using UML. > > There's nothing in this patch limiting this to UML. Not that one, but the abort thing segv thing is, eventually. To support other architectures we'd need to make a wrapper to that hack which Brendan added, and then allow each os to implement its own call, and add an asm-generic helper. Are you volunteering to add the x86 hook? Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 08/19] arch: um: add shim to trap to allow installing a fault catcher for tests
On Wed, Nov 28, 2018 at 11:36:25AM -0800, Brendan Higgins wrote:
> diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
> index cced829460427..bf90e678b3d71 100644
> --- a/arch/um/kernel/trap.c
> +++ b/arch/um/kernel/trap.c
> @@ -201,6 +201,12 @@ void segv_handler(int sig, struct siginfo *unused_si,
> struct uml_pt_regs *regs)
> segv(*fi, UPT_IP(regs), UPT_IS_USER(regs), regs);
> }
>
> +static void segv_run_catcher(jmp_buf *catcher, void *fault_addr)
> +{
> + current->thread.fault_addr = fault_addr;
> + UML_LONGJMP(catcher, 1);
> +}
> +
> /*
> * We give a *copy* of the faultinfo in the regs to segv.
> * This must be done, since nesting SEGVs could overwrite
> @@ -219,7 +225,10 @@ unsigned long segv(struct faultinfo fi, unsigned long
> ip, int is_user,
> if (!is_user && regs)
> current->thread.segv_regs = container_of(regs, struct pt_regs,
> regs);
>
> - if (!is_user && (address >= start_vm) && (address < end_vm)) {
> + catcher = current->thread.fault_catcher;
This and..
> + if (catcher && current->thread.is_running_test)
> + segv_run_catcher(catcher, (void *) address);
> + else if (!is_user && (address >= start_vm) && (address < end_vm)) {
> flush_tlb_kernel_vm();
> goto out;
> }
*not this*
> @@ -246,12 +255,10 @@ unsigned long segv(struct faultinfo fi, unsigned long
> ip, int is_user,
> address = 0;
> }
>
> - catcher = current->thread.fault_catcher;
> if (!err)
> goto out;
> else if (catcher != NULL) {
> - current->thread.fault_addr = (void *) address;
> - UML_LONGJMP(catcher, 1);
> + segv_run_catcher(catcher, (void *) address);
> }
> else if (current->thread.fault_addr != NULL)
> panic("fault_addr set but no fault catcher");
But with this seems one atomic change which should be submitted
separately, its just a helper. Think it would make the actual
change needed easier to review, ie, your needed changes would
be smaller and clearer for what you need.
Luis
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 06/19] arch: um: enable running kunit from User Mode Linux
On Wed, Nov 28, 2018 at 11:36:23AM -0800, Brendan Higgins wrote: > Make minimum number of changes outside of the KUnit directories for > KUnit to build and run using UML. > > Signed-off-by: Brendan Higgins > --- > Kconfig | 2 ++ > Makefile | 2 +- > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Kconfig b/Kconfig > index 48a80beab6853..10428501edb78 100644 > --- a/Kconfig > +++ b/Kconfig > @@ -30,3 +30,5 @@ source "crypto/Kconfig" > source "lib/Kconfig" > > source "lib/Kconfig.debug" > + > +source "kunit/Kconfig" Since this is all UML why not source it from arch/um/Kconfig instead? Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 03/19] kunit: test: add string_stream a std::stream like string builder
On Wed, Nov 28, 2018 at 11:36:20AM -0800, Brendan Higgins wrote:
> A number of test features need to do pretty complicated string printing
> where it may not be possible to rely on a single preallocated string
> with parameters.
>
> So provide a library for constructing the string as you go similar to
> C++'s std::string.
Hrm, what's the potential for such thing actually being eventually
generically useful for printk folks, I wonder? Petr?
Luis
>
> Signed-off-by: Brendan Higgins
> ---
> include/kunit/string-stream.h | 44 ++
> kunit/Makefile| 3 +-
> kunit/string-stream.c | 149 ++
> 3 files changed, 195 insertions(+), 1 deletion(-)
> create mode 100644 include/kunit/string-stream.h
> create mode 100644 kunit/string-stream.c
>
> diff --git a/include/kunit/string-stream.h b/include/kunit/string-stream.h
> new file mode 100644
> index 0..933ed5740cf07
> --- /dev/null
> +++ b/include/kunit/string-stream.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * C++ stream style string builder used in KUnit for building messages.
> + *
> + * Copyright (C) 2018, Google LLC.
> + * Author: Brendan Higgins
> + */
> +
> +#ifndef _KUNIT_STRING_STREAM_H
> +#define _KUNIT_STRING_STREAM_H
> +
> +#include
> +#include
> +#include
> +#include
> +
> +struct string_stream_fragment {
> + struct list_head node;
> + char *fragment;
> +};
> +
> +struct string_stream {
> + size_t length;
> + struct list_head fragments;
> +
> + /* length and fragments are protected by this lock */
> + spinlock_t lock;
> + struct kref refcount;
> + int (*add)(struct string_stream *this, const char *fmt, ...);
> + int (*vadd)(struct string_stream *this, const char *fmt, va_list args);
> + char *(*get_string)(struct string_stream *this);
> + void (*clear)(struct string_stream *this);
> + bool (*is_empty)(struct string_stream *this);
> +};
> +
> +struct string_stream *new_string_stream(void);
> +
> +void destroy_string_stream(struct string_stream *stream);
> +
> +void string_stream_get(struct string_stream *stream);
> +
> +int string_stream_put(struct string_stream *stream);
> +
> +#endif /* _KUNIT_STRING_STREAM_H */
> diff --git a/kunit/Makefile b/kunit/Makefile
> index 5efdc4dea2c08..275b565a0e81f 100644
> --- a/kunit/Makefile
> +++ b/kunit/Makefile
> @@ -1 +1,2 @@
> -obj-$(CONFIG_KUNIT) += test.o
> +obj-$(CONFIG_KUNIT) += test.o \
> + string-stream.o
> diff --git a/kunit/string-stream.c b/kunit/string-stream.c
> new file mode 100644
> index 0..1e7efa630cc35
> --- /dev/null
> +++ b/kunit/string-stream.c
> @@ -0,0 +1,149 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * C++ stream style string builder used in KUnit for building messages.
> + *
> + * Copyright (C) 2018, Google LLC.
> + * Author: Brendan Higgins
> + */
> +
> +#include
> +#include
> +#include
> +
> +static int string_stream_vadd(struct string_stream *this,
> +const char *fmt,
> +va_list args)
> +{
> + struct string_stream_fragment *fragment;
> + int len;
> + va_list args_for_counting;
> + unsigned long flags;
> +
> + /* Make a copy because `vsnprintf` could change it */
> + va_copy(args_for_counting, args);
> +
> + /* Need space for null byte. */
> + len = vsnprintf(NULL, 0, fmt, args_for_counting) + 1;
> +
> + va_end(args_for_counting);
> +
> + fragment = kmalloc(sizeof(*fragment), GFP_KERNEL);
> + if (!fragment)
> + return -ENOMEM;
> +
> + fragment->fragment = kmalloc(len, GFP_KERNEL);
> + if (!fragment->fragment) {
> + kfree(fragment);
> + return -ENOMEM;
> + }
> +
> + len = vsnprintf(fragment->fragment, len, fmt, args);
> + spin_lock_irqsave(>lock, flags);
> + this->length += len;
> + list_add_tail(>node, >fragments);
> + spin_unlock_irqrestore(>lock, flags);
> + return 0;
> +}
> +
> +static int string_stream_add(struct string_stream *this, const char *fmt,
> ...)
> +{
> + va_list args;
> + int result;
> +
> + va_start(args, fmt);
> + result = string_stream_vadd(this, fmt, args);
> + va_end(args);
> + return result;
> +}
> +
> +static void string_stream_clear(struct string_stream *this)
> +{
> + struct string_stream_fragment *fragment, *fragment_safe;
> + unsigned long flags;
> +
> + spin_lock_irqsave(>lock, flags);
> + list_for_each_entry_safe(fragment,
> + fragment_safe,
> + >fragments,
> + node) {
> + list_del(>node);
> + kfree(fragment->fragment);
> + kfree(fragment);
> + }
> + this->length = 0;
> + spin_unlock_irqrestore(>lock, flags);
> +}
> +
> +static char
Re: [RFC v3 01/19] kunit: test: add KUnit test runner core
> +static void kunit_run_case_internal(struct kunit *test,
> + struct kunit_module *module,
> + struct kunit_case *test_case)
> +{
> + int ret;
> +
> + if (module->init) {
> + ret = module->init(test);
> + if (ret) {
> + kunit_err(test, "failed to initialize: %d", ret);
> + kunit_set_success(test, false);
> + return;
> + }
> + }
> +
> + test_case->run_case(test);
> +}
<-- snip -->
> +static bool kunit_run_case(struct kunit *test,
> +struct kunit_module *module,
> +struct kunit_case *test_case)
> +{
> + kunit_set_success(test, true);
> +
> + kunit_run_case_internal(test, module, test_case);
> + kunit_run_case_cleanup(test, module, test_case);
> +
> + return kunit_get_success(test);
> +}
So we are running the module->init() for each test case... is that
correct? Shouldn't the init run once? Also, typically init calls are
pegged with __init so we free them later. You seem to have skipped the
init annotations. Why?
Luis
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 01/19] kunit: test: add KUnit test runner core
On Wed, Nov 28, 2018 at 11:36:18AM -0800, Brendan Higgins wrote:
> +#define module_test(module) \
> + static int module_kunit_init##module(void) \
> + { \
> + return kunit_run_tests(); \
> + } \
> + late_initcall(module_kunit_init##module)
Here in lies an assumption that suffices. I'm inclined to believe we
need new initcall level here so to ensure we *do* run after all the
respective kernels iniut calls. Otherwise we're left at the whims of
link order for kunit. For instance if a kunit test relies on frameworks
which are also late_initcall() we'd have complete incompatibility with
anything linked *after* kunit.
> diff --git a/kunit/Kconfig b/kunit/Kconfig
> new file mode 100644
> index 0..49b44c4f6630a
> --- /dev/null
> +++ b/kunit/Kconfig
> @@ -0,0 +1,17 @@
> +#
> +# KUnit base configuration
> +#
> +
> +menu "KUnit support"
> +
> +config KUNIT
> + bool "Enable support for unit tests (KUnit)"
> + depends on UML
Consider using:
if UML
...
endif
That allows the depends to be done once.
> + help
> + Enables support for kernel unit tests (KUnit), a lightweight unit
> + testing and mocking framework for the Linux kernel. These tests are
> + able to be run locally on a developer's workstation without a VM or
> + special hardware.
Some mention of UML may be good here?
> For more information, please see
> + Documentation/kunit/
> +
> +endmenu
I'm a bit conflicted here. This currently depends on UML but yet you
noted on RFC v2 that your intention is to liberate kunit from UML and
ideally allow unit tests to depend only on userspace. I've addressed
tests using both selftests kernels drivers and also re-written kernel
APIs to userspace to test there. I think we may need to live with both.
Then for the UML stuff, I think if we *really* accept that UML will
always be a viable option we should probably consider now throwing these
things under drivers/platform/uml/. This follows the pattern of arch
specific drivers. Whether or not we end up with a complete userspace
component independent of UML may implicate having a shared component
somewhere else.
Likewise, I realize the goal is to *avoid* using a virtual machine for
these tests, but would it in any way make sense to share kunit to be
supported for other architectures to allow easier-to-write tests as
well?
Luis
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 00/14] kunit: introduce KUnit, the Linux kernel unit testing framework
On Wed, Nov 28, 2018 at 01:50:01PM -0700, shuah wrote: > On 11/28/18 12:54 PM, Knut Omang wrote: > > On Mon, 2018-11-26 at 17:41 -0800, Brendan Higgins wrote: > > Both approaches provide assertion macros for running tests inside the > > kernel, > > I doubt the kernel community would like to see yet two such sets of macros, > > with differing syntax merged. One of the good reasons to have a *framework* > > is that it is widely used and understood, so that people coming from one > > part of the > > kernel can easily run, understand and relate to selected tests from another > > part. > > > > The goal with KTF is to allow this for any kernel, old or new, not just > > kernels > > built specifically for testing purposes. We felt that providing it as a > > separate git > > module (still open source, for anyone to contribute to) is a more efficient > > approach > > until we have more examples and experience with using it in different parts > > of the kernel. We can definitely post the kernel side of KTF as a patch > > series fairly soon > > if the community wants us to. Except for hybrid tests, the ktf.ko module > > works fine > > independently of any user side support, just using the debugfs interface to > > run and > > examine tests. > > > > Having test framework in the kernel sources tree has benefits. It allows > us (kernel developers) to do co-development of kernel features and tests > for these features. Agreed! > It encourages developers to write regression tests. More importantly, > kernel features and tests for these features are included in the same > release in most cases and/or allows us freedom to do so if test framework > and tests are part of the kernel sources. > > We have seen this with our experience with kselftest. It would not have > see the same level of attention and growth if it stayed outside the > kernel sources. > > Most kernel developers would not want to include a externally maintained > module for testing. As a general rule, it has to be easy to run tests. > > > I think there are good uses cases for having the ability to maintain a > > single source for tests that can be run against multiple kernels, > > also distro kernels that the test framework cannot expect to be able to > > modify, > > except from using the module interfaces. > > Same reasons as above. Having the tests included in the kernel sources > makes it easier for distros to run those tests and include running them > during their qualification. Also... selftests are an example of tests which *are* upstream and yet there are teams out there using them to test these tests on older kernels. So the scripts for instance are supposed to work with older kernels. So if you expand on a feature your selftest script should detect if the new mechanism is present or not, and also be backward compatible with older kernels. > > And there are good arguments for having (at least parts of) > > the test framework easily available within the kernel under test. > > > > When Kernel unit, functional, and regressions tests reside in the kernel > sources, they evolve quicker as kernel developers contribute tests as > part of their kernel work-flow. Maintaining tests and framework > separately will make it harder to maintain them and keep them updated > for us the kernel community. Agreed! Also, I actually see no issue with having *both* kunit / ktest merged upstream. IMHO we should not be forcing people to pick one or the other but rather we should: let the best test framework win. Similar as we did with LSMs. Each test framework has its own gains / advantages. Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
