* Re: [PATCH v12 09/18] kunit: test: add support for test abort
From: Brendan Higgins @ 2019-08-13 7:52 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190813055615.CA787206C2@mail.kernel.org>
On Mon, Aug 12, 2019 at 10:56 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-08-12 21:57:55)
> > On Mon, Aug 12, 2019 at 9:22 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Brendan Higgins (2019-08-12 11:24:12)
> > > > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > > index 2625bcfeb19ac..93381f841e09f 100644
> > > > --- a/include/kunit/test.h
> > > > +++ b/include/kunit/test.h
> > > > @@ -176,6 +178,11 @@ struct kunit {
> > > > */
> > > > bool success; /* Read only after test_case finishes! */
> > > > spinlock_t lock; /* Gaurds all mutable test state. */
> > > > + /*
> > > > + * death_test may be both set and unset from multiple threads in a test
> > > > + * case.
> > > > + */
> > > > + bool death_test; /* Protected by lock. */
> > > > /*
> > > > * Because resources is a list that may be updated multiple times (with
> > > > * new resources) from any thread associated with a test case, we must
> > > > @@ -184,6 +191,13 @@ struct kunit {
> > > > struct list_head resources; /* Protected by lock. */
> > > > };
> > > >
> > > > +static inline void kunit_set_death_test(struct kunit *test, bool death_test)
> > > > +{
> > > > + spin_lock(&test->lock);
> > > > + test->death_test = death_test;
> > > > + spin_unlock(&test->lock);
> > > > +}
> > >
> > > These getters and setters are using spinlocks again. It doesn't make any
> > > sense. It probably needs a rework like was done for the other bool
> > > member, success.
> >
> > No, this is intentional. death_test can transition from false to true
> > and then back to false within the same test. Maybe that deserves a
> > comment?
>
> Yes. How does it transition from true to false again?
The purpose is to tell try_catch that it was expected for the test to
bail out. Given the default implementation there is no way for this to
happen aside from abort() being called, but in some implementations it
is possible to implement a fault catcher which allows a test suite to
recover from an unexpected failure.
Maybe it would be best to drop this until I add one of those
alternative implementations.
> Either way, having a spinlock around a read/write API doesn't make sense
> because it just makes sure that two writes don't overlap, but otherwise
> does nothing to keep things synchronized. For example a set to true
> after a set to false when the two calls to set true or false aren't
> synchronized means they can happen in any order. So I don't see how it
> needs a spinlock. The lock needs to be one level higher.
There shouldn't be any cases where one thread is trying to set it
while another is trying to unset it. The thing I am worried about here
is making sure all the cores see the write, and making sure no reads
or writes get reordered before it. So I guess I just want a fence. So
I take it I should probably have is a WRITE_ONCE here and READ_ONCE in
the getter?
> >
> > > > +
> > > > void kunit_init_test(struct kunit *test, const char *name);
> > > >
> > > > int kunit_run_tests(struct kunit_suite *suite);
> > > > diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
> > > > new file mode 100644
> > > > index 0000000000000..8a414a9af0b64
> > > > --- /dev/null
> > > > +++ b/include/kunit/try-catch.h
> [...]
> > > > +
> > > > +/*
> > > > + * struct kunit_try_catch - provides a generic way to run code which might fail.
> > > > + * @context: used to pass user data to the try and catch functions.
> > > > + *
> > > > + * kunit_try_catch provides a generic, architecture independent way to execute
> > > > + * an arbitrary function of type kunit_try_catch_func_t which may bail out by
> > > > + * calling kunit_try_catch_throw(). If kunit_try_catch_throw() is called, @try
> > > > + * is stopped at the site of invocation and @catch is catch is called.
> > > > + *
> > > > + * struct kunit_try_catch provides a generic interface for the functionality
> > > > + * needed to implement kunit->abort() which in turn is needed for implementing
> > > > + * assertions. Assertions allow stating a precondition for a test simplifying
> > > > + * how test cases are written and presented.
> > > > + *
> > > > + * Assertions are like expectations, except they abort (call
> > > > + * kunit_try_catch_throw()) when the specified condition is not met. This is
> > > > + * useful when you look at a test case as a logical statement about some piece
> > > > + * of code, where assertions are the premises for the test case, and the
> > > > + * conclusion is a set of predicates, rather expectations, that must all be
> > > > + * true. If your premises are violated, it does not makes sense to continue.
> > > > + */
> > > > +struct kunit_try_catch {
> > > > + /* private: internal use only. */
> > > > + struct kunit *test;
> > > > + struct completion *try_completion;
> > > > + int try_result;
> > > > + kunit_try_catch_func_t try;
> > > > + kunit_try_catch_func_t catch;
> > >
> > > Can these other variables be documented in the kernel doc? And should
> > > context be marked as 'public'?
> >
> > Sure, I can document them.
> >
> > But I don't think context should be public; it should only be accessed
> > by kunit_try_catch_* functions. context should only be populated by
> > *_init, and will be passed into *try and *catch when they are called
> > internally.
>
> Ok. Then I guess just document them all but keep them all marked as
> private.
Will do.
> >
> > > > + */
> > > > +void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch);
> > > > +
> > > > +#endif /* _KUNIT_TRY_CATCH_H */
> > > > diff --git a/kunit/test.c b/kunit/test.c
> > > > index e5080a2c6b29c..995cb53fe4ee9 100644
> > > > --- a/kunit/test.c
> > > > +++ b/kunit/test.c
> > > > @@ -158,6 +171,21 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
> > > > kunit_print_string_stream(test, stream);
> > > > }
> > > >
> > > > +void __noreturn kunit_abort(struct kunit *test)
> > > > +{
> > > > + kunit_set_death_test(test, true);
> > > > +
> > > > + kunit_try_catch_throw(&test->try_catch);
> > > > +
> > > > + /*
> > > > + * Throw could not abort from test.
> > > > + *
> > > > + * XXX: we should never reach this line! As kunit_try_catch_throw is
> > > > + * marked __noreturn.
> > > > + */
> > > > + WARN_ONCE(true, "Throw could not abort from test!\n");
> > >
> > > Should this just be a BUG_ON? It's supposedly impossible.
> >
> > It should be impossible; it will only reach this line if there is a
> > bug in kunit_try_catch_throw. The reason I didn't use BUG_ON was
> > because I previously got yelled at for having BUG_ON in this code
> > path.
> >
> > Nevertheless, I think BUG_ON is more correct, so if you will stand by
> > it, then that's what I will do.
>
> Yeah BUG_ON is appropriate here and self documenting so please use it.
Cool, will do.
> >
> > > > + return;
> > > > + }
> > > > +
> > > > + if (kunit_get_death_test(test)) {
> > > > + /*
> > > > + * EXPECTED DEATH: kunit_run_case_internal encountered
> > > > + * anticipated fatal error. Everything should be in a safe
> > > > + * state.
> > > > + */
> > > > + kunit_run_case_cleanup(test, suite);
> > > > + } else {
> > > > + /*
> > > > + * UNEXPECTED DEATH: kunit_run_case_internal encountered an
> > > > + * unanticipated fatal error. We have no idea what the state of
> > > > + * the test case is in.
> > > > + */
> > > > + kunit_handle_test_crash(test, suite, test_case);
> > > > + kunit_set_failure(test);
> > >
> > > Like was done here.
> >
> > Sorry, like what?
>
> Just saying this has braces for the if-else.
Ah, gotcha.
^ permalink raw reply
* Re: [PATCH v12 10/18] kunit: test: add tests for kunit test abort
From: Brendan Higgins @ 2019-08-13 7:53 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190813055707.8B2BB206C2@mail.kernel.org>
On Mon, Aug 12, 2019 at 10:57 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-08-12 22:06:04)
> > On Mon, Aug 12, 2019 at 9:24 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Brendan Higgins (2019-08-12 11:24:13)
> > > > +
> > > > +static int kunit_try_catch_test_init(struct kunit *test)
> > > > +{
> > > > + struct kunit_try_catch_test_context *ctx;
> > > > +
> > > > + ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> > >
> > > Can this fail? Should return -ENOMEM in that case?
> >
> > Yes, I should do that.
>
> Looks like it's asserted to not be an error. If it's pushed into the API
> then there's nothing to do here, and you can have my reviewed-by on this
> patch.
>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Cool, thanks!
^ permalink raw reply
* Re: [PATCH v12 12/18] kunit: test: add tests for KUnit managed resources
From: Brendan Higgins @ 2019-08-13 7:57 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg,
Avinash Kondareddy
In-Reply-To: <20190813043140.67FF320644@mail.kernel.org>
On Mon, Aug 12, 2019 at 9:31 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-08-12 11:24:15)
> > +
> > +static int kunit_resource_test_init(struct kunit *test)
> > +{
> > + struct kunit_test_resource_context *ctx =
> > + kzalloc(sizeof(*ctx), GFP_KERNEL);
> > +
> > + if (!ctx)
> > + return -ENOMEM;
>
> Should this use the test assertion logic to make sure that it's not
> failing allocations during init?
Yep. Will fix.
> BTW, maybe kunit allocation APIs should
> fail the test if they fail to allocate in general. Unless we're unit
> testing failure to allocate problems.
Yeah, I thought about that. I wasn't sure how people would feel about
it, and I thought it would be a pain to tease out all the issues
arising from aborting in different contexts when someone might not
expect it.
I am thinking later we can have kunit_kmalloc_or_abort variants? And
then we can punt this issue to a later time?
> > +
> > + test->priv = ctx;
> > +
> > + kunit_init_test(&ctx->test, "test_test_context");
> > +
> > + return 0;
^ permalink raw reply
* Re: [PATCH v12 14/18] kunit: defconfig: add defconfigs for building KUnit tests
From: Brendan Higgins @ 2019-08-13 7:59 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190813043859.661F82054F@mail.kernel.org>
On Mon, Aug 12, 2019 at 9:39 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-08-12 11:24:17)
> > diff --git a/arch/um/configs/kunit_defconfig b/arch/um/configs/kunit_defconfig
> > new file mode 100644
> > index 0000000000000..bfe49689038f1
> > --- /dev/null
> > +++ b/arch/um/configs/kunit_defconfig
> > @@ -0,0 +1,8 @@
> > +CONFIG_OF=y
> > +CONFIG_OF_UNITTEST=y
> > +CONFIG_OF_OVERLAY=y
> > +CONFIG_I2C=y
> > +CONFIG_I2C_MUX=y
> > +CONFIG_KUNIT=y
> > +CONFIG_KUNIT_TEST=y
> > +CONFIG_KUNIT_EXAMPLE_TEST=y
> > diff --git a/tools/testing/kunit/configs/all_tests.config b/tools/testing/kunit/configs/all_tests.config
> > new file mode 100644
> > index 0000000000000..bfe49689038f1
> > --- /dev/null
> > +++ b/tools/testing/kunit/configs/all_tests.config
> > @@ -0,0 +1,8 @@
> > +CONFIG_OF=y
> > +CONFIG_OF_UNITTEST=y
> > +CONFIG_OF_OVERLAY=y
> > +CONFIG_I2C=y
> > +CONFIG_I2C_MUX=y
>
> Are these above config options necessary? I don't think they're part of
> the patch series anymore so it looks odd to enable the OF unittests and
> i2c configs.
Oh whoops, I forgot that we dropped the OF_UNITTEST from this
patchset. Will fix.
> > +CONFIG_KUNIT=y
> > +CONFIG_KUNIT_TEST=y
> > +CONFIG_KUNIT_EXAMPLE_TEST=y
^ permalink raw reply
* Re: [RFC 00/19] Integration of Kernel Test Framework (KTF) into the kernel tree
From: Brendan Higgins @ 2019-08-13 8:10 UTC (permalink / raw)
To: Knut Omang
Cc: open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List,
open list:DOCUMENTATION, linux-kbuild, Shuah Khan,
Jonathan Corbet, Masahiro Yamada, Michal Marek,
Greg Kroah-Hartman, Shreyans Devendra Doshi, Alan Maguire,
Kevin Hilman, Hidenori Yamaji, Frank Rowand, Timothy Bird,
Luis Chamberlain, Theodore Ts'o, Daniel Vetter, Stephen Boyd
In-Reply-To: <cover.92d76bb4f6dcedc971d0b72a49e8e459a98bca54.1565676440.git-series.knut.omang@oracle.com>
On Mon, Aug 12, 2019 at 11:11 PM Knut Omang <knut.omang@oracle.com> wrote:
>
> KTF has already been available for a while as a separate git repository with
> means to facilitate use with any kernel version.
>
> KTF can be used both for "pure" unit testing and for more pragmatic
> approaches to component testing. Apart from some useful features that
> KTF uses from the kernel toolbox (such as kallsyms, kprobes),
> KTF does not depend on any special environment such as UML or on a
> large set of mocked up APIs to be useful. KTF basically allows test
> code to be inserted and managed as separate kernel modules, while
> providing the tests convenient access to almost the full range of kernel
> APIs, both exposed and private. And once a KTF test set exists, it
> should be fairly easy to compile and run it against older versions of
> the kernel as well.
>
> This series proposes a non-intrusive integration of KTF into the kernel
> hopefully presented in digestable pieces.
> For convenience, the patch set is also available on top of v5.2
> at https://github.com/knuto/linux/pull/new/ktf_v1 .
>
> The high level structure of the KTF code is as follows:
>
> External dependencies for the user land side:
> * libnl3 for netlink communication
> * googletest for test runner, reporting and test selection support.
So I take it that KTF depends on a fully booted kernel with an up and
running userspace?
> Kernel components:
> * Simple core test and test suite related abstractions:
> core data structures ktf_case, ktf_test, an assertion macro "infrastructure"
> with ASSERT_* and EXPECT_* macros and helper functions
> * Bookkeeping data structures:
> - ktf_map - a (key, value) mapping container used to implement management
> of instances of the higher level abstraction needs, such as ktf_handle and ktf_context.
> - ktf_handle: A global environment that a test runs within.
> - ktf_context: a test suite specific abstraction to allow a test to execute within
> one or more contexts. An example use of context can be a device
> instance: A test can be instantiated to run on all available such
> devices, according to test suite defined criteria.
> * A generic netlink protocol (ktf_nl, ktf_unlproto) which defines operations to
> allow a user space part of a test to query the kernel for test information,
> invoke tests and get feedback reports about results.
> * An alternative debugfs interface to allow examining and executing kernel-only tests
> without a user level program.
> * Support for overriding and modifying behaviour of kernel calls.
>
> User mode components:
> * A test executor based on and integrated with Googletest.
> Googletest is one of several mature user land unit test suites for
> C/C++. The choice allowed us to focus on kernel specific
> functionality rather than having to reinvent too many wheels.
> * Tools to aid in creating new test modules (suites):
> To facilitate a developer friendly way of testing internals of a module or the
> kernel itself, one of the important features of KTF, we often need to access
> symbols deliberately not exposed from a module.
> KTF contains a script used to create definitions based on kallsyms
> lookup for easy access to symbols not exposed by a module or the kernel.
> The user just provides a simple text file with a list of the symbols by
> module.
>
> This series is an attempt to address feedback from several people
> that having the functionality readily available within the kernel repository
> is desired.
>
> An in-tree KTF allows test suites to be provided with the kernel, which makes
> it easy to use KTF based suites as part of the kernel selftests and driver
> test suites. Having the ability to still build and run the latest versions of
> test suites against older kernels should be of great value to stable maintainers,
> distributions and driver maintainers, who would want to have an easy path,
> with minimal backporting efforts to make sure that criterias implemented by
> new test logic is also valid for these kernels.
>
> Our definite goal moving forward is to try to satisfy both needs in a
> transparent way. The plan is to let the standalone KTF repository follow the
> in-kernel one, and to allow test suites to be maintained similarly,
> and to support maintenance by proper tooling.
>
> Mode of integration into the kernel
> ===================================
>
> One feature of KTF is that it allows tests to work, look and feel similar
> whether they execute entirely in user mode, entirely in kernel mode,
> or half and half (hybrid tests). KTF consist of both user space
> and kernel code. Unlike e.g. kselftest, KTF in the Github version
> does not attempt to address the test runner aspects of testing.
>
> Due to the need for building modules, KTF requires access to kernel module
> build facilities (obj-m). But KTF also has nontrivial needs for user
> land building, and we think it is good to keep the build structure in a way that
> allows KTF to be built both in-tree and out-of-tree without
> necessarily having to reconfigure the kernel.
>
> This first version of kernel integration of KTF solves this challenge
> by co-locating everything associated with KTF under ktf/ as in the
> github version, but use the little used hostprogs-y and hostlibs-y
> features to build the user space side. The first patch in the series is
> fixes to make it work in a natural way to suit our needs.
>
> Positioning for natural building within the kernel tree
> =======================================================
>
> Currently we find significant amount of C level tests within the following paths:
>
> tools/testing/selftests/ (kselftests, almost entirely user space so far)
> lib/ (various kernel level mostly unit tests)
>
> and in the making::
>
> kunit/ (kernel only (UML))
>
> So all kernel code are currently located directly within the kernel
> build paths, accessed from the top level Makefile, to allow everything
> to be controlled by config and from the main build targets for the
> kernel. But this also poses challenges, in that .config has to be
> modified to build tests. And once a .config is changed, we no longer
> in principle logically operate on the same kernel.
>
> A better approach seems to be to follow the principle
> taken by kselftest: To have all the logic associated with the test
> inside the test tree, and make it available for building separately
> from the kernel itself. This require us to have a means to build
> kernel modules from within the test tree, separately from the main
> kernel paths. Currently this seems to only by supported via the M=
> option used to build out-of-tree modules. This was also easy to get to work
> for the kernel parts, based on the Github version of KTF, where we
> already do this. With the additional need to compile user land code,
> using the corresponding hostprogs-y and hostNNlibs-y seemed natural,
> but this has been challenging: The build macros does not really
> support hostprogs-y etc as "first class citizens" so some amount of
> hacking is in there in this first draft version.
> Using hostprogs-y etc is also a different approach that what is used
> for C code in kselftest today, but we imagine that there's room for
> unification here to get the best of both worlds, with the help of
> the wider Kbuild community.
>
> As an initial proposal, we have positioned ktf as an additional
> kselftest target, under tools/testing/selftests/ktf, and the recommended:
>
> make TARGETS="ktf" kselftest
>
> way of building and running should work, even from normal user accounts,
> if the user running it has sudo rights (for the kernel module insertion
> and removal). This will run the selftests for KTF itself, and should
> be a good starting point for adding more test cases. We also have had
> activities going to take some of the existing test suites under lib/
> and convert them into KTF based test suites, and will get back to this
> later.
>
> A trimmed down output from the above make target would look like this:
>
> ...
> CC [M] tools/testing/selftests/ktf/kernel/ktf_override.o
> LD [M] tools/testing/selftests/ktf/kernel/ktf.o
> HOSTCC -fPIC tools/testing/selftests/ktf/lib/ktf_unlproto.o
> HOSTCXX -fPIC tools/testing/selftests/ktf/lib/ktf_int.o
> KTFSYMS tools/testing/selftests/ktf/selftest/ktf_syms.h
> CC [M] tools/testing/selftests/ktf/selftest/self.o
> LD [M] tools/testing/selftests/ktf/selftest/selftest.o
> HOSTCXX tools/testing/selftests/ktf/user/ktftest.o
> HOSTCXX tools/testing/selftests/ktf/user/hybrid.o
> HOSTLD tools/testing/selftests/ktf/user/ktftest
> Building modules, stage 2.
> MODPOST 7 modules
> LD [M] tools/testing/selftests/ktf/kernel/ktf.ko
> LD [M] tools/testing/selftests/ktf/selftest/selftest.ko
> running tests
> make BUILD=/net/abi/local/abi/build/kernel/ktf/tools/testing/selftests -f scripts/runtests.mk run_tests
> TAP version 13
> 1..1
> ...
> ok 1 selftests: ktf: runtests.sh
>
> We're looking forward to feedback on this, and also to more discussion
> around unit testing at the testing & fuzzing workshop at LPC!
Sounds good! Glad to have this on the lists!
> Alan Maguire (3):
> ktf: Implementation of ktf support for overriding function entry and return.
> ktf: A simple debugfs interface to test results
> ktf: Simple coverage support
>
> Knut Omang (16):
> kbuild: Fixes to rules for host-cshlib and host-cxxshlib
> ktf: Introduce the main part of the kernel side of ktf
> ktf: Introduce a generic netlink protocol for test result communication
> ktf: An implementation of a generic associative array container
> ktf: Configurable context support for network info setup
> ktf: resolve: A helper utility to aid in exposing private kernel symbols to KTF tests.
> ktf: Add documentation for Kernel Test Framework (KTF)
> ktf: Add a small test suite with a few tests to test KTF itself
> ktf: Main part of user land library for executing tests
> ktf: Integration logic for running ktf tests from googletest
> ktf: Internal debugging facilities
> ktf: Some simple examples
> ktf: Some user applications to run tests
> ktf: Toplevel ktf Makefile/makefile includes and scripts to run from kselftest
> kselftests: Enable building ktf
> Documentation/dev-tools: Add index entry for KTF documentation
>
> Documentation/dev-tools/index.rst | 1 +-
> Documentation/dev-tools/ktf/concepts.rst | 242 +++-
> Documentation/dev-tools/ktf/debugging.rst | 248 +++-
> Documentation/dev-tools/ktf/examples.rst | 26 +-
> Documentation/dev-tools/ktf/features.rst | 307 ++++-
> Documentation/dev-tools/ktf/implementation.rst | 70 +-
> Documentation/dev-tools/ktf/index.rst | 14 +-
> Documentation/dev-tools/ktf/installation.rst | 73 +-
> Documentation/dev-tools/ktf/introduction.rst | 134 ++-
> Documentation/dev-tools/ktf/progref.rst | 144 ++-
> scripts/Makefile.host | 17 +-
> tools/testing/selftests/Makefile | 1 +-
> tools/testing/selftests/ktf/Makefile | 21 +-
> tools/testing/selftests/ktf/examples/Makefile | 17 +-
> tools/testing/selftests/ktf/examples/h2.c | 45 +-
> tools/testing/selftests/ktf/examples/h3.c | 84 +-
> tools/testing/selftests/ktf/examples/h4.c | 62 +-
> tools/testing/selftests/ktf/examples/hello.c | 38 +-
> tools/testing/selftests/ktf/examples/kgdemo.c | 61 +-
> tools/testing/selftests/ktf/kernel/Makefile | 15 +-
> tools/testing/selftests/ktf/kernel/ktf.h | 604 +++++++-
> tools/testing/selftests/ktf/kernel/ktf_context.c | 409 +++++-
> tools/testing/selftests/ktf/kernel/ktf_cov.c | 690 ++++++++-
> tools/testing/selftests/ktf/kernel/ktf_cov.h | 94 +-
> tools/testing/selftests/ktf/kernel/ktf_debugfs.c | 356 ++++-
> tools/testing/selftests/ktf/kernel/ktf_debugfs.h | 34 +-
> tools/testing/selftests/ktf/kernel/ktf_map.c | 261 +++-
> tools/testing/selftests/ktf/kernel/ktf_map.h | 154 ++-
> tools/testing/selftests/ktf/kernel/ktf_netctx.c | 132 ++-
> tools/testing/selftests/ktf/kernel/ktf_netctx.h | 64 +-
> tools/testing/selftests/ktf/kernel/ktf_nl.c | 516 ++++++-
> tools/testing/selftests/ktf/kernel/ktf_nl.h | 15 +-
> tools/testing/selftests/ktf/kernel/ktf_override.c | 45 +-
> tools/testing/selftests/ktf/kernel/ktf_override.h | 15 +-
> tools/testing/selftests/ktf/kernel/ktf_test.c | 397 +++++-
> tools/testing/selftests/ktf/kernel/ktf_test.h | 381 ++++-
> tools/testing/selftests/ktf/kernel/ktf_unlproto.h | 105 +-
> tools/testing/selftests/ktf/lib/Makefile | 21 +-
> tools/testing/selftests/ktf/lib/ktf.h | 114 +-
> tools/testing/selftests/ktf/lib/ktf_debug.cc | 20 +-
> tools/testing/selftests/ktf/lib/ktf_debug.h | 59 +-
> tools/testing/selftests/ktf/lib/ktf_int.cc | 1031 ++++++++++++-
> tools/testing/selftests/ktf/lib/ktf_int.h | 84 +-
> tools/testing/selftests/ktf/lib/ktf_run.cc | 177 ++-
> tools/testing/selftests/ktf/lib/ktf_unlproto.c | 21 +-
> tools/testing/selftests/ktf/scripts/ktf_syms.mk | 16 +-
> tools/testing/selftests/ktf/scripts/resolve | 188 ++-
> tools/testing/selftests/ktf/scripts/runtests.mk | 3 +-
> tools/testing/selftests/ktf/scripts/runtests.sh | 100 +-
> tools/testing/selftests/ktf/scripts/top_make.mk | 14 +-
> tools/testing/selftests/ktf/selftest/Makefile | 17 +-
> tools/testing/selftests/ktf/selftest/context.c | 149 ++-
> tools/testing/selftests/ktf/selftest/context.h | 15 +-
> tools/testing/selftests/ktf/selftest/context_self.h | 34 +-
> tools/testing/selftests/ktf/selftest/hybrid.c | 35 +-
> tools/testing/selftests/ktf/selftest/hybrid.h | 24 +-
> tools/testing/selftests/ktf/selftest/hybrid_self.h | 27 +-
> tools/testing/selftests/ktf/selftest/ktf_syms.txt | 17 +-
> tools/testing/selftests/ktf/selftest/self.c | 661 ++++++++-
> tools/testing/selftests/ktf/user/Makefile | 26 +-
> tools/testing/selftests/ktf/user/hybrid.cc | 39 +-
> tools/testing/selftests/ktf/user/ktfcov.cc | 68 +-
> tools/testing/selftests/ktf/user/ktfrun.cc | 20 +-
> tools/testing/selftests/ktf/user/ktftest.cc | 46 +-
> 64 files changed, 8909 insertions(+), 9 deletions(-)
> create mode 100644 Documentation/dev-tools/ktf/concepts.rst
> create mode 100644 Documentation/dev-tools/ktf/debugging.rst
> create mode 100644 Documentation/dev-tools/ktf/examples.rst
> create mode 100644 Documentation/dev-tools/ktf/features.rst
> create mode 100644 Documentation/dev-tools/ktf/implementation.rst
> create mode 100644 Documentation/dev-tools/ktf/index.rst
> create mode 100644 Documentation/dev-tools/ktf/installation.rst
> create mode 100644 Documentation/dev-tools/ktf/introduction.rst
> create mode 100644 Documentation/dev-tools/ktf/progref.rst
> create mode 100644 tools/testing/selftests/ktf/Makefile
> create mode 100644 tools/testing/selftests/ktf/examples/Makefile
> create mode 100644 tools/testing/selftests/ktf/examples/h2.c
> create mode 100644 tools/testing/selftests/ktf/examples/h3.c
> create mode 100644 tools/testing/selftests/ktf/examples/h4.c
> create mode 100644 tools/testing/selftests/ktf/examples/hello.c
> create mode 100644 tools/testing/selftests/ktf/examples/kgdemo.c
> create mode 100644 tools/testing/selftests/ktf/kernel/Makefile
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf.h
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_context.c
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_cov.c
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_cov.h
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_debugfs.c
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_debugfs.h
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_map.c
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_map.h
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_netctx.c
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_netctx.h
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_nl.c
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_nl.h
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_override.c
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_override.h
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_test.c
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_test.h
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_unlproto.h
> create mode 100644 tools/testing/selftests/ktf/lib/Makefile
> create mode 100644 tools/testing/selftests/ktf/lib/ktf.h
> create mode 100644 tools/testing/selftests/ktf/lib/ktf_debug.cc
> create mode 100644 tools/testing/selftests/ktf/lib/ktf_debug.h
> create mode 100644 tools/testing/selftests/ktf/lib/ktf_int.cc
> create mode 100644 tools/testing/selftests/ktf/lib/ktf_int.h
> create mode 100644 tools/testing/selftests/ktf/lib/ktf_run.cc
> create mode 100644 tools/testing/selftests/ktf/lib/ktf_unlproto.c
> create mode 100644 tools/testing/selftests/ktf/scripts/ktf_syms.mk
> create mode 100755 tools/testing/selftests/ktf/scripts/resolve
> create mode 100644 tools/testing/selftests/ktf/scripts/runtests.mk
> create mode 100755 tools/testing/selftests/ktf/scripts/runtests.sh
> create mode 100644 tools/testing/selftests/ktf/scripts/top_make.mk
> create mode 100644 tools/testing/selftests/ktf/selftest/Makefile
> create mode 100644 tools/testing/selftests/ktf/selftest/context.c
> create mode 100644 tools/testing/selftests/ktf/selftest/context.h
> create mode 100644 tools/testing/selftests/ktf/selftest/context_self.h
> create mode 100644 tools/testing/selftests/ktf/selftest/hybrid.c
> create mode 100644 tools/testing/selftests/ktf/selftest/hybrid.h
> create mode 100644 tools/testing/selftests/ktf/selftest/hybrid_self.h
> create mode 100644 tools/testing/selftests/ktf/selftest/ktf_syms.txt
> create mode 100644 tools/testing/selftests/ktf/selftest/self.c
> create mode 100644 tools/testing/selftests/ktf/user/Makefile
> create mode 100644 tools/testing/selftests/ktf/user/hybrid.cc
> create mode 100644 tools/testing/selftests/ktf/user/ktfcov.cc
> create mode 100644 tools/testing/selftests/ktf/user/ktfrun.cc
> create mode 100644 tools/testing/selftests/ktf/user/ktftest.cc
>
> base-commit: 0ecfebd2b52404ae0c54a878c872bb93363ada36
> --
> git-series 0.9.1
^ permalink raw reply
* Re: [RFC 00/19] Integration of Kernel Test Framework (KTF) into the kernel tree
From: Brendan Higgins @ 2019-08-13 8:17 UTC (permalink / raw)
To: Knut Omang
Cc: open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List,
open list:DOCUMENTATION, linux-kbuild, Shuah Khan,
Jonathan Corbet, Masahiro Yamada, Michal Marek,
Greg Kroah-Hartman, Shreyans Devendra Doshi, Alan Maguire,
Kevin Hilman, Hidenori Yamaji, Frank Rowand, Timothy Bird,
Luis Chamberlain, Theodore Ts'o, Daniel Vetter, Stephen Boyd
In-Reply-To: <cover.92d76bb4f6dcedc971d0b72a49e8e459a98bca54.1565676440.git-series.knut.omang@oracle.com>
On Mon, Aug 12, 2019 at 11:11 PM Knut Omang <knut.omang@oracle.com> wrote:
[...]
> Alan Maguire (3):
> ktf: Implementation of ktf support for overriding function entry and return.
> ktf: A simple debugfs interface to test results
> ktf: Simple coverage support
>
> Knut Omang (16):
> kbuild: Fixes to rules for host-cshlib and host-cxxshlib
> ktf: Introduce the main part of the kernel side of ktf
> ktf: Introduce a generic netlink protocol for test result communication
> ktf: An implementation of a generic associative array container
> ktf: Configurable context support for network info setup
> ktf: resolve: A helper utility to aid in exposing private kernel symbols to KTF tests.
> ktf: Add documentation for Kernel Test Framework (KTF)
> ktf: Add a small test suite with a few tests to test KTF itself
> ktf: Main part of user land library for executing tests
> ktf: Integration logic for running ktf tests from googletest
> ktf: Internal debugging facilities
> ktf: Some simple examples
> ktf: Some user applications to run tests
> ktf: Toplevel ktf Makefile/makefile includes and scripts to run from kselftest
> kselftests: Enable building ktf
> Documentation/dev-tools: Add index entry for KTF documentation
>
> Documentation/dev-tools/index.rst | 1 +-
> Documentation/dev-tools/ktf/concepts.rst | 242 +++-
> Documentation/dev-tools/ktf/debugging.rst | 248 +++-
> Documentation/dev-tools/ktf/examples.rst | 26 +-
> Documentation/dev-tools/ktf/features.rst | 307 ++++-
> Documentation/dev-tools/ktf/implementation.rst | 70 +-
> Documentation/dev-tools/ktf/index.rst | 14 +-
> Documentation/dev-tools/ktf/installation.rst | 73 +-
> Documentation/dev-tools/ktf/introduction.rst | 134 ++-
> Documentation/dev-tools/ktf/progref.rst | 144 ++-
> scripts/Makefile.host | 17 +-
> tools/testing/selftests/Makefile | 1 +-
> tools/testing/selftests/ktf/Makefile | 21 +-
> tools/testing/selftests/ktf/examples/Makefile | 17 +-
> tools/testing/selftests/ktf/examples/h2.c | 45 +-
> tools/testing/selftests/ktf/examples/h3.c | 84 +-
> tools/testing/selftests/ktf/examples/h4.c | 62 +-
> tools/testing/selftests/ktf/examples/hello.c | 38 +-
> tools/testing/selftests/ktf/examples/kgdemo.c | 61 +-
> tools/testing/selftests/ktf/kernel/Makefile | 15 +-
> tools/testing/selftests/ktf/kernel/ktf.h | 604 +++++++-
> tools/testing/selftests/ktf/kernel/ktf_context.c | 409 +++++-
> tools/testing/selftests/ktf/kernel/ktf_cov.c | 690 ++++++++-
> tools/testing/selftests/ktf/kernel/ktf_cov.h | 94 +-
> tools/testing/selftests/ktf/kernel/ktf_debugfs.c | 356 ++++-
> tools/testing/selftests/ktf/kernel/ktf_debugfs.h | 34 +-
> tools/testing/selftests/ktf/kernel/ktf_map.c | 261 +++-
> tools/testing/selftests/ktf/kernel/ktf_map.h | 154 ++-
> tools/testing/selftests/ktf/kernel/ktf_netctx.c | 132 ++-
> tools/testing/selftests/ktf/kernel/ktf_netctx.h | 64 +-
> tools/testing/selftests/ktf/kernel/ktf_nl.c | 516 ++++++-
> tools/testing/selftests/ktf/kernel/ktf_nl.h | 15 +-
> tools/testing/selftests/ktf/kernel/ktf_override.c | 45 +-
> tools/testing/selftests/ktf/kernel/ktf_override.h | 15 +-
> tools/testing/selftests/ktf/kernel/ktf_test.c | 397 +++++-
> tools/testing/selftests/ktf/kernel/ktf_test.h | 381 ++++-
> tools/testing/selftests/ktf/kernel/ktf_unlproto.h | 105 +-
> tools/testing/selftests/ktf/lib/Makefile | 21 +-
> tools/testing/selftests/ktf/lib/ktf.h | 114 +-
> tools/testing/selftests/ktf/lib/ktf_debug.cc | 20 +-
> tools/testing/selftests/ktf/lib/ktf_debug.h | 59 +-
> tools/testing/selftests/ktf/lib/ktf_int.cc | 1031 ++++++++++++-
> tools/testing/selftests/ktf/lib/ktf_int.h | 84 +-
> tools/testing/selftests/ktf/lib/ktf_run.cc | 177 ++-
> tools/testing/selftests/ktf/lib/ktf_unlproto.c | 21 +-
> tools/testing/selftests/ktf/scripts/ktf_syms.mk | 16 +-
> tools/testing/selftests/ktf/scripts/resolve | 188 ++-
> tools/testing/selftests/ktf/scripts/runtests.mk | 3 +-
> tools/testing/selftests/ktf/scripts/runtests.sh | 100 +-
> tools/testing/selftests/ktf/scripts/top_make.mk | 14 +-
> tools/testing/selftests/ktf/selftest/Makefile | 17 +-
> tools/testing/selftests/ktf/selftest/context.c | 149 ++-
> tools/testing/selftests/ktf/selftest/context.h | 15 +-
> tools/testing/selftests/ktf/selftest/context_self.h | 34 +-
> tools/testing/selftests/ktf/selftest/hybrid.c | 35 +-
> tools/testing/selftests/ktf/selftest/hybrid.h | 24 +-
> tools/testing/selftests/ktf/selftest/hybrid_self.h | 27 +-
> tools/testing/selftests/ktf/selftest/ktf_syms.txt | 17 +-
> tools/testing/selftests/ktf/selftest/self.c | 661 ++++++++-
> tools/testing/selftests/ktf/user/Makefile | 26 +-
> tools/testing/selftests/ktf/user/hybrid.cc | 39 +-
> tools/testing/selftests/ktf/user/ktfcov.cc | 68 +-
> tools/testing/selftests/ktf/user/ktfrun.cc | 20 +-
> tools/testing/selftests/ktf/user/ktftest.cc | 46 +-
> 64 files changed, 8909 insertions(+), 9 deletions(-)
It also looks like all your test code lives outside of the kernel
source dir. I take it you intend for tests to live in
tools/testing/selftests/ktf/ ?
[...]
^ permalink raw reply
* Re: [RFC 06/19] ktf: A simple debugfs interface to test results
From: Greg Kroah-Hartman @ 2019-08-13 8:21 UTC (permalink / raw)
To: Knut Omang
Cc: linux-kselftest, linux-kernel, linux-doc, linux-kbuild,
Shuah Khan, Jonathan Corbet, Masahiro Yamada, Michal Marek,
Shreyans Devendra Doshi, Alan Maguire, Brendan Higgins,
Kevin Hilman, Hidenori Yamaji, Frank Rowand, Timothy Bird,
Luis Chamberlain, Theodore Ts'o, Daniel Vetter, Stephen Boyd
In-Reply-To: <ae6c38384e2338aa3cfb8a4e4dd1002833789253.1565676440.git-series.knut.omang@oracle.com>
On Tue, Aug 13, 2019 at 08:09:21AM +0200, Knut Omang wrote:
> From: Alan Maguire <alan.maguire@oracle.com>
>
> While test results is available via netlink from user space, sometimes
> it may be useful to be able to access the results from the kernel as well,
> for instance due to a crash. Make that possible via debugfs.
>
> ktf_debugfs.h: Support for creating a debugfs representation of test
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
> tools/testing/selftests/ktf/kernel/ktf_debugfs.c | 356 ++++++++++++++++-
> tools/testing/selftests/ktf/kernel/ktf_debugfs.h | 34 ++-
> 2 files changed, 390 insertions(+)
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_debugfs.c
> create mode 100644 tools/testing/selftests/ktf/kernel/ktf_debugfs.h
>
> diff --git a/tools/testing/selftests/ktf/kernel/ktf_debugfs.c b/tools/testing/selftests/ktf/kernel/ktf_debugfs.c
> new file mode 100644
> index 0000000..a20fbd2
> --- /dev/null
> +++ b/tools/testing/selftests/ktf/kernel/ktf_debugfs.c
> @@ -0,0 +1,356 @@
> +/*
> + * Copyright (c) 2009, 2017, Oracle and/or its affiliates. All rights reserved.
> + * Author: Alan Maguire <alan.maguire@oracle.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0
Has to be the first line of the file, did you run this through
checkpatch?
> +static int ktf_run_test_open(struct inode *inode, struct file *file)
> +{
> + struct ktf_test *t;
> +
> + if (!try_module_get(THIS_MODULE))
> + return -EIO;
This is an anti-pattern, and one guaranteed to not work properly. NEVER
do this.
> +
> + t = (struct ktf_test *)inode->i_private;
> +
> + return single_open(file, ktf_debugfs_run, t);
> +}
> +
> +static int ktf_debugfs_release(struct inode *inode, struct file *file)
> +{
> + module_put(THIS_MODULE);
Same here, not ok.
> + return single_release(inode, file);
> +}
> +
> +static const struct file_operations ktf_run_test_fops = {
> + .open = ktf_run_test_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = ktf_debugfs_release,
> +};
> +
> +static int ktf_results_test_open(struct inode *inode, struct file *file)
> +{
> + struct ktf_test *t;
> +
> + if (!try_module_get(THIS_MODULE))
> + return -EIO;
Nope!
And why -EIO? That is not an io issue.
> +void ktf_debugfs_create_test(struct ktf_test *t)
> +{
> + struct ktf_case *testset = ktf_case_find(t->tclass);
> +
> + if (!testset)
> + return;
> +
> + memset(&t->debugfs, 0, sizeof(t->debugfs));
> +
> + t->debugfs.debugfs_results_test =
> + debugfs_create_file(t->name, S_IFREG | 0444,
> + testset->debugfs.debugfs_results_test,
> + t, &ktf_results_test_fops);
> +
> + if (t->debugfs.debugfs_results_test) {
How can that variable ever be NULL (hint, it can not.)
> + t->debugfs.debugfs_run_test =
> + debugfs_create_file(t->name, S_IFREG | 0444,
> + testset->debugfs.debugfs_run_test,
> + t, &ktf_run_test_fops);
> + if (!t->debugfs.debugfs_run_test) {
> + _ktf_debugfs_destroy_test(t);
> + } else {
> + /* Take reference for test for debugfs */
> + ktf_test_get(t);
> + }
> + }
Never test the result of any debugfs call, you do not need to. Just
call it and move on, your code flow should NEVER be different with, or
without, a successful debugfs call.
> +static int ktf_run_testset_open(struct inode *inode, struct file *file)
> +{
> + struct ktf_case *testset;
> +
> + if (!try_module_get(THIS_MODULE))
> + return -EIO;
Again no. I hate to know what code you copied this all from, as that
code is very wrong. Do you have a pointer to that code anywhere so we
can fix that up?
> +
> + testset = (struct ktf_case *)inode->i_private;
> +
> + return single_open(file, ktf_debugfs_run_all, testset);
> +}
> +
> +static const struct file_operations ktf_run_testset_fops = {
> + .open = ktf_run_testset_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = ktf_debugfs_release,
If you really care about module references you should be setting the
owner of the module here.
> +};
> +
> +static void _ktf_debugfs_destroy_testset(struct ktf_case *testset)
> +{
> + debugfs_remove(testset->debugfs.debugfs_run_testset);
> + debugfs_remove(testset->debugfs.debugfs_run_test);
> + debugfs_remove(testset->debugfs.debugfs_results_testset);
> + debugfs_remove(testset->debugfs.debugfs_results_test);
Why not just recursivly remove the directory? That way you do not have
to keep track of any individual files.
> +}
> +
> +void ktf_debugfs_create_testset(struct ktf_case *testset)
> +{
> + char tests_subdir[KTF_DEBUGFS_NAMESZ];
> + const char *name = ktf_case_name(testset);
> +
> + memset(&testset->debugfs, 0, sizeof(testset->debugfs));
> +
> + /* First add /sys/kernel/debug/ktf/[results|run]/<testset> */
> + testset->debugfs.debugfs_results_testset =
> + debugfs_create_file(name, S_IFREG | 0444,
> + ktf_debugfs_resultsdir,
> + testset, &ktf_results_testset_fops);
> + if (!testset->debugfs.debugfs_results_testset)
> + goto err;
Again, can never happen, and again, do not do different things depending
on the result of a debugfs call.
> +
> + testset->debugfs.debugfs_run_testset =
> + debugfs_create_file(name, S_IFREG | 0444,
> + ktf_debugfs_rundir,
> + testset, &ktf_run_testset_fops);
> + if (!testset->debugfs.debugfs_run_testset)
> + goto err;
Again, nope.
> +
> + /* Now add parent directories for individual test result/run tests
> + * which live in
> + * /sys/kernel/debug/ktf/[results|run]/<testset>-tests/<testname>
> + */
> + (void)snprintf(tests_subdir, sizeof(tests_subdir), "%s%s",
> + name, KTF_DEBUGFS_TESTS_SUFFIX);
why (void)?
> +
> + testset->debugfs.debugfs_results_test =
> + debugfs_create_dir(tests_subdir, ktf_debugfs_resultsdir);
> + if (!testset->debugfs.debugfs_results_test)
> + goto err;
nope :)
> +
> + testset->debugfs.debugfs_run_test =
> + debugfs_create_dir(tests_subdir, ktf_debugfs_rundir);
> + if (!testset->debugfs.debugfs_run_test)
> + goto err;
Nope :)
> +
> + /* Take reference count for testset. One will do as we will always
> + * free testset debugfs resources together.
> + */
> + ktf_case_get(testset);
> + return;
> +err:
> + _ktf_debugfs_destroy_testset(testset);
> +}
> +
> +void ktf_debugfs_destroy_testset(struct ktf_case *testset)
> +{
> + tlog(T_DEBUG, "Destroying debugfs testset %s", ktf_case_name(testset));
> + _ktf_debugfs_destroy_testset(testset);
> + /* Remove our debugfs reference cout to testset */
> + ktf_case_put(testset);
> +}
> +
> +/* /sys/kernel/debug/ktf/coverage shows coverage statistics. */
> +static int ktf_debugfs_cov(struct seq_file *seq, void *v)
> +{
> + ktf_cov_seq_print(seq);
> +
> + return 0;
> +}
> +
> +static int ktf_cov_open(struct inode *inode, struct file *file)
> +{
> + if (!try_module_get(THIS_MODULE))
> + return -EIO;
{sigh} I'll stop reviewing now :)
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC 00/19] Integration of Kernel Test Framework (KTF) into the kernel tree
From: Greg Kroah-Hartman @ 2019-08-13 8:23 UTC (permalink / raw)
To: Knut Omang
Cc: linux-kselftest, linux-kernel, linux-doc, linux-kbuild,
Shuah Khan, Jonathan Corbet, Masahiro Yamada, Michal Marek,
Shreyans Devendra Doshi, Alan Maguire, Brendan Higgins,
Kevin Hilman, Hidenori Yamaji, Frank Rowand, Timothy Bird,
Luis Chamberlain, Theodore Ts'o, Daniel Vetter, Stephen Boyd
In-Reply-To: <cover.92d76bb4f6dcedc971d0b72a49e8e459a98bca54.1565676440.git-series.knut.omang@oracle.com>
On Tue, Aug 13, 2019 at 08:09:15AM +0200, Knut Omang wrote:
> and in the making::
>
> kunit/ (kernel only (UML))
You are going to have to integrate this with kunit, to come up with a
superset of both in the end.
And I do not think that kunit is only UML, it's just that seems to be
what Brendan tests with, but should work with other arches as well.
thanks,
greg k-h
^ permalink raw reply
* [PATCH v4 2/2] hwmon: pmbus: Add Inspur Power System power supply driver
From: John Wang @ 2019-08-13 8:34 UTC (permalink / raw)
To: jdelvare, linux, corbet, linux-hwmon, linux-doc, linux-kernel,
openbmc, duanzhijia01, mine260309, joel
Add the driver to monitor Inspur Power System power supplies
with hwmon over pmbus.
This driver adds sysfs attributes for additional power supply data,
including vendor, model, part_number, serial number,
firmware revision, hardware revision, and psu mode(active/standby).
Signed-off-by: John Wang <wangzqbj@inspur.com>
---
v4:
- Remove the additional tabs in the Makefile
- Rebased on 5.3-rc4, not 5.2
v3:
- Sort kconfig/makefile entries alphabetically
- Remove unnecessary initialization
- Use ATTRIBUTE_GROUPS instead of expanding directly
- Use memscan to avoid reimplementation
v2:
- Fix typos in commit message
- Invert Christmas tree
- Configure device with sysfs attrs, not debugfs entries
- Fix errno in fw_version_read, ENODATA to EPROTO
- Change the print format of fw-version
- Use sysfs_streq instead of strcmp("xxx" "\n", "xxx")
- Document sysfs attributes
---
Documentation/hwmon/inspur-ipsps1.rst | 79 +++++++++
drivers/hwmon/pmbus/Kconfig | 9 +
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/inspur-ipsps.c | 226 ++++++++++++++++++++++++++
4 files changed, 315 insertions(+)
create mode 100644 Documentation/hwmon/inspur-ipsps1.rst
create mode 100644 drivers/hwmon/pmbus/inspur-ipsps.c
diff --git a/Documentation/hwmon/inspur-ipsps1.rst b/Documentation/hwmon/inspur-ipsps1.rst
new file mode 100644
index 000000000000..aa19f0ccc8b0
--- /dev/null
+++ b/Documentation/hwmon/inspur-ipsps1.rst
@@ -0,0 +1,79 @@
+Kernel driver inspur-ipsps1
+=======================
+
+Supported chips:
+
+ * Inspur Power System power supply unit
+
+Author: John Wang <wangzqbj@inspur.com>
+
+Description
+-----------
+
+This driver supports Inspur Power System power supplies. This driver
+is a client to the core PMBus driver.
+
+Usage Notes
+-----------
+
+This driver does not auto-detect devices. You will have to instantiate the
+devices explicitly. Please see Documentation/i2c/instantiating-devices for
+details.
+
+Sysfs entries
+-------------
+
+The following attributes are supported:
+
+======================= ======================================================
+curr1_input Measured input current
+curr1_label "iin"
+curr1_max Maximum current
+curr1_max_alarm Current high alarm
+curr2_input Measured output current in mA.
+curr2_label "iout1"
+curr2_crit Critical maximum current
+curr2_crit_alarm Current critical high alarm
+curr2_max Maximum current
+curr2_max_alarm Current high alarm
+
+fan1_alarm Fan 1 warning.
+fan1_fault Fan 1 fault.
+fan1_input Fan 1 speed in RPM.
+
+in1_alarm Input voltage under-voltage alarm.
+in1_input Measured input voltage in mV.
+in1_label "vin"
+in2_input Measured output voltage in mV.
+in2_label "vout1"
+in2_lcrit Critical minimum output voltage
+in2_lcrit_alarm Output voltage critical low alarm
+in2_max Maximum output voltage
+in2_max_alarm Output voltage high alarm
+in2_min Minimum output voltage
+in2_min_alarm Output voltage low alarm
+
+power1_alarm Input fault or alarm.
+power1_input Measured input power in uW.
+power1_label "pin"
+power1_max Input power limit
+power2_max_alarm Output power high alarm
+power2_max Output power limit
+power2_input Measured output power in uW.
+power2_label "pout"
+
+temp[1-3]_input Measured temperature
+temp[1-2]_max Maximum temperature
+temp[1-3]_max_alarm Temperature high alarm
+
+vendor Manufacturer name
+model Product model
+part_number Product part number
+serial_number Product serial number
+fw_version Firmware version
+hw_version Hardware version
+mode Work mode. Can be set to active or
+ standby, when set to standby, PSU will
+ automatically switch between standby
+ and redundancy mode.
+======================= ======================================================
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index b6588483fae1..d62d69bb7e49 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -46,6 +46,15 @@ config SENSORS_IBM_CFFPS
This driver can also be built as a module. If so, the module will
be called ibm-cffps.
+config SENSORS_INSPUR_IPSPS
+ tristate "INSPUR Power System Power Supply"
+ help
+ If you say yes here you get hardware monitoring support for the INSPUR
+ Power System power supply.
+
+ This driver can also be built as a module. If so, the module will
+ be called inspur-ipsps.
+
config SENSORS_IR35221
tristate "Infineon IR35221"
help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index c950ea9a5d00..03bacfcfd660 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_PMBUS) += pmbus_core.o
obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
+obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
obj-$(CONFIG_SENSORS_IR38064) += ir38064.o
obj-$(CONFIG_SENSORS_IRPS5401) += irps5401.o
diff --git a/drivers/hwmon/pmbus/inspur-ipsps.c b/drivers/hwmon/pmbus/inspur-ipsps.c
new file mode 100644
index 000000000000..fa981b881a60
--- /dev/null
+++ b/drivers/hwmon/pmbus/inspur-ipsps.c
@@ -0,0 +1,226 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2019 Inspur Corp.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pmbus.h>
+#include <linux/hwmon-sysfs.h>
+
+#include "pmbus.h"
+
+#define IPSPS_REG_VENDOR_ID 0x99
+#define IPSPS_REG_MODEL 0x9A
+#define IPSPS_REG_FW_VERSION 0x9B
+#define IPSPS_REG_PN 0x9C
+#define IPSPS_REG_SN 0x9E
+#define IPSPS_REG_HW_VERSION 0xB0
+#define IPSPS_REG_MODE 0xFC
+
+#define MODE_ACTIVE 0x55
+#define MODE_STANDBY 0x0E
+#define MODE_REDUNDANCY 0x00
+
+#define MODE_ACTIVE_STRING "active"
+#define MODE_STANDBY_STRING "standby"
+#define MODE_REDUNDANCY_STRING "redundancy"
+
+enum ipsps_index {
+ vendor,
+ model,
+ fw_version,
+ part_number,
+ serial_number,
+ hw_version,
+ mode,
+ num_regs,
+};
+
+static const u8 ipsps_regs[num_regs] = {
+ [vendor] = IPSPS_REG_VENDOR_ID,
+ [model] = IPSPS_REG_MODEL,
+ [fw_version] = IPSPS_REG_FW_VERSION,
+ [part_number] = IPSPS_REG_PN,
+ [serial_number] = IPSPS_REG_SN,
+ [hw_version] = IPSPS_REG_HW_VERSION,
+ [mode] = IPSPS_REG_MODE,
+};
+
+static ssize_t ipsps_string_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ u8 reg;
+ int rc;
+ char *p;
+ char data[I2C_SMBUS_BLOCK_MAX + 1];
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ rc = i2c_smbus_read_block_data(client, reg, data);
+ if (rc < 0)
+ return rc;
+
+ /* filled with printable characters, ending with # */
+ p = memscan(data, '#', rc);
+ *p = '\0';
+
+ return snprintf(buf, PAGE_SIZE, "%s\n", data);
+}
+
+static ssize_t ipsps_fw_version_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ u8 reg;
+ int rc;
+ u8 data[I2C_SMBUS_BLOCK_MAX] = { 0 };
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ rc = i2c_smbus_read_block_data(client, reg, data);
+ if (rc < 0)
+ return rc;
+
+ if (rc != 6)
+ return -EPROTO;
+
+ return snprintf(buf, PAGE_SIZE, "%u.%02u%u-%u.%02u\n",
+ data[1], data[2]/* < 100 */, data[3]/*< 10*/,
+ data[4], data[5]/* < 100 */);
+}
+
+static ssize_t ipsps_mode_show(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ u8 reg;
+ int rc;
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ rc = i2c_smbus_read_byte_data(client, reg);
+ if (rc < 0)
+ return rc;
+
+ switch (rc) {
+ case MODE_ACTIVE:
+ return snprintf(buf, PAGE_SIZE, "[%s] %s %s\n",
+ MODE_ACTIVE_STRING,
+ MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+ case MODE_STANDBY:
+ return snprintf(buf, PAGE_SIZE, "%s [%s] %s\n",
+ MODE_ACTIVE_STRING,
+ MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+ case MODE_REDUNDANCY:
+ return snprintf(buf, PAGE_SIZE, "%s %s [%s]\n",
+ MODE_ACTIVE_STRING,
+ MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+ default:
+ return snprintf(buf, PAGE_SIZE, "unspecified\n");
+ }
+}
+
+static ssize_t ipsps_mode_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ u8 reg;
+ int rc;
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ if (sysfs_streq(MODE_STANDBY_STRING, buf)) {
+ rc = i2c_smbus_write_byte_data(client, reg,
+ MODE_STANDBY);
+ if (rc < 0)
+ return rc;
+ return count;
+ } else if (sysfs_streq(MODE_ACTIVE_STRING, buf)) {
+ rc = i2c_smbus_write_byte_data(client, reg,
+ MODE_ACTIVE);
+ if (rc < 0)
+ return rc;
+ return count;
+ }
+
+ return -EINVAL;
+}
+
+static SENSOR_DEVICE_ATTR_RO(vendor, ipsps_string, vendor);
+static SENSOR_DEVICE_ATTR_RO(model, ipsps_string, model);
+static SENSOR_DEVICE_ATTR_RO(part_number, ipsps_string, part_number);
+static SENSOR_DEVICE_ATTR_RO(serial_number, ipsps_string, serial_number);
+static SENSOR_DEVICE_ATTR_RO(hw_version, ipsps_string, hw_version);
+static SENSOR_DEVICE_ATTR_RO(fw_version, ipsps_fw_version, fw_version);
+static SENSOR_DEVICE_ATTR_RW(mode, ipsps_mode, mode);
+
+static struct attribute *ipsps_attrs[] = {
+ &sensor_dev_attr_vendor.dev_attr.attr,
+ &sensor_dev_attr_model.dev_attr.attr,
+ &sensor_dev_attr_part_number.dev_attr.attr,
+ &sensor_dev_attr_serial_number.dev_attr.attr,
+ &sensor_dev_attr_hw_version.dev_attr.attr,
+ &sensor_dev_attr_fw_version.dev_attr.attr,
+ &sensor_dev_attr_mode.dev_attr.attr,
+ NULL,
+};
+
+ATTRIBUTE_GROUPS(ipsps);
+
+static struct pmbus_driver_info ipsps_info = {
+ .pages = 1,
+ .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_IIN | PMBUS_HAVE_POUT | PMBUS_HAVE_PIN |
+ PMBUS_HAVE_FAN12 | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
+ PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_VOUT |
+ PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
+ PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_STATUS_FAN12,
+ .groups = ipsps_groups,
+};
+
+static struct pmbus_platform_data ipsps_pdata = {
+ .flags = PMBUS_SKIP_STATUS_CHECK,
+};
+
+static int ipsps_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ client->dev.platform_data = &ipsps_pdata;
+ return pmbus_do_probe(client, id, &ipsps_info);
+}
+
+static const struct i2c_device_id ipsps_id[] = {
+ { "inspur_ipsps1", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, ipsps_id);
+
+static const struct of_device_id ipsps_of_match[] = {
+ { .compatible = "inspur,ipsps1" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, ipsps_of_match);
+
+static struct i2c_driver ipsps_driver = {
+ .driver = {
+ .name = "inspur-ipsps",
+ .of_match_table = ipsps_of_match,
+ },
+ .probe = ipsps_probe,
+ .remove = pmbus_do_remove,
+ .id_table = ipsps_id,
+};
+
+module_i2c_driver(ipsps_driver);
+
+MODULE_AUTHOR("John Wang");
+MODULE_DESCRIPTION("PMBus driver for Inspur Power System power supplies");
+MODULE_LICENSE("GPL");
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
From: Brendan Higgins @ 2019-08-13 9:04 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190813053023.CC86120651@mail.kernel.org>
On Mon, Aug 12, 2019 at 10:30 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-08-12 22:02:59)
> > On Mon, Aug 12, 2019 at 9:56 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Brendan Higgins (2019-08-12 17:41:05)
> > > > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > > >
> > > > > > kunit_resource_destroy (respective equivalents to devm_kfree, and
> > > > > > devres_destroy) and use kunit_kfree here?
> > > > > >
> > > > >
> > > > > Yes, or drop the API entirely? Does anything need this functionality?
> > > >
> > > > Drop the kunit_resource API? I would strongly prefer not to.
> > >
> > > No. I mean this API, string_stream_clear(). Does anything use it?
> >
> > Oh, right. No.
> >
> > However, now that I added the kunit_resource_destroy, I thought it
> > might be good to free the string_stream after I use it in each call to
> > kunit_assert->format(...) in which case I will be using this logic.
> >
> > So I think the right thing to do is to expose string_stream_destroy so
> > kunit_do_assert can clean up when it's done, and then demote
> > string_stream_clear to static. Sound good?
>
> Ok, sure. I don't really see how clearing it explicitly when the
> assertion prints vs. never allocating it to begin with is really any
> different. Maybe I've missed something though.
It's for the case that we *do* print something out. Once we are doing
printing, we don't want the fragments anymore.
^ permalink raw reply
* Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
From: Brendan Higgins @ 2019-08-13 9:12 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <CAFd5g47v7410QRAizPV8zaHrKrc95-Sk-GNzRRVngN741OKnvg@mail.gmail.com>
On Tue, Aug 13, 2019 at 2:04 AM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Mon, Aug 12, 2019 at 10:30 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Brendan Higgins (2019-08-12 22:02:59)
> > > On Mon, Aug 12, 2019 at 9:56 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > >
> > > > Quoting Brendan Higgins (2019-08-12 17:41:05)
> > > > > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > > > >
> > > > > > > kunit_resource_destroy (respective equivalents to devm_kfree, and
> > > > > > > devres_destroy) and use kunit_kfree here?
> > > > > > >
> > > > > >
> > > > > > Yes, or drop the API entirely? Does anything need this functionality?
> > > > >
> > > > > Drop the kunit_resource API? I would strongly prefer not to.
> > > >
> > > > No. I mean this API, string_stream_clear(). Does anything use it?
> > >
> > > Oh, right. No.
> > >
> > > However, now that I added the kunit_resource_destroy, I thought it
> > > might be good to free the string_stream after I use it in each call to
> > > kunit_assert->format(...) in which case I will be using this logic.
> > >
> > > So I think the right thing to do is to expose string_stream_destroy so
> > > kunit_do_assert can clean up when it's done, and then demote
> > > string_stream_clear to static. Sound good?
> >
> > Ok, sure. I don't really see how clearing it explicitly when the
> > assertion prints vs. never allocating it to begin with is really any
> > different. Maybe I've missed something though.
>
> It's for the case that we *do* print something out. Once we are doing
> printing, we don't want the fragments anymore.
Oops, sorry fat fingered: s/doing/done
^ permalink raw reply
* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Michal Hocko @ 2019-08-13 9:14 UTC (permalink / raw)
To: Joel Fernandes
Cc: Andrew Morton, linux-kernel, Alexey Dobriyan, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen, dancol, fmayer,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Kees Cook,
kernel-team, linux-api, linux-doc, linux-fsdevel, linux-mm,
Mike Rapoport, minchan, namhyung, paulmck, Robin Murphy,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190812145620.GB224541@google.com>
On Mon 12-08-19 10:56:20, Joel Fernandes wrote:
> On Thu, Aug 08, 2019 at 10:00:44AM +0200, Michal Hocko wrote:
> > On Wed 07-08-19 17:31:05, Joel Fernandes wrote:
> > > On Wed, Aug 07, 2019 at 01:58:40PM -0700, Andrew Morton wrote:
> > > > On Wed, 7 Aug 2019 16:45:30 -0400 Joel Fernandes <joel@joelfernandes.org> wrote:
> > > >
> > > > > On Wed, Aug 07, 2019 at 01:04:02PM -0700, Andrew Morton wrote:
> > > > > > On Wed, 7 Aug 2019 13:15:54 -0400 "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote:
> > > > > >
> > > > > > > In Android, we are using this for the heap profiler (heapprofd) which
> > > > > > > profiles and pin points code paths which allocates and leaves memory
> > > > > > > idle for long periods of time. This method solves the security issue
> > > > > > > with userspace learning the PFN, and while at it is also shown to yield
> > > > > > > better results than the pagemap lookup, the theory being that the window
> > > > > > > where the address space can change is reduced by eliminating the
> > > > > > > intermediate pagemap look up stage. In virtual address indexing, the
> > > > > > > process's mmap_sem is held for the duration of the access.
> > > > > >
> > > > > > So is heapprofd a developer-only thing? Is heapprofd included in
> > > > > > end-user android loads? If not then, again, wouldn't it be better to
> > > > > > make the feature Kconfigurable so that Android developers can enable it
> > > > > > during development then disable it for production kernels?
> > > > >
> > > > > Almost all of this code is already configurable with
> > > > > CONFIG_IDLE_PAGE_TRACKING. If you disable it, then all of this code gets
> > > > > disabled.
> > > > >
> > > > > Or are you referring to something else that needs to be made configurable?
> > > >
> > > > Yes - the 300+ lines of code which this patchset adds!
> > > >
> > > > The impacted people will be those who use the existing
> > > > idle-page-tracking feature but who will not use the new feature. I
> > > > guess we can assume this set is small...
> > >
> > > Yes, I think this set should be small. The code size increase of page_idle.o
> > > is from ~1KB to ~2KB. Most of the extra space is consumed by
> > > page_idle_proc_generic() function which this patch adds. I don't think adding
> > > another CONFIG option to disable this while keeping existing
> > > CONFIG_IDLE_PAGE_TRACKING enabled, is worthwhile but I am open to the
> > > addition of such an option if anyone feels strongly about it. I believe that
> > > once this patch is merged, most like this new interface being added is what
> > > will be used more than the old interface (for some of the usecases) so it
> > > makes sense to keep it alive with CONFIG_IDLE_PAGE_TRACKING.
> >
> > I would tend to agree with Joel here. The functionality falls into an
> > existing IDLE_PAGE_TRACKING config option quite nicely. If there really
> > are users who want to save some space and this is standing in the way
> > then they can easily add a new config option with some justification so
> > the savings are clear. Without that an additional config simply adds to
> > the already existing configurability complexity and balkanization.
>
> Michal, Andrew, Minchan,
>
> Would you have any other review comments on the v5 series? This is just a new
> interface that does not disrupt existing users of the older page-idle
> tracking, so as such it is a safe change (as in, doesn't change existing
> functionality except for the draining bug fix).
I hope to find some more time to finish the review but let me point out
that "it's new it is regression safe" is not really a great argument for
a new user visible API. If the API is flawed then this is likely going
to kick us later and will be hard to fix. I am still not convinced about
the swap part of the thing TBH.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC 00/19] Integration of Kernel Test Framework (KTF) into the kernel tree
From: Knut Omang @ 2019-08-13 9:51 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kselftest, linux-kernel, linux-doc, linux-kbuild,
Shuah Khan, Jonathan Corbet, Masahiro Yamada, Michal Marek,
Shreyans Devendra Doshi, Alan Maguire, Brendan Higgins,
Kevin Hilman, Hidenori Yamaji, Frank Rowand, Timothy Bird,
Luis Chamberlain, Theodore Ts'o, Daniel Vetter, Stephen Boyd
In-Reply-To: <20190813082336.GB17627@kroah.com>
On Tue, 2019-08-13 at 10:23 +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 13, 2019 at 08:09:15AM +0200, Knut Omang wrote:
> > and in the making::
> >
> > kunit/ (kernel only (UML))
>
> You are going to have to integrate this with kunit, to come up with a
> superset of both in the end.
Yes, I agree - getting to a unified approach has been my intention since I first brought this
up at LPC'17.
> And I do not think that kunit is only UML, it's just that seems to be
> what Brendan tests with, but should work with other arches as well.
If I get Brendan right, it is UML only now but can be extended to also support
kernels running on real hardware. Still it is kernel only, while KTF also has the
hybrid mode, where a test can have code and assertions both in user mode and kernel mode.
This is made easier and more streamlined by letting all reporting happen from user mode.
Thanks!
Knut
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Michal Hocko @ 2019-08-13 10:08 UTC (permalink / raw)
To: Jann Horn
Cc: Joel Fernandes (Google), kernel list, Alexey Dobriyan,
Andrew Morton, Borislav Petkov, Brendan Gregg, Catalin Marinas,
Christian Hansen, Daniel Colascione, fmayer, H. Peter Anvin,
Ingo Molnar, Joel Fernandes, Jonathan Corbet, Kees Cook,
kernel-team, Linux API, linux-doc, linux-fsdevel, Linux-MM,
Mike Rapoport, Minchan Kim, namhyung, Paul E. McKenney,
Robin Murphy, Roman Gushchin, Stephen Rothwell,
Suren Baghdasaryan, Thomas Gleixner, Todd Kjos, Vladimir Davydov,
Vlastimil Babka, Will Deacon
In-Reply-To: <CAG48ez0ysprvRiENhBkLeV9YPTN_MB18rbu2HDa2jsWo5FYR8g@mail.gmail.com>
On Mon 12-08-19 20:14:38, Jann Horn wrote:
> On Wed, Aug 7, 2019 at 7:16 PM Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> > The page_idle tracking feature currently requires looking up the pagemap
> > for a process followed by interacting with /sys/kernel/mm/page_idle.
> > Looking up PFN from pagemap in Android devices is not supported by
> > unprivileged process and requires SYS_ADMIN and gives 0 for the PFN.
> >
> > This patch adds support to directly interact with page_idle tracking at
> > the PID level by introducing a /proc/<pid>/page_idle file. It follows
> > the exact same semantics as the global /sys/kernel/mm/page_idle, but now
> > looking up PFN through pagemap is not needed since the interface uses
> > virtual frame numbers, and at the same time also does not require
> > SYS_ADMIN.
> >
> > In Android, we are using this for the heap profiler (heapprofd) which
> > profiles and pin points code paths which allocates and leaves memory
> > idle for long periods of time. This method solves the security issue
> > with userspace learning the PFN, and while at it is also shown to yield
> > better results than the pagemap lookup, the theory being that the window
> > where the address space can change is reduced by eliminating the
> > intermediate pagemap look up stage. In virtual address indexing, the
> > process's mmap_sem is held for the duration of the access.
>
> What happens when you use this interface on shared pages, like memory
> inherited from the zygote, library file mappings and so on? If two
> profilers ran concurrently for two different processes that both map
> the same libraries, would they end up messing up each other's data?
Yup PageIdle state is shared. That is the page_idle semantic even now
IIRC.
> Can this be used to observe which library pages other processes are
> accessing, even if you don't have access to those processes, as long
> as you can map the same libraries? I realize that there are already a
> bunch of ways to do that with side channels and such; but if you're
> adding an interface that allows this by design, it seems to me like
> something that should be gated behind some sort of privilege check.
Hmm, you need to be priviledged to get the pfn now and without that you
cannot get to any page so the new interface is weakening the rules.
Maybe we should limit setting the idle state to processes with the write
status. Or do you think that even observing idle status is useful for
practical side channel attacks? If yes, is that a problem of the
profiler which does potentially dangerous things?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
From: Ganapatrao Kulkarni @ 2019-08-13 10:55 UTC (permalink / raw)
To: Mark Rutland
Cc: Ganapatrao Kulkarni, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, will@kernel.org,
corbet@lwn.net, Jayachandran Chandrasekharan Nair, Robert Richter,
Jan Glauber
In-Reply-To: <20190812120125.GA50712@lakrids.cambridge.arm.com>
Hi Mark,
On Mon, Aug 12, 2019 at 5:31 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Tue, Jul 23, 2019 at 09:16:28AM +0000, Ganapatrao Kulkarni wrote:
> > CCPI2 is a low-latency high-bandwidth serial interface for connecting
> > ThunderX2 processors. This patch adds support to capture CCPI2 perf events.
>
> It would be worth pointing out in the commit message how the CCPI2
> counters differ from the others. I realise you have that in the body of
> patch 1, but it's critical information when reviewing this patch...
Ok, I will add in next version.
>
> >
> > Signed-off-by: Ganapatrao Kulkarni <gkulkarni@marvell.com>
> > ---
> > drivers/perf/thunderx2_pmu.c | 248 ++++++++++++++++++++++++++++++-----
> > 1 file changed, 214 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
> > index 43d76c85da56..a4e1273eafa3 100644
> > --- a/drivers/perf/thunderx2_pmu.c
> > +++ b/drivers/perf/thunderx2_pmu.c
> > @@ -17,22 +17,31 @@
> > */
> >
> > #define TX2_PMU_MAX_COUNTERS 4
>
> Shouldn't this be 8 now?
It is kept unchanged to 4(as suggested by Will), which is same for
both L3 and DMC.
For CCPI2 this macro is not used.
>
> [...]
>
> > /*
> > - * pmu on each socket has 2 uncore devices(dmc and l3c),
> > - * each device has 4 counters.
> > + * pmu on each socket has 3 uncore devices(dmc, l3ci and ccpi2),
> > + * dmc and l3c has 4 counters and ccpi2 8.
> > */
>
> How about:
>
> /*
> * Each socket has 3 uncore device associated with a PMU. The DMC and
> * L3C have 4 32-bit counters, and the CCPI2 has 8 64-bit counters.
> */
Thanks.
>
> > struct tx2_uncore_pmu {
> > struct hlist_node hpnode;
> > @@ -69,12 +86,14 @@ struct tx2_uncore_pmu {
> > int node;
> > int cpu;
> > u32 max_counters;
> > + u32 counters_mask;
> > u32 prorate_factor;
> > u32 max_events;
> > + u32 events_mask;
> > u64 hrtimer_interval;
> > void __iomem *base;
> > DECLARE_BITMAP(active_counters, TX2_PMU_MAX_COUNTERS);
>
> This bitmap isn't big enough for the 4 new counters.
>
> > - struct perf_event *events[TX2_PMU_MAX_COUNTERS];
> > + struct perf_event **events;
>
> As above, can't we bump TX2_PMU_MAX_COUNTERS to 8 rather than making
> this a dynamic allocation?
events is only relevant for L3 and DMC since they use timer callbacks.
This is done as per previous review comments.
>
> [...]
>
> > static inline u32 reg_readl(unsigned long addr)
> > {
> > return readl((void __iomem *)addr);
> > }
> >
> > +static inline u32 reg_readq(unsigned long addr)
> > +{
> > + return readq((void __iomem *)addr);
> > +}
>
> Presumably reg_readq() should return a u64.
Yes, My bad.
>
> [...]
>
> > +static void uncore_start_event_ccpi2(struct perf_event *event, int flags)
> > +{
> > + u32 emask;
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct tx2_uncore_pmu *tx2_pmu;
> > +
> > + tx2_pmu = pmu_to_tx2_pmu(event->pmu);
> > + emask = tx2_pmu->events_mask;
> > +
> > + /* Bit [09:00] to set event id, set level and type to 1 */
> > + reg_writel((3 << 10) |
>
> Do you mean that bits [11:10] are level and type?
Yes, i will change the comment.
>
> What exactly are 'level' and 'type'?
They are for other settings which are not relevant for software/kernel.
>
> Can we give those bits mnemonics?
>
> > + GET_EVENTID(event, emask), hwc->config_base);
> > + /* reset[4], enable[0] and start[1] counters */
>
> Rather than using magic numbers everywhere, please give these mnemonics,
> e.g.
>
> #define CCPI2_PERF_CTL_ENABLE BIT(0)
> #define CCPI2_PERF_CTL_START BIT(1)
> #define CCPI2_PERF_CTL_RESET BIT(4)
not used everywhere, only in this function.
I can add these macros.
>
> > + reg_writel(0x13, hwc->event_base + CCPI2_PERF_CTL);
>
> ... and then you can OR them in here:
OK
>
> ctl = CCPI2_PERF_CTL_ENABLE |
> CCPI2_PERF_CTL_START |
> CCPI2_PERF_CTL_RESET;
> reg_writel(ctl, hwc->event_base + CCPI2_PERF_CTL);
>
> [...]
>
> > @@ -456,8 +603,9 @@ static void tx2_uncore_event_start(struct perf_event *event, int flags)
> > tx2_pmu->start_event(event, flags);
> > perf_event_update_userpage(event);
> >
> > - /* Start timer for first event */
> > - if (bitmap_weight(tx2_pmu->active_counters,
> > + /* Start timer for first non ccpi2 event */
> > + if (tx2_pmu->type != PMU_TYPE_CCPI2 &&
> > + bitmap_weight(tx2_pmu->active_counters,
> > tx2_pmu->max_counters) == 1) {
> > hrtimer_start(&tx2_pmu->hrtimer,
> > ns_to_ktime(tx2_pmu->hrtimer_interval),
>
> This would be easier to read as two statements:
>
> /* No hrtimer needed with 64-bit counters */
> if (tx2_pmu->type == PMU_TYPE_CCPI2)
> return;
>
> /* Start timer for first event */
> if (bitmap_weight(tx2_pmu->active_counters,
> tx2_pmu->max_counters) != 1) {
> ...
> }
>
OK, makes sense.
> > @@ -495,7 +643,8 @@ static int tx2_uncore_event_add(struct perf_event *event, int flags)
> > if (hwc->idx < 0)
> > return -EAGAIN;
> >
> > - tx2_pmu->events[hwc->idx] = event;
> > + if (tx2_pmu->events)
> > + tx2_pmu->events[hwc->idx] = event;
>
> So this is NULL for CCPI2?
Yes.
>
> I guess we don't strictly need the if we don't have a hrtimer to update
> event counts, but this makes the code more complicated than it needs to
> be.
Yes I am using tx2_pmu->events to differentiate the type, it is NULL for CCPI2.
I can extend same to tx2_uncore_event_start().
>
> [...]
>
> > @@ -580,8 +732,12 @@ static int tx2_uncore_pmu_add_dev(struct tx2_uncore_pmu *tx2_pmu)
> > cpu_online_mask);
> >
> > tx2_pmu->cpu = cpu;
> > - hrtimer_init(&tx2_pmu->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > - tx2_pmu->hrtimer.function = tx2_hrtimer_callback;
> > + /* CCPI2 counters are 64 bit counters, no overflow */
> > + if (tx2_pmu->type != PMU_TYPE_CCPI2) {
> > + hrtimer_init(&tx2_pmu->hrtimer,
> > + CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > + tx2_pmu->hrtimer.function = tx2_hrtimer_callback;
> > + }
>
> Hmmm... this means that tx2_pmu->hrtimer.function is NULL for the CCPI2
> PMU. I think it would be best to check that when (re)programming the
> counters rather than the PMU type. For example, in
> tx2_uncore_event_start() we could have:
>
> if (!tx2_pmu->hrtimer.function)
> return;
> if (bitmap_weight(tx2_pmu->active_counters,
> tx2_pmu->max_counters) != 1) {
> ...
> }
>
Yes it is NULL for CCPI2.
Ok, I can use tx2_pmu->events instead(like other places).
> Thanks,
> Mark.
Thanks,
Ganapat
^ permalink raw reply
* Re: [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
From: Mark Rutland @ 2019-08-13 11:03 UTC (permalink / raw)
To: Ganapatrao Kulkarni
Cc: Ganapatrao Kulkarni, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, will@kernel.org,
corbet@lwn.net, Jayachandran Chandrasekharan Nair, Robert Richter,
Jan Glauber
In-Reply-To: <CAKTKpr7juHd9Bgam28LESadihFadEAevRAhc-7w3PTMYY7HLNw@mail.gmail.com>
On Tue, Aug 13, 2019 at 04:25:15PM +0530, Ganapatrao Kulkarni wrote:
> Hi Mark,
>
> On Mon, Aug 12, 2019 at 5:31 PM Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Tue, Jul 23, 2019 at 09:16:28AM +0000, Ganapatrao Kulkarni wrote:
> > > CCPI2 is a low-latency high-bandwidth serial interface for connecting
> > > ThunderX2 processors. This patch adds support to capture CCPI2 perf events.
> >
> > It would be worth pointing out in the commit message how the CCPI2
> > counters differ from the others. I realise you have that in the body of
> > patch 1, but it's critical information when reviewing this patch...
>
> Ok, I will add in next version.
> >
> > >
> > > Signed-off-by: Ganapatrao Kulkarni <gkulkarni@marvell.com>
> > > ---
> > > drivers/perf/thunderx2_pmu.c | 248 ++++++++++++++++++++++++++++++-----
> > > 1 file changed, 214 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
> > > index 43d76c85da56..a4e1273eafa3 100644
> > > --- a/drivers/perf/thunderx2_pmu.c
> > > +++ b/drivers/perf/thunderx2_pmu.c
> > > @@ -17,22 +17,31 @@
> > > */
> > >
> > > #define TX2_PMU_MAX_COUNTERS 4
> >
> > Shouldn't this be 8 now?
>
> It is kept unchanged to 4(as suggested by Will), which is same for
> both L3 and DMC.
> For CCPI2 this macro is not used.
Hmmm....
I disagree with that suggestion given that this also affects the
active_counters bitmap size (and thus it is not correctly sized as of
this patch), and it doesn't really save us much.
I think it would be better to bump this to 8 and always update the
events array, even though it will be unused for CCPI2. That's less
surprising, needs fewer special-cases, and we can use the hrtimer
function pointer alone to determine if we need to do any hrtimer work.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v7 1/2] arm64: Define Documentation/arm64/tagged-address-abi.rst
From: Dave Martin @ 2019-08-13 11:10 UTC (permalink / raw)
To: Catalin Marinas
Cc: Dave Hansen, linux-arch, linux-doc, Szabolcs Nagy,
Andrey Konovalov, Kevin Brodsky, Will Deacon, Vincenzo Frascino,
linux-arm-kernel
In-Reply-To: <20190812173611.GD62772@arrakis.emea.arm.com>
On Mon, Aug 12, 2019 at 06:36:12PM +0100, Catalin Marinas wrote:
> On Fri, Aug 09, 2019 at 07:10:18AM -0700, Dave Hansen wrote:
> > On 8/8/19 10:27 AM, Catalin Marinas wrote:
> > > On Wed, Aug 07, 2019 at 01:38:16PM -0700, Dave Hansen wrote:
> > >> Also, shouldn't this be converted over to an arch_prctl()?
> > >
> > > What do you mean by arch_prctl()? We don't have such thing, apart from
> > > maybe arch_prctl_spec_ctrl_*(). We achieve the same thing with the
> > > {SET,GET}_TAGGED_ADDR_CTRL macros. They could be renamed to
> > > arch_prctl_tagged_addr_{set,get} or something but I don't see much
> > > point.
> >
> > Silly me. We have an x86-specific:
> >
> > SYSCALL_DEFINE2(arch_prctl, int , option, unsigned long , arg2)
> >
> > I guess there's no ARM equivalent so you're stuck with the generic one.
> >
> > > What would be better (for a separate patch series) is to clean up
> > > sys_prctl() and move the arch-specific options into separate
> > > arch_prctl() under arch/*/kernel/. But it's not really for this series.
> >
> > I think it does make sense for truly arch-specific features to stay out
> > of the arch-generic prctl(). Yes, I know I've personally violated this
> > in the past. :)
>
> Maybe Dave M could revive his prctl() clean-up patches which moves the
> arch specific cases to the corresponding arch/*/ code
I'll try to take a look at it.
> > >> What is the scope of these prctl()'s? Are they thread-scoped or
> > >> process-scoped? Can two threads in the same process run with different
> > >> tagging ABI modes?
> > >
> > > Good point. They are thread-scoped and this should be made clear in the
> > > doc. Two threads can have different modes.
> > >
> > > The expectation is that this is invoked early during process start (by
> > > the dynamic loader or libc init) while in single-thread mode and
> > > subsequent threads will inherit the same mode. However, other uses are
> > > possible.
> >
> > If that's the expectation, it would be really nice to codify it.
> > Basically, you can't enable the feature if another thread is already
> > been forked off.
>
> Well, that's my expectation but I'm not a userspace developer. I don't
> think there is any good reason to prevent it. It doesn't cost us
> anything to support in the kernel, other than making the documentation
> clearer.
This came up for SVE and eventually we didn't bother, partly because the
kernel doesn't fully know what userspace is trying to do, in general.
If userspace has some kind of worker threads that run specific code,
they could legitimately interface differently with the kernel compared
with, say, the main thread. This model already exists for e.g.,
seccomp.
In any case, I think it's not up to the kernel to dictate how user
runtimes initialise themselves.
[...]
Cheers
---Dave
^ permalink raw reply
* Re: [RFC 00/19] Integration of Kernel Test Framework (KTF) into the kernel tree
From: Knut Omang @ 2019-08-13 11:29 UTC (permalink / raw)
To: Brendan Higgins
Cc: open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List,
open list:DOCUMENTATION, linux-kbuild, Shuah Khan,
Jonathan Corbet, Masahiro Yamada, Michal Marek,
Greg Kroah-Hartman, Shreyans Devendra Doshi, Alan Maguire,
Kevin Hilman, Hidenori Yamaji, Frank Rowand, Timothy Bird,
Luis Chamberlain, Theodore Ts'o, Daniel Vetter, Stephen Boyd
In-Reply-To: <CAFd5g47bK2hv6dRvqE3hOyq-FrgrR8NJo__HonHFoYOOMkWh6w@mail.gmail.com>
On Tue, 2019-08-13 at 01:17 -0700, Brendan Higgins wrote:
> On Mon, Aug 12, 2019 at 11:11 PM Knut Omang <knut.omang@oracle.com> wrote:
> [...]
> > Alan Maguire (3):
> > ktf: Implementation of ktf support for overriding function entry and return.
> > ktf: A simple debugfs interface to test results
> > ktf: Simple coverage support
> >
> > Knut Omang (16):
> > kbuild: Fixes to rules for host-cshlib and host-cxxshlib
> > ktf: Introduce the main part of the kernel side of ktf
> > ktf: Introduce a generic netlink protocol for test result communication
> > ktf: An implementation of a generic associative array container
> > ktf: Configurable context support for network info setup
> > ktf: resolve: A helper utility to aid in exposing private kernel symbols to KTF tests.
> > ktf: Add documentation for Kernel Test Framework (KTF)
> > ktf: Add a small test suite with a few tests to test KTF itself
> > ktf: Main part of user land library for executing tests
> > ktf: Integration logic for running ktf tests from googletest
> > ktf: Internal debugging facilities
> > ktf: Some simple examples
> > ktf: Some user applications to run tests
> > ktf: Toplevel ktf Makefile/makefile includes and scripts to run from kselftest
> > kselftests: Enable building ktf
> > Documentation/dev-tools: Add index entry for KTF documentation
> >
> > Documentation/dev-tools/index.rst | 1 +-
> > Documentation/dev-tools/ktf/concepts.rst | 242 +++-
> > Documentation/dev-tools/ktf/debugging.rst | 248 +++-
> > Documentation/dev-tools/ktf/examples.rst | 26 +-
> > Documentation/dev-tools/ktf/features.rst | 307 ++++-
> > Documentation/dev-tools/ktf/implementation.rst | 70 +-
> > Documentation/dev-tools/ktf/index.rst | 14 +-
> > Documentation/dev-tools/ktf/installation.rst | 73 +-
> > Documentation/dev-tools/ktf/introduction.rst | 134 ++-
> > Documentation/dev-tools/ktf/progref.rst | 144 ++-
> > scripts/Makefile.host | 17 +-
> > tools/testing/selftests/Makefile | 1 +-
> > tools/testing/selftests/ktf/Makefile | 21 +-
> > tools/testing/selftests/ktf/examples/Makefile | 17 +-
> > tools/testing/selftests/ktf/examples/h2.c | 45 +-
> > tools/testing/selftests/ktf/examples/h3.c | 84 +-
> > tools/testing/selftests/ktf/examples/h4.c | 62 +-
> > tools/testing/selftests/ktf/examples/hello.c | 38 +-
> > tools/testing/selftests/ktf/examples/kgdemo.c | 61 +-
> > tools/testing/selftests/ktf/kernel/Makefile | 15 +-
> > tools/testing/selftests/ktf/kernel/ktf.h | 604 +++++++-
> > tools/testing/selftests/ktf/kernel/ktf_context.c | 409 +++++-
> > tools/testing/selftests/ktf/kernel/ktf_cov.c | 690 ++++++++-
> > tools/testing/selftests/ktf/kernel/ktf_cov.h | 94 +-
> > tools/testing/selftests/ktf/kernel/ktf_debugfs.c | 356 ++++-
> > tools/testing/selftests/ktf/kernel/ktf_debugfs.h | 34 +-
> > tools/testing/selftests/ktf/kernel/ktf_map.c | 261 +++-
> > tools/testing/selftests/ktf/kernel/ktf_map.h | 154 ++-
> > tools/testing/selftests/ktf/kernel/ktf_netctx.c | 132 ++-
> > tools/testing/selftests/ktf/kernel/ktf_netctx.h | 64 +-
> > tools/testing/selftests/ktf/kernel/ktf_nl.c | 516 ++++++-
> > tools/testing/selftests/ktf/kernel/ktf_nl.h | 15 +-
> > tools/testing/selftests/ktf/kernel/ktf_override.c | 45 +-
> > tools/testing/selftests/ktf/kernel/ktf_override.h | 15 +-
> > tools/testing/selftests/ktf/kernel/ktf_test.c | 397 +++++-
> > tools/testing/selftests/ktf/kernel/ktf_test.h | 381 ++++-
> > tools/testing/selftests/ktf/kernel/ktf_unlproto.h | 105 +-
> > tools/testing/selftests/ktf/lib/Makefile | 21 +-
> > tools/testing/selftests/ktf/lib/ktf.h | 114 +-
> > tools/testing/selftests/ktf/lib/ktf_debug.cc | 20 +-
> > tools/testing/selftests/ktf/lib/ktf_debug.h | 59 +-
> > tools/testing/selftests/ktf/lib/ktf_int.cc | 1031 ++++++++++++-
> > tools/testing/selftests/ktf/lib/ktf_int.h | 84 +-
> > tools/testing/selftests/ktf/lib/ktf_run.cc | 177 ++-
> > tools/testing/selftests/ktf/lib/ktf_unlproto.c | 21 +-
> > tools/testing/selftests/ktf/scripts/ktf_syms.mk | 16 +-
> > tools/testing/selftests/ktf/scripts/resolve | 188 ++-
> > tools/testing/selftests/ktf/scripts/runtests.mk | 3 +-
> > tools/testing/selftests/ktf/scripts/runtests.sh | 100 +-
> > tools/testing/selftests/ktf/scripts/top_make.mk | 14 +-
> > tools/testing/selftests/ktf/selftest/Makefile | 17 +-
> > tools/testing/selftests/ktf/selftest/context.c | 149 ++-
> > tools/testing/selftests/ktf/selftest/context.h | 15 +-
> > tools/testing/selftests/ktf/selftest/context_self.h | 34 +-
> > tools/testing/selftests/ktf/selftest/hybrid.c | 35 +-
> > tools/testing/selftests/ktf/selftest/hybrid.h | 24 +-
> > tools/testing/selftests/ktf/selftest/hybrid_self.h | 27 +-
> > tools/testing/selftests/ktf/selftest/ktf_syms.txt | 17 +-
> > tools/testing/selftests/ktf/selftest/self.c | 661 ++++++++-
> > tools/testing/selftests/ktf/user/Makefile | 26 +-
> > tools/testing/selftests/ktf/user/hybrid.cc | 39 +-
> > tools/testing/selftests/ktf/user/ktfcov.cc | 68 +-
> > tools/testing/selftests/ktf/user/ktfrun.cc | 20 +-
> > tools/testing/selftests/ktf/user/ktftest.cc | 46 +-
> > 64 files changed, 8909 insertions(+), 9 deletions(-)
>
> It also looks like all your test code lives outside of the kernel
> source dir. I take it you intend for tests to live in
> tools/testing/selftests/ktf/ ?
Good point,
with this RFC it would be just to add another directory under ktf/
but this was just to find a simple way to integrate it below selftests,
without interfering with the current structure.
I imagine a tighter integration/unification between normal Kbuild targets and selftests
targets that also took kernel module building into consideration would be a better solution.
I think this is a good topic for discussion.
As I indicate above, I think it is problematic that test code has to be explicitly
configured in as we configure code features of the kernel, which changes the "logical"
kernel we build.
So some more "native" support for test modules are desired, IMHO.
Knut
> [...]
^ permalink raw reply
* Re: [PATCH v1] kernel-doc: Allow anonymous enum
From: Andy Shevchenko @ 2019-08-13 12:19 UTC (permalink / raw)
To: Jonathan Corbet; +Cc: linux-doc, Mika Westerberg, Linus Walleij
In-Reply-To: <20190812151317.746379b2@lwn.net>
On Mon, Aug 12, 2019 at 03:13:17PM -0600, Jonathan Corbet wrote:
> On Mon, 12 Aug 2019 19:06:31 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > In C is a valid construction to have an anonymous enumerator.
> >
> > Though we have now:
> >
> > drivers/pinctrl/intel/pinctrl-intel.c:240: error: Cannot parse enum!
> >
> > Support it in the kernel-doc script.
>
> So I don't get this error; I guess the only anonymous enum of interest has
> yet to find its way into the mainline.
The other one is already in mainline (drivers/remoteproc/omap_remoteproc.h),
but suddenly it misses the second * in the header of kernel-doc comment
(perhaps to avoid above error, or for another reason).
>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > scripts/kernel-doc | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> > index 6b03012750da..079502bcc5a3 100755
> > --- a/scripts/kernel-doc
> > +++ b/scripts/kernel-doc
> > @@ -1245,7 +1245,7 @@ sub dump_enum($$) {
> > # strip #define macros inside enums
> > $x =~ s@#\s*((define|ifdef)\s+|endif)[^;]*;@@gos;
> >
> > - if ($x =~ /enum\s+(\w+)\s*\{(.*)\}/) {
> > + if ($x =~ /enum\s+(\w*)\s*\{(.*)\}/) {
>
> Ah the joy of regexes...
>
> Applied, thanks.
Thanks!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v1] kernel-doc: Allow anonymous enum
From: Andy Shevchenko @ 2019-08-13 12:31 UTC (permalink / raw)
To: Jonathan Corbet; +Cc: linux-doc, Mika Westerberg, Linus Walleij
In-Reply-To: <20190813121912.GV30120@smile.fi.intel.com>
On Tue, Aug 13, 2019 at 03:19:12PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 12, 2019 at 03:13:17PM -0600, Jonathan Corbet wrote:
> > On Mon, 12 Aug 2019 19:06:31 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >
> > > In C is a valid construction to have an anonymous enumerator.
> > >
> > > Though we have now:
> > >
> > > drivers/pinctrl/intel/pinctrl-intel.c:240: error: Cannot parse enum!
> > >
> > > Support it in the kernel-doc script.
> >
> > So I don't get this error; I guess the only anonymous enum of interest has
> > yet to find its way into the mainline.
>
> The other one is already in mainline (drivers/remoteproc/omap_remoteproc.h),
> but suddenly it misses the second * in the header of kernel-doc comment
> (perhaps to avoid above error, or for another reason).
Actually more without full description
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h:186:
Or with format which messes name with description
drivers/misc/mic/cosm/cosm_main.h:19:
include/linux/amba/pl022.h:166:
Or above variants together
drivers/pinctrl/pinctrl-rockchip.c:86:
drivers/pinctrl/pinctrl-rockchip.c:98:
Noticed also broken references like
drivers/scsi/isci/port.c:1193:
drivers/usb/host/ehci-timer.c:381:
where enum mentioned without leading &.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] driver/core: Fix build error when SRCU and lockdep disabled
From: Joel Fernandes @ 2019-08-13 13:39 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, kernel-team, kbuild test robot, Josh Triplett,
Lai Jiangshan, linux-doc, Mathieu Desnoyers, Paul E. McKenney,
Rafael J. Wysocki, rcu, Steven Rostedt
In-Reply-To: <20190813060540.GE6670@kroah.com>
On Tue, Aug 13, 2019 at 08:05:40AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 12, 2019 at 05:49:17PM -0400, Joel Fernandes (Google) wrote:
> > Check if lockdep lock checking is disabled. If so, then do not define
> > device_links_read_lock_held(). It is used only from places where lockdep
> > checking is enabled.
> >
> > Also fix a bug where I was not checking dep_map. Previously, I did not
> > test !SRCU configs so this got missed. Now it is sorted.
> >
> > Link: https://lore.kernel.org/lkml/201908080026.WSAFx14k%25lkp@intel.com/
> > Fixes: c9e4d3a2fee8 ("acpi: Use built-in RCU list checking for acpi_ioremaps list")
> > (Based on RCU's dev branch)
> >
> > Cc: kernel-team@android.com
> > Cc: kbuild test robot <lkp@intel.com>,
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
> > Cc: Josh Triplett <josh@joshtriplett.org>,
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>,
> > Cc: linux-doc@vger.kernel.org,
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
> > Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>,
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
> > Cc: rcu@vger.kernel.org,
> > Cc: Steven Rostedt <rostedt@goodmis.org>,
> >
> > Reported-by: kbuild test robot <lkp@intel.com>
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> Nit, drop those blank lines above, should all be in one big "block">
Ok.
> > drivers/base/core.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 32cf83d1c744..c22271577c84 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -97,10 +97,12 @@ void device_links_read_unlock(int not_used)
> > up_read(&device_links_lock);
> > }
> >
> > +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> > int device_links_read_lock_held(void)
> > {
> > - return lock_is_held(&device_links_lock);
> > + return lock_is_held(&(device_links_lock.dep_map));
> > }
> > +#endif
>
> I don't know what the original code looks like here, but I'm guessing
> that some .h file will need to be fixed up as you are just preventing
> this function from ever being present without that option enabled?
No, it doesn't. I already thought about that and it is not an issue. I know
this may be confusing because the patch I am fixing is not yet in mainline
but in -rcu dev branch, however I did CC you on that RCU patch before but
understandably it is not in the series so it was harder to review.
Let me explain, the lock checking facility that this patch uses is here:
https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=28875945ba98d1b47a8a706812b6494d165bb0a0
If you see, the CONFIG_PROVE_RCU_LIST defines an alternate __list_check_rcu()
which is just a NOOP if CONFIG_PROVE_RCU_LIST=n.
CONFIG_PROVE_RCU_LIST depends on CONFIG_PROVE_RCU which is def_bool on
CONFIG_PROVE_LOCKING which selects CONFIG_DEBUG_LOCK_ALLOC.
So there cannot be a scenario where CONFIG_PROVE_RCU_LIST is enabled but
CONFIG_DEBUG_LOCK_ALLOC is disabled.
To verify this, one could clone the RCU tree's dev branch and do this:
Initially PROVE_RCU_LIST is not in config:
joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
Neither is DEBUG_LOCK_ALLOC:
joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock .config
# CONFIG_DEBUG_LOCK_ALLOC is not set
Enable all configs:
joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_RCU_EXPERT
joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_LOCKING
joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_RCU
joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_RCU_LIST
joelaf@joelaf:~/repo/linux-master$ make olddefconfig
DEBUG_LOCK_ALLOC shows up:
joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock_all .config
CONFIG_DEBUG_LOCK_ALLOC=y
So does PROVE_RCU options:
joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
CONFIG_PROVE_RCU=y
CONFIG_PROVE_RCU_LIST=y
Now, force disable DEBUG_LOCK_ALLOC:
joelaf@joelaf:~/repo/linux-master$ ./scripts/config -d CONFIG_DEBUG_LOCK_ALLOC
joelaf@joelaf:~/repo/linux-master$ make olddefconfig
Options are still enabled:
joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock .config
CONFIG_DEBUG_LOCK_ALLOC=y
joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
CONFIG_PROVE_RCU=y
CONFIG_PROVE_RCU_LIST=y
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v2] driver/core: Fix build error when SRCU and lockdep disabled
From: Joel Fernandes @ 2019-08-13 13:40 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, kernel-team, kbuild test robot, Josh Triplett,
Lai Jiangshan, linux-doc, Mathieu Desnoyers, Paul E. McKenney,
Rafael J. Wysocki, rcu, Steven Rostedt
In-Reply-To: <20190813133905.GA258732@google.com>
On Tue, Aug 13, 2019 at 09:39:05AM -0400, Joel Fernandes wrote:
[snip]
> > > drivers/base/core.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 32cf83d1c744..c22271577c84 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -97,10 +97,12 @@ void device_links_read_unlock(int not_used)
> > > up_read(&device_links_lock);
> > > }
> > >
> > > +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> > > int device_links_read_lock_held(void)
> > > {
> > > - return lock_is_held(&device_links_lock);
> > > + return lock_is_held(&(device_links_lock.dep_map));
> > > }
> > > +#endif
> >
> > I don't know what the original code looks like here, but I'm guessing
> > that some .h file will need to be fixed up as you are just preventing
> > this function from ever being present without that option enabled?
>
> No, it doesn't. I already thought about that and it is not an issue. I know
> this may be confusing because the patch I am fixing is not yet in mainline
> but in -rcu dev branch, however I did CC you on that RCU patch before but
> understandably it is not in the series so it was harder to review.
>
> Let me explain, the lock checking facility that this patch uses is here:
> https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=28875945ba98d1b47a8a706812b6494d165bb0a0
>
> If you see, the CONFIG_PROVE_RCU_LIST defines an alternate __list_check_rcu()
> which is just a NOOP if CONFIG_PROVE_RCU_LIST=n.
>
> CONFIG_PROVE_RCU_LIST depends on CONFIG_PROVE_RCU which is def_bool on
> CONFIG_PROVE_LOCKING which selects CONFIG_DEBUG_LOCK_ALLOC.
>
> So there cannot be a scenario where CONFIG_PROVE_RCU_LIST is enabled but
> CONFIG_DEBUG_LOCK_ALLOC is disabled.
>
> To verify this, one could clone the RCU tree's dev branch and do this:
>
> Initially PROVE_RCU_LIST is not in config:
>
> joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
>
> Neither is DEBUG_LOCK_ALLOC:
>
> joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock .config
> # CONFIG_DEBUG_LOCK_ALLOC is not set
>
> Enable all configs:
> joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_RCU_EXPERT
> joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_LOCKING
> joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_RCU
> joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_RCU_LIST
> joelaf@joelaf:~/repo/linux-master$ make olddefconfig
>
> DEBUG_LOCK_ALLOC shows up:
>
> joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock_all .config
> CONFIG_DEBUG_LOCK_ALLOC=y
>
> So does PROVE_RCU options:
> joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
> CONFIG_PROVE_RCU=y
> CONFIG_PROVE_RCU_LIST=y
>
> Now, force disable DEBUG_LOCK_ALLOC:
>
> joelaf@joelaf:~/repo/linux-master$ ./scripts/config -d CONFIG_DEBUG_LOCK_ALLOC
>
> joelaf@joelaf:~/repo/linux-master$ make olddefconfig
>
> Options are still enabled:
>
> joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock .config
> CONFIG_DEBUG_LOCK_ALLOC=y
> joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
> CONFIG_PROVE_RCU=y
> CONFIG_PROVE_RCU_LIST=y
Also, appreciate if you could Ack the fix ;-) (assuming the newlines in
commit message are fixed).
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Joel Fernandes @ 2019-08-13 13:51 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, linux-kernel, Alexey Dobriyan, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen, dancol, fmayer,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Kees Cook,
kernel-team, linux-api, linux-doc, linux-fsdevel, linux-mm,
Mike Rapoport, minchan, namhyung, paulmck, Robin Murphy,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190813091430.GE17933@dhcp22.suse.cz>
On Tue, Aug 13, 2019 at 11:14:30AM +0200, Michal Hocko wrote:
> On Mon 12-08-19 10:56:20, Joel Fernandes wrote:
> > On Thu, Aug 08, 2019 at 10:00:44AM +0200, Michal Hocko wrote:
> > > On Wed 07-08-19 17:31:05, Joel Fernandes wrote:
> > > > On Wed, Aug 07, 2019 at 01:58:40PM -0700, Andrew Morton wrote:
> > > > > On Wed, 7 Aug 2019 16:45:30 -0400 Joel Fernandes <joel@joelfernandes.org> wrote:
> > > > >
> > > > > > On Wed, Aug 07, 2019 at 01:04:02PM -0700, Andrew Morton wrote:
> > > > > > > On Wed, 7 Aug 2019 13:15:54 -0400 "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote:
> > > > > > >
> > > > > > > > In Android, we are using this for the heap profiler (heapprofd) which
> > > > > > > > profiles and pin points code paths which allocates and leaves memory
> > > > > > > > idle for long periods of time. This method solves the security issue
> > > > > > > > with userspace learning the PFN, and while at it is also shown to yield
> > > > > > > > better results than the pagemap lookup, the theory being that the window
> > > > > > > > where the address space can change is reduced by eliminating the
> > > > > > > > intermediate pagemap look up stage. In virtual address indexing, the
> > > > > > > > process's mmap_sem is held for the duration of the access.
> > > > > > >
> > > > > > > So is heapprofd a developer-only thing? Is heapprofd included in
> > > > > > > end-user android loads? If not then, again, wouldn't it be better to
> > > > > > > make the feature Kconfigurable so that Android developers can enable it
> > > > > > > during development then disable it for production kernels?
> > > > > >
> > > > > > Almost all of this code is already configurable with
> > > > > > CONFIG_IDLE_PAGE_TRACKING. If you disable it, then all of this code gets
> > > > > > disabled.
> > > > > >
> > > > > > Or are you referring to something else that needs to be made configurable?
> > > > >
> > > > > Yes - the 300+ lines of code which this patchset adds!
> > > > >
> > > > > The impacted people will be those who use the existing
> > > > > idle-page-tracking feature but who will not use the new feature. I
> > > > > guess we can assume this set is small...
> > > >
> > > > Yes, I think this set should be small. The code size increase of page_idle.o
> > > > is from ~1KB to ~2KB. Most of the extra space is consumed by
> > > > page_idle_proc_generic() function which this patch adds. I don't think adding
> > > > another CONFIG option to disable this while keeping existing
> > > > CONFIG_IDLE_PAGE_TRACKING enabled, is worthwhile but I am open to the
> > > > addition of such an option if anyone feels strongly about it. I believe that
> > > > once this patch is merged, most like this new interface being added is what
> > > > will be used more than the old interface (for some of the usecases) so it
> > > > makes sense to keep it alive with CONFIG_IDLE_PAGE_TRACKING.
> > >
> > > I would tend to agree with Joel here. The functionality falls into an
> > > existing IDLE_PAGE_TRACKING config option quite nicely. If there really
> > > are users who want to save some space and this is standing in the way
> > > then they can easily add a new config option with some justification so
> > > the savings are clear. Without that an additional config simply adds to
> > > the already existing configurability complexity and balkanization.
> >
> > Michal, Andrew, Minchan,
> >
> > Would you have any other review comments on the v5 series? This is just a new
> > interface that does not disrupt existing users of the older page-idle
> > tracking, so as such it is a safe change (as in, doesn't change existing
> > functionality except for the draining bug fix).
>
> I hope to find some more time to finish the review but let me point out
> that "it's new it is regression safe" is not really a great argument for
> a new user visible API.
Actually, I think you misunderstood me and took it out of context. I never
intended to say "it is regression safe". I meant to say it is "low risk", as
in that in all likelihood should not be hurting *existing users* of the *old
interface*. Also as you know, it has been tested.
> If the API is flawed then this is likely going
> to kick us later and will be hard to fix. I am still not convinced about
> the swap part of the thing TBH.
Ok, then let us discuss it. As I mentioned before, without this we lose the
access information due to MADVISE or swapping. Minchan and Konstantin both
suggested it that's why I also added it (other than me also realizing that it
is neeed). For x86, it uses existing bits in pte so it is not adding any more
bits. For arm64, it uses unused bits that the hardware cannot use. So I
don't see why this is an issue to you.
thanks,
- Joel
^ permalink raw reply
* Re: [RFC 01/19] kbuild: Fixes to rules for host-cshlib and host-cxxshlib
From: Masahiro Yamada @ 2019-08-13 14:01 UTC (permalink / raw)
To: Knut Omang
Cc: open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List,
open list:DOCUMENTATION, Linux Kbuild mailing list, Shuah Khan,
Jonathan Corbet, Michal Marek, Greg Kroah-Hartman,
Shreyans Devendra Doshi, Alan Maguire, Brendan Higgins,
Kevin Hilman, Hidenori Yamaji, Frank Rowand, Timothy Bird,
Luis Chamberlain, Theodore Ts'o, Daniel Vetter, Stephen Boyd
In-Reply-To: <be2c361eac49ded2848b2a555b75e30cc3c24e71.1565676440.git-series.knut.omang@oracle.com>
On Tue, Aug 13, 2019 at 3:13 PM Knut Omang <knut.omang@oracle.com> wrote:
>
> C++ libraries interfacing to C APIs might sometimes need some glue
> logic more easily written in C.
> Allow a C++ library to also contain 0 or more C objects.
>
> Also fix rules for both C and C++ shared libraries:
> - C++ shared libraries depended on .c instead of .cc files
> - Rules were referenced as -objs instead of the intended
> -cobjs and -cxxobjs following the pattern from hostprogs*.
>
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
How is this patch related to the rest of this series?
This patch breaks GCC-plugins.
Did you really compile-test this patch before the submission?
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Michal Hocko @ 2019-08-13 14:14 UTC (permalink / raw)
To: Joel Fernandes
Cc: Andrew Morton, linux-kernel, Alexey Dobriyan, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen, dancol, fmayer,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Kees Cook,
kernel-team, linux-api, linux-doc, linux-fsdevel, linux-mm,
Mike Rapoport, minchan, namhyung, paulmck, Robin Murphy,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190813135152.GC258732@google.com>
On Tue 13-08-19 09:51:52, Joel Fernandes wrote:
> On Tue, Aug 13, 2019 at 11:14:30AM +0200, Michal Hocko wrote:
> > On Mon 12-08-19 10:56:20, Joel Fernandes wrote:
> > > On Thu, Aug 08, 2019 at 10:00:44AM +0200, Michal Hocko wrote:
> > > > On Wed 07-08-19 17:31:05, Joel Fernandes wrote:
> > > > > On Wed, Aug 07, 2019 at 01:58:40PM -0700, Andrew Morton wrote:
> > > > > > On Wed, 7 Aug 2019 16:45:30 -0400 Joel Fernandes <joel@joelfernandes.org> wrote:
> > > > > >
> > > > > > > On Wed, Aug 07, 2019 at 01:04:02PM -0700, Andrew Morton wrote:
> > > > > > > > On Wed, 7 Aug 2019 13:15:54 -0400 "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote:
> > > > > > > >
> > > > > > > > > In Android, we are using this for the heap profiler (heapprofd) which
> > > > > > > > > profiles and pin points code paths which allocates and leaves memory
> > > > > > > > > idle for long periods of time. This method solves the security issue
> > > > > > > > > with userspace learning the PFN, and while at it is also shown to yield
> > > > > > > > > better results than the pagemap lookup, the theory being that the window
> > > > > > > > > where the address space can change is reduced by eliminating the
> > > > > > > > > intermediate pagemap look up stage. In virtual address indexing, the
> > > > > > > > > process's mmap_sem is held for the duration of the access.
> > > > > > > >
> > > > > > > > So is heapprofd a developer-only thing? Is heapprofd included in
> > > > > > > > end-user android loads? If not then, again, wouldn't it be better to
> > > > > > > > make the feature Kconfigurable so that Android developers can enable it
> > > > > > > > during development then disable it for production kernels?
> > > > > > >
> > > > > > > Almost all of this code is already configurable with
> > > > > > > CONFIG_IDLE_PAGE_TRACKING. If you disable it, then all of this code gets
> > > > > > > disabled.
> > > > > > >
> > > > > > > Or are you referring to something else that needs to be made configurable?
> > > > > >
> > > > > > Yes - the 300+ lines of code which this patchset adds!
> > > > > >
> > > > > > The impacted people will be those who use the existing
> > > > > > idle-page-tracking feature but who will not use the new feature. I
> > > > > > guess we can assume this set is small...
> > > > >
> > > > > Yes, I think this set should be small. The code size increase of page_idle.o
> > > > > is from ~1KB to ~2KB. Most of the extra space is consumed by
> > > > > page_idle_proc_generic() function which this patch adds. I don't think adding
> > > > > another CONFIG option to disable this while keeping existing
> > > > > CONFIG_IDLE_PAGE_TRACKING enabled, is worthwhile but I am open to the
> > > > > addition of such an option if anyone feels strongly about it. I believe that
> > > > > once this patch is merged, most like this new interface being added is what
> > > > > will be used more than the old interface (for some of the usecases) so it
> > > > > makes sense to keep it alive with CONFIG_IDLE_PAGE_TRACKING.
> > > >
> > > > I would tend to agree with Joel here. The functionality falls into an
> > > > existing IDLE_PAGE_TRACKING config option quite nicely. If there really
> > > > are users who want to save some space and this is standing in the way
> > > > then they can easily add a new config option with some justification so
> > > > the savings are clear. Without that an additional config simply adds to
> > > > the already existing configurability complexity and balkanization.
> > >
> > > Michal, Andrew, Minchan,
> > >
> > > Would you have any other review comments on the v5 series? This is just a new
> > > interface that does not disrupt existing users of the older page-idle
> > > tracking, so as such it is a safe change (as in, doesn't change existing
> > > functionality except for the draining bug fix).
> >
> > I hope to find some more time to finish the review but let me point out
> > that "it's new it is regression safe" is not really a great argument for
> > a new user visible API.
>
> Actually, I think you misunderstood me and took it out of context. I never
> intended to say "it is regression safe". I meant to say it is "low risk", as
> in that in all likelihood should not be hurting *existing users* of the *old
> interface*. Also as you know, it has been tested.
Yeah, misreading on my end.
> > If the API is flawed then this is likely going
> > to kick us later and will be hard to fix. I am still not convinced about
> > the swap part of the thing TBH.
>
> Ok, then let us discuss it. As I mentioned before, without this we lose the
> access information due to MADVISE or swapping. Minchan and Konstantin both
> suggested it that's why I also added it (other than me also realizing that it
> is neeed).
I have described my concerns about the general idle bit behavior after
unmapping pointing to discrepancy with !anon pages. And I believe those
haven't been addressed yet. Besides that I am still not seeing any
description of the usecase that would suffer from the lack of the
functionality in changelogs.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox