* Re: [PATCH 7/9] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1)
From: Paul E. McKenney @ 2019-07-16 18:42 UTC (permalink / raw)
To: Joel Fernandes
Cc: Bjorn Helgaas, linux-kernel, Alexey Kuznetsov, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716040303.GA73383@google.com>
On Tue, Jul 16, 2019 at 12:03:03AM -0400, Joel Fernandes wrote:
> On Mon, Jul 15, 2019 at 03:02:35PM -0500, Bjorn Helgaas wrote:
> > On Mon, Jul 15, 2019 at 10:37:03AM -0400, Joel Fernandes (Google) wrote:
> > > The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a
> > > reader-lock held, because the pci_mmcfg_lock is already held. Make this
> > > known to the list macro so that it fixes new lockdep warnings that
> > > trigger due to lockdep checks added to list_for_each_entry_rcu().
> > >
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >
> > Ingo takes care of most patches to this file, but FWIW,
> >
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> Thanks.
>
> > I would personally prefer if you capitalized the subject to match the
> > "x86/PCI:" convention that's used fairly consistently in
> > arch/x86/pci/.
> >
> > Also, I didn't apply this to be sure, but it looks like this might
> > make a line or two wider than 80 columns, which I would rewrap if I
> > were applying this.
>
> Updated below is the patch with the nits corrected:
I am OK with this going either way, but it does depend on an earlier
patch.
Thanx, Paul
> ---8<-----------------------
>
> >From 73fab09d7e33ca2110c24215f8ed428c12625dbe Mon Sep 17 00:00:00 2001
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Date: Sat, 1 Jun 2019 15:05:49 -0400
> Subject: [PATCH] x86/PCI: Pass lockdep condition to pcm_mmcfg_list iterator
> (v1)
>
> The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a
> reader-lock held, because the pci_mmcfg_lock is already held. Make this
> known to the list macro so that it fixes new lockdep warnings that
> trigger due to lockdep checks added to list_for_each_entry_rcu().
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> arch/x86/pci/mmconfig-shared.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> index 7389db538c30..9e3250ec5a37 100644
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -29,6 +29,7 @@
> static bool pci_mmcfg_running_state;
> static bool pci_mmcfg_arch_init_failed;
> static DEFINE_MUTEX(pci_mmcfg_lock);
> +#define pci_mmcfg_lock_held() lock_is_held(&(pci_mmcfg_lock).dep_map)
>
> LIST_HEAD(pci_mmcfg_list);
>
> @@ -54,7 +55,8 @@ static void list_add_sorted(struct pci_mmcfg_region *new)
> struct pci_mmcfg_region *cfg;
>
> /* keep list sorted by segment and starting bus number */
> - list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
> + list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list,
> + pci_mmcfg_lock_held()) {
> if (cfg->segment > new->segment ||
> (cfg->segment == new->segment &&
> cfg->start_bus >= new->start_bus)) {
> @@ -118,7 +120,8 @@ struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus)
> {
> struct pci_mmcfg_region *cfg;
>
> - list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
> + list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list
> + pci_mmcfg_lock_held())
> if (cfg->segment == segment &&
> cfg->start_bus <= bus && bus <= cfg->end_bus)
> return cfg;
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH 6/9] workqueue: Convert for_each_wq to use built-in list check (v2)
From: Paul E. McKenney @ 2019-07-16 18:41 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190715143705.117908-7-joel@joelfernandes.org>
On Mon, Jul 15, 2019 at 10:37:02AM -0400, Joel Fernandes (Google) wrote:
> list_for_each_entry_rcu now has support to check for RCU reader sections
> as well as lock. Just use the support in it, instead of explictly
> checking in the caller.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
We need an ack from one of the subsystem maintainers on this one.
Thanx, Paul
> ---
> kernel/workqueue.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 601d61150b65..e882477ebf6e 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -364,11 +364,6 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
> !lockdep_is_held(&wq_pool_mutex), \
> "RCU or wq_pool_mutex should be held")
>
> -#define assert_rcu_or_wq_mutex(wq) \
> - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
> - !lockdep_is_held(&wq->mutex), \
> - "RCU or wq->mutex should be held")
> -
> #define assert_rcu_or_wq_mutex_or_pool_mutex(wq) \
> RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
> !lockdep_is_held(&wq->mutex) && \
> @@ -425,9 +420,8 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
> * ignored.
> */
> #define for_each_pwq(pwq, wq) \
> - list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node) \
> - if (({ assert_rcu_or_wq_mutex(wq); false; })) { } \
> - else
> + list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node, \
> + lock_is_held(&(wq->mutex).dep_map))
>
> #ifdef CONFIG_DEBUG_OBJECTS_WORK
>
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH 8/9] acpi: Use built-in RCU list checking for acpi_ioremaps list (v1)
From: Paul E. McKenney @ 2019-07-16 18:43 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190715143705.117908-9-joel@joelfernandes.org>
On Mon, Jul 15, 2019 at 10:37:04AM -0400, Joel Fernandes (Google) wrote:
> list_for_each_entry_rcu has built-in RCU and lock checking. Make use of
> it for acpi_ioremaps list traversal.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Given that Rafael acked it, this one looks ready.
Thanx, Paul
> ---
> drivers/acpi/osl.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 9c0edf2fc0dd..2f9d0d20b836 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -14,6 +14,7 @@
> #include <linux/slab.h>
> #include <linux/mm.h>
> #include <linux/highmem.h>
> +#include <linux/lockdep.h>
> #include <linux/pci.h>
> #include <linux/interrupt.h>
> #include <linux/kmod.h>
> @@ -80,6 +81,7 @@ struct acpi_ioremap {
>
> static LIST_HEAD(acpi_ioremaps);
> static DEFINE_MUTEX(acpi_ioremap_lock);
> +#define acpi_ioremap_lock_held() lock_is_held(&acpi_ioremap_lock.dep_map)
>
> static void __init acpi_request_region (struct acpi_generic_address *gas,
> unsigned int length, char *desc)
> @@ -206,7 +208,7 @@ acpi_map_lookup(acpi_physical_address phys, acpi_size size)
> {
> struct acpi_ioremap *map;
>
> - list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> + list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
> if (map->phys <= phys &&
> phys + size <= map->phys + map->size)
> return map;
> @@ -249,7 +251,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
> {
> struct acpi_ioremap *map;
>
> - list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> + list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
> if (map->virt <= virt &&
> virt + size <= map->virt + map->size)
> return map;
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
From: Joel Fernandes @ 2019-07-16 18:46 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716183833.GD14271@linux.ibm.com>
On Tue, Jul 16, 2019 at 11:38:33AM -0700, Paul E. McKenney wrote:
> On Mon, Jul 15, 2019 at 10:36:58AM -0400, Joel Fernandes (Google) wrote:
> > This patch adds support for checking RCU reader sections in list
> > traversal macros. Optionally, if the list macro is called under SRCU or
> > other lock/mutex protection, then appropriate lockdep expressions can be
> > passed to make the checks pass.
> >
> > Existing list_for_each_entry_rcu() invocations don't need to pass the
> > optional fourth argument (cond) unless they are under some non-RCU
> > protection and needs to make lockdep check pass.
> >
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> Now that I am on the correct version, again please fold in the checks
> for the extra argument. The ability to have an optional argument looks
> quite helpful, especially when compared to growing the RCU API!
I did fold this and replied with a pull request URL based on /dev branch. But
we can hold off on the pull requests until we decide on the below comments:
> A few more things below.
> > ---
> > include/linux/rculist.h | 28 ++++++++++++++++++++-----
> > include/linux/rcupdate.h | 7 +++++++
> > kernel/rcu/Kconfig.debug | 11 ++++++++++
> > kernel/rcu/update.c | 44 ++++++++++++++++++++++++----------------
> > 4 files changed, 67 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > index e91ec9ddcd30..1048160625bb 100644
> > --- a/include/linux/rculist.h
> > +++ b/include/linux/rculist.h
> > @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> > */
> > #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next)))
> >
> > +/*
> > + * Check during list traversal that we are within an RCU reader
> > + */
> > +
> > +#ifdef CONFIG_PROVE_RCU_LIST
>
> This new Kconfig option is OK temporarily, but unless there is reason to
> fear malfunction that a few weeks of rcutorture, 0day, and -next won't
> find, it would be better to just use CONFIG_PROVE_RCU. The overall goal
> is to reduce the number of RCU knobs rather than grow them, must though
> history might lead one to believe otherwise. :-/
If you want, we can try to drop this option and just use PROVE_RCU however I
must say there may be several warnings that need to be fixed in a short
period of time (even a few weeks may be too short) considering the 1000+
uses of RCU lists.
But I don't mind dropping it and it may just accelerate the fixing up of all
callers.
> > +#define __list_check_rcu(dummy, cond, ...) \
> > + ({ \
> > + RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \
> > + "RCU-list traversed in non-reader section!"); \
> > + })
> > +#else
> > +#define __list_check_rcu(dummy, cond, ...) ({})
> > +#endif
> > +
> > /*
> > * Insert a new entry between two known consecutive entries.
> > *
> > @@ -343,14 +357,16 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> > * @pos: the type * to use as a loop cursor.
> > * @head: the head for your list.
> > * @member: the name of the list_head within the struct.
> > + * @cond: optional lockdep expression if called from non-RCU protection.
> > *
> > * This list-traversal primitive may safely run concurrently with
> > * the _rcu list-mutation primitives such as list_add_rcu()
> > * as long as the traversal is guarded by rcu_read_lock().
> > */
> > -#define list_for_each_entry_rcu(pos, head, member) \
> > - for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> > - &pos->member != (head); \
> > +#define list_for_each_entry_rcu(pos, head, member, cond...) \
> > + for (__list_check_rcu(dummy, ## cond, 0), \
> > + pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> > + &pos->member != (head); \
> > pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> >
> > /**
> > @@ -616,13 +632,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> > * @pos: the type * to use as a loop cursor.
> > * @head: the head for your list.
> > * @member: the name of the hlist_node within the struct.
> > + * @cond: optional lockdep expression if called from non-RCU protection.
> > *
> > * This list-traversal primitive may safely run concurrently with
> > * the _rcu list-mutation primitives such as hlist_add_head_rcu()
> > * as long as the traversal is guarded by rcu_read_lock().
> > */
> > -#define hlist_for_each_entry_rcu(pos, head, member) \
> > - for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > +#define hlist_for_each_entry_rcu(pos, head, member, cond...) \
> > + for (__list_check_rcu(dummy, ## cond, 0), \
> > + pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > typeof(*(pos)), member); \
> > pos; \
> > pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 8f7167478c1d..f3c29efdf19a 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -221,6 +221,7 @@ int debug_lockdep_rcu_enabled(void);
> > int rcu_read_lock_held(void);
> > int rcu_read_lock_bh_held(void);
> > int rcu_read_lock_sched_held(void);
> > +int rcu_read_lock_any_held(void);
> >
> > #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> >
> > @@ -241,6 +242,12 @@ static inline int rcu_read_lock_sched_held(void)
> > {
> > return !preemptible();
> > }
> > +
> > +static inline int rcu_read_lock_any_held(void)
> > +{
> > + return !preemptible();
> > +}
> > +
> > #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> >
> > #ifdef CONFIG_PROVE_RCU
> > diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
> > index 5ec3ea4028e2..7fbd21dbfcd0 100644
> > --- a/kernel/rcu/Kconfig.debug
> > +++ b/kernel/rcu/Kconfig.debug
> > @@ -8,6 +8,17 @@ menu "RCU Debugging"
> > config PROVE_RCU
> > def_bool PROVE_LOCKING
> >
> > +config PROVE_RCU_LIST
> > + bool "RCU list lockdep debugging"
> > + depends on PROVE_RCU
>
> This must also depend on RCU_EXPERT.
Sure.
> > + default n
> > + help
> > + Enable RCU lockdep checking for list usages. By default it is
> > + turned off since there are several list RCU users that still
> > + need to be converted to pass a lockdep expression. To prevent
> > + false-positive splats, we keep it default disabled but once all
> > + users are converted, we can remove this config option.
> > +
> > config TORTURE_TEST
> > tristate
> > default n
> > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > index 9dd5aeef6e70..b7a4e3b5fa98 100644
> > --- a/kernel/rcu/update.c
> > +++ b/kernel/rcu/update.c
> > @@ -91,14 +91,18 @@ module_param(rcu_normal_after_boot, int, 0);
> > * Similarly, we avoid claiming an SRCU read lock held if the current
> > * CPU is offline.
> > */
> > +#define rcu_read_lock_held_common() \
> > + if (!debug_lockdep_rcu_enabled()) \
> > + return 1; \
> > + if (!rcu_is_watching()) \
> > + return 0; \
> > + if (!rcu_lockdep_current_cpu_online()) \
> > + return 0;
>
> Nice abstraction of common code!
Thanks!
^ permalink raw reply
* Re: [PATCH 0/9] Harden list_for_each_entry_rcu() and family
From: Paul E. McKenney @ 2019-07-16 18:46 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190715143705.117908-1-joel@joelfernandes.org>
On Mon, Jul 15, 2019 at 10:36:56AM -0400, Joel Fernandes (Google) wrote:
> Hi,
> This series aims to provide lockdep checking to RCU list macros for additional
> kernel hardening.
>
> RCU has a number of primitives for "consumption" of an RCU protected pointer.
> Most of the time, these consumers make sure that such accesses are under a RCU
> reader-section (such as rcu_dereference{,sched,bh} or under a lock, such as
> with rcu_dereference_protected()).
>
> However, there are other ways to consume RCU pointers, such as by
> list_for_each_entry_rcu or hlist_for_each_enry_rcu. Unlike the rcu_dereference
> family, these consumers do no lockdep checking at all. And with the growing
> number of RCU list uses (1000+), it is possible for bugs to creep in and go
> unnoticed which lockdep checks can catch.
>
> Since RCU consolidation efforts last year, the different traditional RCU
> flavors (preempt, bh, sched) are all consolidated. In other words, any of these
> flavors can cause a reader section to occur and all of them must cease before
> the reader section is considered to be unlocked. Thanks to this, we can
> generically check if we are in an RCU reader. This is what patch 1 does. Note
> that the list_for_each_entry_rcu and family are different from the
> rcu_dereference family in that, there is no _bh or _sched version of this
> macro. They are used under many different RCU reader flavors, and also SRCU.
> Patch 1 adds a new internal function rcu_read_lock_any_held() which checks
> if any reader section is active at all, when these macros are called. If no
> reader section exists, then the optional fourth argument to
> list_for_each_entry_rcu() can be a lockdep expression which is evaluated
> (similar to how rcu_dereference_check() works). If no lockdep expression is
> passed, and we are not in a reader, then a splat occurs. Just take off the
> lockdep expression after applying the patches, by using the following diff and
> see what happens:
>
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -55,7 +55,7 @@ static void list_add_sorted(struct pci_mmcfg_region *new)
> struct pci_mmcfg_region *cfg;
>
> /* keep list sorted by segment and starting bus number */
> - list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held()) {
> + list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
>
>
> The optional argument trick to list_for_each_entry_rcu() can also be used in
> the future to possibly remove rcu_dereference_{,bh,sched}_protected() API and
> we can pass an optional lockdep expression to rcu_dereference() itself. Thus
> eliminating 3 more RCU APIs.
>
> Note that some list macro wrappers already do their own lockdep checking in the
> caller side. These can be eliminated in favor of the built-in lockdep checking
> in the list macro that this series adds. For example, workqueue code has a
> assert_rcu_or_wq_mutex() function which is called in for_each_wq(). This
> series replaces that in favor of the built-in check.
>
> Also in the future, we can extend these checks to list_entry_rcu() and other
> list macros as well, if needed.
>
> Please note that I have kept this option default-disabled under a new config:
> CONFIG_PROVE_RCU_LIST. This is so that until all users are converted to pass
> the optional argument, we should keep the check disabled. There are about a
> 1000 or so users and it is not possible to pass in the optional lockdep
> expression in a single series since it is done on a case-by-case basis. I did
> convert a few users in this series itself.
I do like the optional argument as opposed to the traditional practice
of expanding the RCU API! Good stuff!!!
Please resend incorporating the acks and the changes from feedback.
I will hold off on any patches not yet having their maintainer's ack,
but it is OK to include them in v4. (I will just avoid applying them.)
The documentation patch needs a bit of wordsmithing, but I can do that.
Feel free to take another pass on it if you wish, though.
Thanx, Paul
> v2->v3: Simplified rcu-sync logic after rebase (Paul)
> Added check for bh_map (Paul)
> Refactored out more of the common code (Joel)
> Added Oleg ack to rcu-sync patch.
>
> v1->v2: Have assert_rcu_or_wq_mutex deleted (Daniel Jordan)
> Simplify rcu_read_lock_any_held() (Peter Zijlstra)
> Simplified rcu-sync logic (Oleg Nesterov)
> Updated documentation and rculist comments.
> Added GregKH ack.
>
> RFC->v1:
> Simplify list checking macro (Rasmus Villemoes)
>
> Joel Fernandes (Google) (9):
> rcu/update: Remove useless check for debug_locks (v1)
> rcu: Add support for consolidated-RCU reader checking (v3)
> rcu/sync: Remove custom check for reader-section (v2)
> ipv4: add lockdep condition to fix for_each_entry (v1)
> driver/core: Convert to use built-in RCU list checking (v1)
> workqueue: Convert for_each_wq to use built-in list check (v2)
> x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1)
> acpi: Use built-in RCU list checking for acpi_ioremaps list (v1)
> doc: Update documentation about list_for_each_entry_rcu (v1)
>
> Documentation/RCU/lockdep.txt | 15 ++++++++---
> Documentation/RCU/whatisRCU.txt | 9 ++++++-
> arch/x86/pci/mmconfig-shared.c | 5 ++--
> drivers/acpi/osl.c | 6 +++--
> drivers/base/base.h | 1 +
> drivers/base/core.c | 10 +++++++
> drivers/base/power/runtime.c | 15 +++++++----
> include/linux/rcu_sync.h | 4 +--
> include/linux/rculist.h | 28 +++++++++++++++----
> include/linux/rcupdate.h | 7 +++++
> kernel/rcu/Kconfig.debug | 11 ++++++++
> kernel/rcu/update.c | 48 ++++++++++++++++++---------------
> kernel/workqueue.c | 10 ++-----
> net/ipv4/fib_frontend.c | 3 ++-
> 14 files changed, 119 insertions(+), 53 deletions(-)
>
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH v2 2/9] rcu: Add support for consolidated-RCU reader checking
From: Paul E. McKenney @ 2019-07-16 18:50 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716183517.GA129705@google.com>
On Tue, Jul 16, 2019 at 02:35:17PM -0400, Joel Fernandes wrote:
> On Tue, Jul 16, 2019 at 11:22:37AM -0700, Paul E. McKenney wrote:
> > On Fri, Jul 12, 2019 at 01:00:17PM -0400, Joel Fernandes (Google) wrote:
> > > This patch adds support for checking RCU reader sections in list
> > > traversal macros. Optionally, if the list macro is called under SRCU or
> > > other lock/mutex protection, then appropriate lockdep expressions can be
> > > passed to make the checks pass.
> > >
> > > Existing list_for_each_entry_rcu() invocations don't need to pass the
> > > optional fourth argument (cond) unless they are under some non-RCU
> > > protection and needs to make lockdep check pass.
> > >
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >
> > If you fold in the checks for extra parameters, I will take this
> > one and also 1/9.
>
> I folded the checks in and also threw in the rcu-sync with Oleg's ack:
>
> Could you pull into /dev branch?
>
> git pull https://github.com/joelagnel/linux-kernel.git list-first-three
> (Based on your dev branch)
Given that I am going to have to rebase these a few times, please
email a v4.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Brendan Higgins @ 2019-07-16 18: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: <20190716175021.9CA412173C@mail.kernel.org>
On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-07-16 01:37:34)
> > On Tue, Jul 16, 2019 at 12:57 AM Brendan Higgins
> > <brendanhiggins@google.com> wrote:
> > >
> > > A `struct kunit_stream` is usually associated with a message that is
> > > being built up over time like maybe an expectation; it is meant to
> > > capture the idea that we might want to send some information out to
> > > the user pertaining to some thing 'X', but we aren't sure that we
> > > actually want to send it until 'X' is complete, but do to the nature
> > > of 'X' it is easier to start constructing the message before 'X' is
> > > complete.
> > >
> > > Consider a complicated expectation, there might be multiple conditions
> > > that satisfy it and multiple conditions which could make it fail. As
> > > we start exploring the input to the expectation we gain information
> > > that we might want to share back with the user if the expectation were
> > > to fail and we might get that information before we are actually sure
> > > that the expectation does indeed fail.
> > >
> > > When we first step into the expectation we immediately know the
> > > function name, file name, and line number where we are called and
> > > would want to put that information into any message we would send to
> > > the user about this expectation. Next, we might want to check a
> > > property of the input, it may or may not be enough information on its
> > > own for the expectation to fail, but we want to share the result of
> > > the property check with the user regardless, BUT only if the
> > > expectation as a whole fails.
> > >
> > > Hence, we can have multiple `struct kunit_stream`s associated with a
> > > `struct kunit` active at any given time.
>
> I'm coming back to this now after reading the rest of the patches that
> deal with assertions and expectations. It looks like the string stream
> is there to hold a few different pieces of information:
>
> - Line Number
> - File Name
> - Function Name
>
> The above items could be stored in a structure on the stack that then
> gets printed and formatted when the expectation or assertion fails. That
> would make the whole string stream structure and code unnecessary.
Most of the expectations and assertions in this patchset are fairly
simple, and what you are describing would probably work. However, I
have some expectations I plan on adding in later patchsets that make
much more complicated checks.
> The only hypothetical case where this can't be done is a complicated
> assertion or expectation that does more than one check and can't be
> written as a function that dumps out what went wrong. Is this a real
> problem? Maybe such an assertion should just open code that logic so we
> don't have to build up a string for all the other simple cases.
I have some expectations in follow up patchsets for which I created a
set of composable matchers for matching structures and function calls
that by their nature cannot be written as a single function. The
matcher thing is a bit speculative, I know, but for any kind of
function call matching, you need to store a record of functions you
are expecting to have called and then each one needs to have a set of
expectations defined by the user; I don't think there is a way to do
that that doesn't involve having multiple separate functions each
having some information useful to constructing the message.
I know the code in question isn't in this patchset; the function
matching code was in one of the earlier versions of the RFC, but I
dropped it to make this patchset smaller and more manageable. So I get
it if you would like me to drop it and add it back in when I try to
get the function and structure matching stuff in, but I would really
prefer to keep it as is if you don't care too much.
> It seems far simpler to get rid of the string stream API and just have a
> struct for this.
>
> struct kunit_fail_msg {
> const char *line;
> const char *file;
> const char *func;
> const char *msg;
> };
>
> Then you can have the assertion macros create this on the stack (with
> another macro?).
>
> #define DEFINE_KUNIT_FAIL_MSG(name, _msg) \
> struct kunit_fail_msg name = { \
> .line = __LINE__, \
> .file = __FILE__, \
> .func = __func__, \
> .msg = _msg, \
> }
>
> Note: I don't know if the __LINE__ above will use the macro location, so
> this probably needs another wrapper to put the right line number there.
No, that should work. It picks up where the macro ends up being
finally evaluated.
> I don't want to derail this whole series on this topic, but it seems
> like a bunch of code is there to construct this same set of information
> over and over again into a buffer a little bit at a time and then throw
> it away when nothing fails just because we may want to support the case
> where we have some unstructured data to inform the user about.
Yeah, that's fair. I think there are a number of improvements to be
made with how the expectations are defined other than that, but I was
hoping I could do that after this patchset is merged. I just figured
with the kinds of things I would like to do, it would lead to a whole
new round of discussion.
In either case, I think I would still like to use the `struct
kunit_stream` as part of the interface to share the failure message
with the test case runner code in test.c, at least eventually, so that
I only have to have one way to receive data from expectations, but I
think I can do that and still do what you suggest by just constructing
the kunit_stream at the end of expectations where it is feasible.
All in all I agree with what you are saying, but I would rather do it
as a follow up possibly once we have some more code on the table. I
could just see this opening up a whole new can of worms where we
debate about exactly how expectations and assertions work for another
several months, only to rip it all out shortly there after. I know
that's how these things go, but that's my preference.
I can do what you suggest if you feel strongly about it, but I would
prefer to hold off until later. It's your call.
> Why not build in the structured part into the framework (i.e. the struct
> above) so that it's always there and then add the string building part
> later when we have to?
See above comments.
^ permalink raw reply
* Re: [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
From: Paul E. McKenney @ 2019-07-16 18:53 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716184649.GA130463@google.com>
On Tue, Jul 16, 2019 at 02:46:49PM -0400, Joel Fernandes wrote:
> On Tue, Jul 16, 2019 at 11:38:33AM -0700, Paul E. McKenney wrote:
> > On Mon, Jul 15, 2019 at 10:36:58AM -0400, Joel Fernandes (Google) wrote:
> > > This patch adds support for checking RCU reader sections in list
> > > traversal macros. Optionally, if the list macro is called under SRCU or
> > > other lock/mutex protection, then appropriate lockdep expressions can be
> > > passed to make the checks pass.
> > >
> > > Existing list_for_each_entry_rcu() invocations don't need to pass the
> > > optional fourth argument (cond) unless they are under some non-RCU
> > > protection and needs to make lockdep check pass.
> > >
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >
> > Now that I am on the correct version, again please fold in the checks
> > for the extra argument. The ability to have an optional argument looks
> > quite helpful, especially when compared to growing the RCU API!
>
> I did fold this and replied with a pull request URL based on /dev branch. But
> we can hold off on the pull requests until we decide on the below comments:
>
> > A few more things below.
> > > ---
> > > include/linux/rculist.h | 28 ++++++++++++++++++++-----
> > > include/linux/rcupdate.h | 7 +++++++
> > > kernel/rcu/Kconfig.debug | 11 ++++++++++
> > > kernel/rcu/update.c | 44 ++++++++++++++++++++++++----------------
> > > 4 files changed, 67 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > > index e91ec9ddcd30..1048160625bb 100644
> > > --- a/include/linux/rculist.h
> > > +++ b/include/linux/rculist.h
> > > @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> > > */
> > > #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next)))
> > >
> > > +/*
> > > + * Check during list traversal that we are within an RCU reader
> > > + */
> > > +
> > > +#ifdef CONFIG_PROVE_RCU_LIST
> >
> > This new Kconfig option is OK temporarily, but unless there is reason to
> > fear malfunction that a few weeks of rcutorture, 0day, and -next won't
> > find, it would be better to just use CONFIG_PROVE_RCU. The overall goal
> > is to reduce the number of RCU knobs rather than grow them, must though
> > history might lead one to believe otherwise. :-/
>
> If you want, we can try to drop this option and just use PROVE_RCU however I
> must say there may be several warnings that need to be fixed in a short
> period of time (even a few weeks may be too short) considering the 1000+
> uses of RCU lists.
Do many people other than me build with CONFIG_PROVE_RCU? If so, then
that would be a good reason for a temporary CONFIG_PROVE_RCU_LIST,
as in going away in a release or two once the warnings get fixed.
> But I don't mind dropping it and it may just accelerate the fixing up of all
> callers.
I will let you decide based on the above question. But if you have
CONFIG_PROVE_RCU_LIST, as noted below, it needs to depend on RCU_EXPERT.
Thanx, Paul
> > > +#define __list_check_rcu(dummy, cond, ...) \
> > > + ({ \
> > > + RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \
> > > + "RCU-list traversed in non-reader section!"); \
> > > + })
> > > +#else
> > > +#define __list_check_rcu(dummy, cond, ...) ({})
> > > +#endif
> > > +
> > > /*
> > > * Insert a new entry between two known consecutive entries.
> > > *
> > > @@ -343,14 +357,16 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> > > * @pos: the type * to use as a loop cursor.
> > > * @head: the head for your list.
> > > * @member: the name of the list_head within the struct.
> > > + * @cond: optional lockdep expression if called from non-RCU protection.
> > > *
> > > * This list-traversal primitive may safely run concurrently with
> > > * the _rcu list-mutation primitives such as list_add_rcu()
> > > * as long as the traversal is guarded by rcu_read_lock().
> > > */
> > > -#define list_for_each_entry_rcu(pos, head, member) \
> > > - for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> > > - &pos->member != (head); \
> > > +#define list_for_each_entry_rcu(pos, head, member, cond...) \
> > > + for (__list_check_rcu(dummy, ## cond, 0), \
> > > + pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> > > + &pos->member != (head); \
> > > pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> > >
> > > /**
> > > @@ -616,13 +632,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> > > * @pos: the type * to use as a loop cursor.
> > > * @head: the head for your list.
> > > * @member: the name of the hlist_node within the struct.
> > > + * @cond: optional lockdep expression if called from non-RCU protection.
> > > *
> > > * This list-traversal primitive may safely run concurrently with
> > > * the _rcu list-mutation primitives such as hlist_add_head_rcu()
> > > * as long as the traversal is guarded by rcu_read_lock().
> > > */
> > > -#define hlist_for_each_entry_rcu(pos, head, member) \
> > > - for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > > +#define hlist_for_each_entry_rcu(pos, head, member, cond...) \
> > > + for (__list_check_rcu(dummy, ## cond, 0), \
> > > + pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > > typeof(*(pos)), member); \
> > > pos; \
> > > pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
> > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > index 8f7167478c1d..f3c29efdf19a 100644
> > > --- a/include/linux/rcupdate.h
> > > +++ b/include/linux/rcupdate.h
> > > @@ -221,6 +221,7 @@ int debug_lockdep_rcu_enabled(void);
> > > int rcu_read_lock_held(void);
> > > int rcu_read_lock_bh_held(void);
> > > int rcu_read_lock_sched_held(void);
> > > +int rcu_read_lock_any_held(void);
> > >
> > > #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> > >
> > > @@ -241,6 +242,12 @@ static inline int rcu_read_lock_sched_held(void)
> > > {
> > > return !preemptible();
> > > }
> > > +
> > > +static inline int rcu_read_lock_any_held(void)
> > > +{
> > > + return !preemptible();
> > > +}
> > > +
> > > #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> > >
> > > #ifdef CONFIG_PROVE_RCU
> > > diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
> > > index 5ec3ea4028e2..7fbd21dbfcd0 100644
> > > --- a/kernel/rcu/Kconfig.debug
> > > +++ b/kernel/rcu/Kconfig.debug
> > > @@ -8,6 +8,17 @@ menu "RCU Debugging"
> > > config PROVE_RCU
> > > def_bool PROVE_LOCKING
> > >
> > > +config PROVE_RCU_LIST
> > > + bool "RCU list lockdep debugging"
> > > + depends on PROVE_RCU
> >
> > This must also depend on RCU_EXPERT.
>
> Sure.
>
> > > + default n
> > > + help
> > > + Enable RCU lockdep checking for list usages. By default it is
> > > + turned off since there are several list RCU users that still
> > > + need to be converted to pass a lockdep expression. To prevent
> > > + false-positive splats, we keep it default disabled but once all
> > > + users are converted, we can remove this config option.
> > > +
> > > config TORTURE_TEST
> > > tristate
> > > default n
> > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > > index 9dd5aeef6e70..b7a4e3b5fa98 100644
> > > --- a/kernel/rcu/update.c
> > > +++ b/kernel/rcu/update.c
> > > @@ -91,14 +91,18 @@ module_param(rcu_normal_after_boot, int, 0);
> > > * Similarly, we avoid claiming an SRCU read lock held if the current
> > > * CPU is offline.
> > > */
> > > +#define rcu_read_lock_held_common() \
> > > + if (!debug_lockdep_rcu_enabled()) \
> > > + return 1; \
> > > + if (!rcu_is_watching()) \
> > > + return 0; \
> > > + if (!rcu_lockdep_current_cpu_online()) \
> > > + return 0;
> >
> > Nice abstraction of common code!
>
> Thanks!
>
^ permalink raw reply
* Re: [PATCH v9 03/18] kunit: test: add string_stream a std::stream like string builder
From: Brendan Higgins @ 2019-07-16 18:55 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: <20190716153400.5CB182054F@mail.kernel.org>
On Tue, Jul 16, 2019 at 8:34 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-07-15 15:43:20)
> > On Mon, Jul 15, 2019 at 3:11 PM Brendan Higgins
> > <brendanhiggins@google.com> wrote:
> > >
> > > On Mon, Jul 15, 2019 at 3:04 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > >
> > > > Quoting Brendan Higgins (2019-07-15 14:11:50)
> > > > > On Mon, Jul 15, 2019 at 1:43 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > > > >
> > > > > > I also wonder if it would be better to just have a big slop buffer of a
> > > > > > 4K page or something so that we almost never have to allocate anything
> > > > > > with a string_stream and we can just rely on a reader consuming data
> > > > > > while writers are writing. That might work out better, but I don't quite
> > > > > > understand the use case for the string stream.
> > > > >
> > > > > That makes sense, but might that also waste memory since we will
> > > > > almost never need that much memory?
> > > >
> > > > Why do we care? These are unit tests.
> > >
> > > Agreed.
> > >
> > > > Having allocations in here makes
> > > > things more complicated, whereas it would be simpler to have a pointer
> > > > and a spinlock operating on a chunk of memory that gets flushed out
> > > > periodically.
> > >
> > > I am not so sure. I have to have the logic to allocate memory in some
> > > case no matter what (what if I need more memory that my preallocated
> > > chuck?). I think it is simpler to always request an allocation than to
> > > only sometimes request an allocation.
> >
> > Another even simpler alternative might be to just allocate memory
> > using kunit_kmalloc as we need it and just let the kunit_resource code
> > handle cleaning it all up when the test case finishes.
>
> Sure, sounds like a nice way to avoid duplicating similar logic to
> maintain a list of things to free later.
I think I will go that route for now.
> >
> > What do you think?
>
> If you go the allocation route then you'll need to have the flags to
> know what context you're in to allocate appropriately. Does that mean
> all the string operations will now take GFP flags?
We could set the GFP flags in the constructor, store them in a field,
and then just reuse them.
Thanks!
^ permalink raw reply
* Re: [RFC v1 0/4] arm64: MMU enabled kexec kernel relocation
From: Bhupesh Sharma @ 2019-07-16 19:14 UTC (permalink / raw)
To: Pavel Tatashin
Cc: James Morris, sashal, Eric Biederman, kexec mailing list,
Linux Kernel Mailing List, Jonathan Corbet, Catalin Marinas, will,
Linux Doc Mailing List, linux-arm-kernel
In-Reply-To: <20190716165641.6990-1-pasha.tatashin@soleen.com>
Hi Pavel,
On Tue, Jul 16, 2019 at 10:26 PM Pavel Tatashin
<pasha.tatashin@soleen.com> wrote:
>
> Added identity mapped page table, and keep MMU enabled while
> kernel is being relocated from sparse pages to the final
> destination during kexec.
>
> More description about the problem I am trying to solve here, can be
> found here:
> https://lore.kernel.org/lkml/20190709182014.16052-1-pasha.tatashin@soleen.com/
>
> This patch series works in terms, that I can kexec-reboot both in QEMU
> and on a physical machine. However, I do not see performance improvement
> during relocation. The performance is just as slow as before with disabled
> caches.
Thanks for the patchset, but if the changes still don't positively
impact the kexec-reboot timings, I am not sure we if gain by adding
these to the kernel.
Like I mentioned in the previous threads, we have been carrying some
relevant fixes for the same in Linux distros. I have been trying to
find time to fix them and send them upstream, but I am caught up with
some nasty kexec_file_load() issues on arm64 currently.
So, I will find some time to work on them (may be next week) and will
Cc you when I post them out after some checks on real physical
hardware.
Thanks,
Bhupesh
> Am I missing something? Perhaps, there is some flag that I should also
> enable in page table? Please provide me with any suggestions.
>
> Pavel Tatashin (4):
> arm64, mm: identity mapped page table
> arm64, kexec: interface preparation for mmu enabled kexec
> arm64, kexec: add kexec's own identity page table
> arm64: Keep MMU on while kernel is being relocated
>
> arch/arm64/include/asm/ident_map.h | 26 ++++++
> arch/arm64/include/asm/kexec.h | 5 +-
> arch/arm64/kernel/cpu-reset.S | 8 --
> arch/arm64/kernel/cpu-reset.h | 7 +-
> arch/arm64/kernel/machine_kexec.c | 128 +++++++++++++++++++++-------
> arch/arm64/kernel/relocate_kernel.S | 36 +++++---
> arch/arm64/mm/Makefile | 1 +
> arch/arm64/mm/ident_map.c | 99 +++++++++++++++++++++
> 8 files changed, 255 insertions(+), 55 deletions(-)
> create mode 100644 arch/arm64/include/asm/ident_map.h
> create mode 100644 arch/arm64/mm/ident_map.c
>
> --
> 2.22.0
>
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply
* Re: [RFC v1 0/4] arm64: MMU enabled kexec kernel relocation
From: Pavel Tatashin @ 2019-07-16 19:26 UTC (permalink / raw)
To: Bhupesh Sharma
Cc: James Morris, Sasha Levin, Eric Biederman, kexec mailing list,
Linux Kernel Mailing List, Jonathan Corbet, Catalin Marinas, will,
Linux Doc Mailing List, linux-arm-kernel
In-Reply-To: <CACi5LpOO+sF3o+5u4jHXzba+Ki8fZ5auekKLayxSwNOL6Lp=-w@mail.gmail.com>
On Tue, Jul 16, 2019 at 3:14 PM Bhupesh Sharma <bhsharma@redhat.com> wrote:
>
> Hi Pavel,
>
> On Tue, Jul 16, 2019 at 10:26 PM Pavel Tatashin
> <pasha.tatashin@soleen.com> wrote:
> >
> > Added identity mapped page table, and keep MMU enabled while
> > kernel is being relocated from sparse pages to the final
> > destination during kexec.
> >
> > More description about the problem I am trying to solve here, can be
> > found here:
> > https://lore.kernel.org/lkml/20190709182014.16052-1-pasha.tatashin@soleen.com/
> >
> > This patch series works in terms, that I can kexec-reboot both in QEMU
> > and on a physical machine. However, I do not see performance improvement
> > during relocation. The performance is just as slow as before with disabled
> > caches.
>
> Thanks for the patchset, but if the changes still don't positively
> impact the kexec-reboot timings, I am not sure we if gain by adding
> these to the kernel.
Hi Bhupesh,
I am not asking to add these to the kernel (hence RFC), I am looking
for help to figure out why the relocation is still slow, once that is
understood I will submit the patches for integration. My previous
patch series fixed the relocation problem by pre-reserving space, but
because the culprit of the problem was narrowed down to disabled
caches it was decided that a better fix would be to do relocation with
MMU still enabled, this is why I created this new series.
>
> Like I mentioned in the previous threads, we have been carrying some
> relevant fixes for the same in Linux distros. I have been trying to
> find time to fix them and send them upstream, but I am caught up with
> some nasty kexec_file_load() issues on arm64 currently.
As I understood, the fixes were for slow purgatory checksum checking,
and not for relocation of there kernel. Are you saying redhat is
carrying some patches that address slow relocation problem as well?
Thank you,
Pasha
>
> So, I will find some time to work on them (may be next week) and will
> Cc you when I post them out after some checks on real physical
> hardware.
>
> Thanks,
> Bhupesh
>
> > Am I missing something? Perhaps, there is some flag that I should also
> > enable in page table? Please provide me with any suggestions.
> >
> > Pavel Tatashin (4):
> > arm64, mm: identity mapped page table
> > arm64, kexec: interface preparation for mmu enabled kexec
> > arm64, kexec: add kexec's own identity page table
> > arm64: Keep MMU on while kernel is being relocated
> >
> > arch/arm64/include/asm/ident_map.h | 26 ++++++
> > arch/arm64/include/asm/kexec.h | 5 +-
> > arch/arm64/kernel/cpu-reset.S | 8 --
> > arch/arm64/kernel/cpu-reset.h | 7 +-
> > arch/arm64/kernel/machine_kexec.c | 128 +++++++++++++++++++++-------
> > arch/arm64/kernel/relocate_kernel.S | 36 +++++---
> > arch/arm64/mm/Makefile | 1 +
> > arch/arm64/mm/ident_map.c | 99 +++++++++++++++++++++
> > 8 files changed, 255 insertions(+), 55 deletions(-)
> > create mode 100644 arch/arm64/include/asm/ident_map.h
> > create mode 100644 arch/arm64/mm/ident_map.c
> >
> > --
> > 2.22.0
> >
> >
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply
* Re: [GIT PULL for v5.3-rc1] docs: addition of a large set of files to the documentation body
From: pr-tracker-bot @ 2019-07-16 19:40 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linus Torvalds, Greg Kroah-Hartman, Jonathan Corbet,
Linux Documentation Mailing List, Linux Kernel Mailing List
In-Reply-To: <20190716080122.28a17014@coco.lan>
The pull request you sent on Tue, 16 Jul 2019 08:01:22 -0300:
> git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media.git tags/docs/v5.3-1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/c309b6f24222246c18a8b65d3950e6e755440865
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH v5 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: bsegall @ 2019-07-16 19:58 UTC (permalink / raw)
To: Dave Chiluk
Cc: Pqhil Auld, Peter Oskolkov, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, Brendan Gregg, Kyle Anderson, Gabriel Munos,
John Hammond, Cong Wang, Jonathan Corbet, linux-doc
In-Reply-To: <1561664970-1555-2-git-send-email-chiluk+linux@indeed.com>
Dave Chiluk <chiluk+linux@indeed.com> writes:
> It has been observed, that highly-threaded, non-cpu-bound applications
> running under cpu.cfs_quota_us constraints can hit a high percentage of
> periods throttled while simultaneously not consuming the allocated
> amount of quota. This use case is typical of user-interactive non-cpu
> bound applications, such as those running in kubernetes or mesos when
> run on multiple cpu cores.
>
> This has been root caused to threads being allocated per cpu bandwidth
> slices, and then not fully using that slice within the period. At which
> point the slice and quota expires. This expiration of unused slice
> results in applications not being able to utilize the quota for which
> they are allocated.
>
> The expiration of per-cpu slices was recently fixed by
> 'commit 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift
> condition")'. Prior to that it appears that this has been broken since
> at least 'commit 51f2176d74ac ("sched/fair: Fix unlocked reads of some
> cfs_b->quota/period")' which was introduced in v3.16-rc1 in 2014. That
> added the following conditional which resulted in slices never being
> expired.
>
> if (cfs_rq->runtime_expires != cfs_b->runtime_expires) {
> /* extend local deadline, drift is bounded above by 2 ticks */
> cfs_rq->runtime_expires += TICK_NSEC;
>
> Because this was broken for nearly 5 years, and has recently been fixed
> and is now being noticed by many users running kubernetes
> (https://github.com/kubernetes/kubernetes/issues/67577) it is my opinion
> that the mechanisms around expiring runtime should be removed
> altogether.
>
> This allows quota already allocated to per-cpu run-queues to live longer
> than the period boundary. This allows threads on runqueues that do not
> use much CPU to continue to use their remaining slice over a longer
> period of time than cpu.cfs_period_us. However, this helps prevents the
> above condition of hitting throttling while also not fully utilizing
> your cpu quota.
>
> This theoretically allows a machine to use slightly more than its
> allotted quota in some periods. This overflow would be bounded by the
> remaining quota left on each per-cpu runqueueu. This is typically no
> more than min_cfs_rq_runtime=1ms per cpu. For CPU bound tasks this will
> change nothing, as they should theoretically fully utilize all of their
> quota in each period. For user-interactive tasks as described above this
> provides a much better user/application experience as their cpu
> utilization will more closely match the amount they requested when they
> hit throttling. This means that cpu limits no longer strictly apply per
> period for non-cpu bound applications, but that they are still accurate
> over longer timeframes.
>
> This greatly improves performance of high-thread-count, non-cpu bound
> applications with low cfs_quota_us allocation on high-core-count
> machines. In the case of an artificial testcase (10ms/100ms of quota on
> 80 CPU machine), this commit resulted in almost 30x performance
> improvement, while still maintaining correct cpu quota restrictions.
> That testcase is available at https://github.com/indeedeng/fibtest.
>
> Fixes: 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift condition")
> Signed-off-by: Dave Chiluk <chiluk+linux@indeed.com>
> ---
> Documentation/scheduler/sched-bwc.txt | 73 ++++++++++++++++++++++++++++-------
> kernel/sched/fair.c | 71 +++-------------------------------
> kernel/sched/sched.h | 4 --
> 3 files changed, 65 insertions(+), 83 deletions(-)
>
> [...]
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index f35930f..a675c69 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4809,11 +4754,9 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> if (!runtime)
> return;
>
> - runtime = distribute_cfs_runtime(cfs_b, runtime, expires);
> + runtime = distribute_cfs_runtime(cfs_b, runtime);
>
> raw_spin_lock_irqsave(&cfs_b->lock, flags);
> - if (expires == cfs_b->runtime_expires)
> - lsub_positive(&cfs_b->runtime, runtime);
The lsub_positive is still needed, just get rid of the if.
Other than that,
Reviewed-by: Ben Segall <bsegall@google.com>
^ permalink raw reply
* Re: [PATCH 1/2] f2fs: include charset encoding information in the superblock
From: Jaegeuk Kim @ 2019-07-16 20:46 UTC (permalink / raw)
To: Daniel Rosenberg
Cc: Chao Yu, Jonathan Corbet, linux-f2fs-devel, linux-kernel,
linux-doc, linux-fsdevel, kernel-team
In-Reply-To: <20190711204556.120381-1-drosen@google.com>
Hi Daniel,
Could you please rebase you patch set?
e.g., f2fs_msg() was replaced with f2fs_err|info|...
On 07/11, Daniel Rosenberg wrote:
> Add charset encoding to f2fs to support casefolding. It is modeled after
> the same feature introduced in commit c83ad55eaa91 ("ext4: include charset
> encoding information in the superblock")
>
> Currently this is not compatible with encryption, similar to the current
> ext4 imlpementation. This will change in the future.
>
> >From the ext4 patch:
> """
> The s_encoding field stores a magic number indicating the encoding
> format and version used globally by file and directory names in the
> filesystem. The s_encoding_flags defines policies for using the charset
> encoding, like how to handle invalid sequences. The magic number is
> mapped to the exact charset table, but the mapping is specific to ext4.
> Since we don't have any commitment to support old encodings, the only
> encoding I am supporting right now is utf8-12.1.0.
>
> The current implementation prevents the user from enabling encoding and
> per-directory encryption on the same filesystem at the same time. The
> incompatibility between these features lies in how we do efficient
> directory searches when we cannot be sure the encryption of the user
> provided fname will match the actual hash stored in the disk without
> decrypting every directory entry, because of normalization cases. My
> quickest solution is to simply block the concurrent use of these
> features for now, and enable it later, once we have a better solution.
> """
>
> Signed-off-by: Daniel Rosenberg <drosen@google.com>
> ---
> fs/f2fs/f2fs.h | 6 +++
> fs/f2fs/super.c | 81 +++++++++++++++++++++++++++++++++++++++++
> include/linux/f2fs_fs.h | 9 ++++-
> 3 files changed, 95 insertions(+), 1 deletion(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 06b89a9862ab2..0e101f699eccd 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -150,6 +150,7 @@ struct f2fs_mount_info {
> #define F2FS_FEATURE_LOST_FOUND 0x0200
> #define F2FS_FEATURE_VERITY 0x0400 /* reserved */
> #define F2FS_FEATURE_SB_CHKSUM 0x0800
> +#define F2FS_FEATURE_CASEFOLD 0x1000
>
> #define __F2FS_HAS_FEATURE(raw_super, mask) \
> ((raw_super->feature & cpu_to_le32(mask)) != 0)
> @@ -1162,6 +1163,10 @@ struct f2fs_sb_info {
> int valid_super_block; /* valid super block no */
> unsigned long s_flag; /* flags for sbi */
> struct mutex writepages; /* mutex for writepages() */
> +#ifdef CONFIG_UNICODE
> + struct unicode_map *s_encoding;
> + __u16 s_encoding_flags;
> +#endif
>
> #ifdef CONFIG_BLK_DEV_ZONED
> unsigned int blocks_per_blkz; /* F2FS blocks per zone */
> @@ -3565,6 +3570,7 @@ F2FS_FEATURE_FUNCS(quota_ino, QUOTA_INO);
> F2FS_FEATURE_FUNCS(inode_crtime, INODE_CRTIME);
> F2FS_FEATURE_FUNCS(lost_found, LOST_FOUND);
> F2FS_FEATURE_FUNCS(sb_chksum, SB_CHKSUM);
> +F2FS_FEATURE_FUNCS(casefold, CASEFOLD);
>
> #ifdef CONFIG_BLK_DEV_ZONED
> static inline bool f2fs_blkz_is_seq(struct f2fs_sb_info *sbi, int devi,
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 6b959bbb336a3..a346f5a01370b 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -23,6 +23,7 @@
> #include <linux/f2fs_fs.h>
> #include <linux/sysfs.h>
> #include <linux/quota.h>
> +#include <linux/unicode.h>
>
> #include "f2fs.h"
> #include "node.h"
> @@ -211,6 +212,36 @@ void f2fs_msg(struct super_block *sb, const char *level, const char *fmt, ...)
> va_end(args);
> }
>
> +#ifdef CONFIG_UNICODE
> +static const struct f2fs_sb_encodings {
> + __u16 magic;
> + char *name;
> + char *version;
> +} f2fs_sb_encoding_map[] = {
> + {F2FS_ENC_UTF8_12_1, "utf8", "12.1.0"},
> +};
> +
> +static int f2fs_sb_read_encoding(const struct f2fs_super_block *sb,
> + const struct f2fs_sb_encodings **encoding,
> + __u16 *flags)
> +{
> + __u16 magic = le16_to_cpu(sb->s_encoding);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(f2fs_sb_encoding_map); i++)
> + if (magic == f2fs_sb_encoding_map[i].magic)
> + break;
> +
> + if (i >= ARRAY_SIZE(f2fs_sb_encoding_map))
> + return -EINVAL;
> +
> + *encoding = &f2fs_sb_encoding_map[i];
> + *flags = le16_to_cpu(sb->s_encoding_flags);
> +
> + return 0;
> +}
> +#endif
> +
> static inline void limit_reserve_root(struct f2fs_sb_info *sbi)
> {
> block_t limit = (sbi->user_block_count << 1) / 1000;
> @@ -812,6 +843,13 @@ static int parse_options(struct super_block *sb, char *options)
> return -EINVAL;
> }
> #endif
> +#ifndef CONFIG_UNICODE
> + if (f2fs_sb_has_casefold(sbi)) {
> + f2fs_msg(sb, KERN_ERR,
> + "Filesystem with casefold feature cannot be mounted without CONFIG_UNICODE");
> + return -EINVAL;
> + }
> +#endif
>
> if (F2FS_IO_SIZE_BITS(sbi) && !test_opt(sbi, LFS)) {
> f2fs_msg(sb, KERN_ERR,
> @@ -1110,6 +1148,9 @@ static void f2fs_put_super(struct super_block *sb)
> destroy_percpu_info(sbi);
> for (i = 0; i < NR_PAGE_TYPE; i++)
> kvfree(sbi->write_io[i]);
> +#ifdef CONFIG_UNICODE
> + utf8_unload(sbi->s_encoding);
> +#endif
> kvfree(sbi);
> }
>
> @@ -3157,6 +3198,42 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> sb->s_maxbytes = sbi->max_file_blocks <<
> le32_to_cpu(raw_super->log_blocksize);
> sb->s_max_links = F2FS_LINK_MAX;
> +#ifdef CONFIG_UNICODE
> + if (f2fs_sb_has_casefold(sbi) && !sbi->s_encoding) {
> + const struct f2fs_sb_encodings *encoding_info;
> + struct unicode_map *encoding;
> + __u16 encoding_flags;
> +
> + if (f2fs_sb_has_encrypt(sbi)) {
> + f2fs_msg(sb, KERN_ERR,
> + "Can't mount with encoding and encryption");
> + goto free_options;
> + }
> +
> + if (f2fs_sb_read_encoding(raw_super, &encoding_info,
> + &encoding_flags)) {
> + f2fs_msg(sb, KERN_ERR,
> + "Encoding requested by superblock is unknown");
> + goto free_options;
> + }
> +
> + encoding = utf8_load(encoding_info->version);
> + if (IS_ERR(encoding)) {
> + f2fs_msg(sb, KERN_ERR,
> + "can't mount with superblock charset: %s-%s "
> + "not supported by the kernel. flags: 0x%x.",
> + encoding_info->name, encoding_info->version,
> + encoding_flags);
> + goto free_options;
> + }
> + f2fs_msg(sb, KERN_INFO, "Using encoding defined by superblock: "
> + "%s-%s with flags 0x%hx", encoding_info->name,
> + encoding_info->version?:"\b", encoding_flags);
> +
> + sbi->s_encoding = encoding;
> + sbi->s_encoding_flags = encoding_flags;
> + }
> +#endif
>
> #ifdef CONFIG_QUOTA
> sb->dq_op = &f2fs_quota_operations;
> @@ -3511,6 +3588,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> free_bio_info:
> for (i = 0; i < NR_PAGE_TYPE; i++)
> kvfree(sbi->write_io[i]);
> +
> +#ifdef CONFIG_UNICODE
> + utf8_unload(sbi->s_encoding);
> +#endif
> free_options:
> #ifdef CONFIG_QUOTA
> for (i = 0; i < MAXQUOTAS; i++)
> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
> index 65559900d4d76..b7c9c7f721339 100644
> --- a/include/linux/f2fs_fs.h
> +++ b/include/linux/f2fs_fs.h
> @@ -36,6 +36,11 @@
>
> #define F2FS_MAX_QUOTAS 3
>
> +#define F2FS_ENC_UTF8_12_1 1
> +#define F2FS_ENC_STRICT_MODE_FL (1 << 0)
> +#define f2fs_has_strict_mode(sbi) \
> + (sbi->s_encoding_flags & F2FS_ENC_STRICT_MODE_FL)
> +
> #define F2FS_IO_SIZE(sbi) (1 << F2FS_OPTION(sbi).write_io_size_bits) /* Blocks */
> #define F2FS_IO_SIZE_KB(sbi) (1 << (F2FS_OPTION(sbi).write_io_size_bits + 2)) /* KB */
> #define F2FS_IO_SIZE_BYTES(sbi) (1 << (F2FS_OPTION(sbi).write_io_size_bits + 12)) /* B */
> @@ -109,7 +114,9 @@ struct f2fs_super_block {
> struct f2fs_device devs[MAX_DEVICES]; /* device list */
> __le32 qf_ino[F2FS_MAX_QUOTAS]; /* quota inode numbers */
> __u8 hot_ext_count; /* # of hot file extension */
> - __u8 reserved[310]; /* valid reserved region */
> + __le16 s_encoding; /* Filename charset encoding */
> + __le16 s_encoding_flags; /* Filename charset encoding flags */
> + __u8 reserved[306]; /* valid reserved region */
> __le32 crc; /* checksum of superblock */
> } __packed;
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: [PATCH 4/9] ipv4: add lockdep condition to fix for_each_entry (v1)
From: David Miller @ 2019-07-16 21:12 UTC (permalink / raw)
To: paulmck
Cc: joel, linux-kernel, kuznet, bhelgaas, bp, c0d1n61at3, edumazet,
gregkh, yoshfuji, hpa, mingo, corbet, josh, keescook,
kernel-hardening, kernel-team, jiangshanlai, lenb, linux-acpi,
linux-doc, linux-pci, linux-pm, mathieu.desnoyers, neilb, netdev,
oleg, pavel, peterz, rjw, rasmus.villemoes, rcu, rostedt, tj,
tglx, will, x86
In-Reply-To: <20190716183955.GF14271@linux.ibm.com>
From: "Paul E. McKenney" <paulmck@linux.ibm.com>
Date: Tue, 16 Jul 2019 11:39:55 -0700
> On Mon, Jul 15, 2019 at 10:37:00AM -0400, Joel Fernandes (Google) wrote:
>> Using the previous support added, use it for adding lockdep conditions
>> to list usage here.
>>
>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> We need an ack or better from the subsystem maintainer for this one.
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: [PATCH v2 08/11] kbuild: create *.mod with full directory path and remove MODVERDIR
From: Joe Lawrence @ 2019-07-16 21:40 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, Sam Ravnborg, Nicolas Pitre, linux-doc,
Jonathan Corbet, linux-kernel, Michal Marek, Joe Lawrence
In-Reply-To: <20190711054434.1177-9-yamada.masahiro@socionext.com>
On Thu, Jul 11, 2019 at 02:44:31PM +0900, Masahiro Yamada wrote:
> While descending directories, Kbuild produces objects for modules,
> but do not link final *.ko files; it is done in the modpost.
>
> To keep track of modules, Kbuild creates a *.mod file in $(MODVERDIR)
> for every module it is building. Some post-processing steps read the
> necessary information from *.mod files. This avoids descending into
> directories again. This mechanism was introduced in 2003 or so.
>
> Later, commit 551559e13af1 ("kbuild: implement modules.order") added
> modules.order. So, we can simply read it out to know all the modules
> with directory paths. This is easier than parsing the first line of
> *.mod files.
>
> $(MODVERDIR) has a flat directory structure, that is, *.mod files
> are named only with base names. This is based on the assumption that
> the module name is unique across the tree. This assumption is really
> fragile.
>
> Stephen Rothwell reported a race condition caused by a module name
> conflict:
>
> https://lkml.org/lkml/2019/5/13/991
>
> In parallel building, two different threads could write to the same
> $(MODVERDIR)/*.mod simultaneously.
>
> Non-unique module names are the source of all kind of troubles, hence
> commit 3a48a91901c5 ("kbuild: check uniqueness of module names")
> introduced a new checker script.
>
> However, it is still fragile in the build system point of view because
> this race happens before scripts/modules-check.sh is invoked. If it
> happens again, the modpost will emit unclear error messages.
>
> To fix this issue completely, create *.mod in the same directory as
> *.ko so that two threads never attempt to write to the same file.
> $(MODVERDIR) is no longer needed.
>
> Since modules with directory paths are listed in modules.order, Kbuild
> is still able to find *.mod files without additional descending.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Acked-by: Nicolas Pitre <nico@fluxnic.net>
> ---
>
> Changes in v2:
> - Remove -r of xargs, which is a GNU extension
> - Add '--' for extra safety
>
> .gitignore | 1 +
> Documentation/dontdiff | 1 +
> Makefile | 20 +++-----------------
> scripts/Makefile.build | 8 ++------
> scripts/Makefile.modpost | 4 ++--
> scripts/adjust_autoksyms.sh | 11 ++++-------
> scripts/mod/sumversion.c | 16 +++-------------
> scripts/package/mkspec | 2 +-
> 8 files changed, 17 insertions(+), 46 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index 7587ef56b92d..8f5422cba6e2 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -30,6 +30,7 @@
> *.lz4
> *.lzma
> *.lzo
> +*.mod
> *.mod.c
> *.o
> *.o.*
> diff --git a/Documentation/dontdiff b/Documentation/dontdiff
> index 5eba889ea84d..9f4392876099 100644
> --- a/Documentation/dontdiff
> +++ b/Documentation/dontdiff
> @@ -30,6 +30,7 @@
> *.lzo
> *.mo
> *.moc
> +*.mod
> *.mod.c
> *.o
> *.o.*
> diff --git a/Makefile b/Makefile
> index b5e21d676ee2..4243b6daffcf 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -488,11 +488,6 @@ export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE
> export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL
> export KBUILD_ARFLAGS
>
> -# When compiling out-of-tree modules, put MODVERDIR in the module
> -# tree rather than in the kernel tree. The kernel tree might
> -# even be read-only.
> -export MODVERDIR := $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/).tmp_versions
> -
> # Files to ignore in find ... statements
>
> export RCS_FIND_IGNORE := \( -name SCCS -o -name BitKeeper -o -name .svn -o \
> @@ -1033,7 +1028,7 @@ vmlinux-deps := $(KBUILD_LDS) $(KBUILD_VMLINUX_OBJS) $(KBUILD_VMLINUX_LIBS)
>
> # Recurse until adjust_autoksyms.sh is satisfied
> PHONY += autoksyms_recursive
> -autoksyms_recursive: $(vmlinux-deps)
> +autoksyms_recursive: $(vmlinux-deps) modules.order
> ifdef CONFIG_TRIM_UNUSED_KSYMS
> $(Q)$(CONFIG_SHELL) $(srctree)/scripts/adjust_autoksyms.sh \
> "$(MAKE) -f $(srctree)/Makefile vmlinux"
> @@ -1117,7 +1112,6 @@ endif
>
> prepare1: prepare3 outputmakefile asm-generic $(version_h) $(autoksyms_h) \
> include/generated/utsrelease.h
> - $(cmd_crmodverdir)
>
> archprepare: archheaders archscripts prepare1 scripts
>
> @@ -1375,7 +1369,7 @@ endif # CONFIG_MODULES
> # make distclean Remove editor backup files, patch leftover files and the like
>
> # Directories & files removed with 'make clean'
> -CLEAN_DIRS += $(MODVERDIR) include/ksym
> +CLEAN_DIRS += include/ksym
> CLEAN_FILES += modules.builtin.modinfo
>
> # Directories & files removed with 'make mrproper'
> @@ -1636,7 +1630,6 @@ PHONY += $(clean-dirs) clean
> $(clean-dirs):
> $(Q)$(MAKE) $(clean)=$(patsubst _clean_%,%,$@)
>
> -clean: rm-dirs := $(MODVERDIR)
> clean: rm-files := $(KBUILD_EXTMOD)/Module.symvers
>
> PHONY += help
> @@ -1650,8 +1643,6 @@ help:
> @echo ''
>
> PHONY += prepare
> -prepare:
> - $(cmd_crmodverdir)
> endif # KBUILD_EXTMOD
>
> clean: $(clean-dirs)
> @@ -1662,7 +1653,7 @@ clean: $(clean-dirs)
> -o -name '*.ko.*' \
> -o -name '*.dtb' -o -name '*.dtb.S' -o -name '*.dt.yaml' \
> -o -name '*.dwo' -o -name '*.lst' \
> - -o -name '*.su' \
> + -o -name '*.su' -o -name '*.mod' \
> -o -name '.*.d' -o -name '.*.tmp' -o -name '*.mod.c' \
> -o -name '*.lex.c' -o -name '*.tab.[ch]' \
> -o -name '*.asn1.[ch]' \
> @@ -1791,11 +1782,6 @@ quiet_cmd_depmod = DEPMOD $(KERNELRELEASE)
> cmd_depmod = $(CONFIG_SHELL) $(srctree)/scripts/depmod.sh $(DEPMOD) \
> $(KERNELRELEASE)
>
> -# Create temporary dir for module support files
> -# clean it up only when building all modules
> -cmd_crmodverdir = $(Q)mkdir -p $(MODVERDIR) \
> - $(if $(KBUILD_MODULES),; rm -f $(MODVERDIR)/*)
> -
> # read saved command lines for existing targets
> existing-targets := $(wildcard $(sort $(targets)))
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 98dede0b2ca8..9fb30633acd2 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -67,8 +67,6 @@ ifeq ($(CONFIG_MODULES)$(need-modorder),y1)
> modorder-target := $(obj)/modules.order
> endif
>
> -# We keep a list of all modules in $(MODVERDIR)
> -
> __build: $(if $(KBUILD_BUILTIN),$(builtin-target) $(lib-target) $(extra-y)) \
> $(if $(KBUILD_MODULES),$(obj-m) $(modorder-target)) \
> $(subdir-ym) $(always)
> @@ -278,13 +276,11 @@ $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE
> $(call cmd,force_checksrc)
> $(call if_changed_rule,cc_o_c)
>
> -# Single-part modules are special since we need to mark them in $(MODVERDIR)
> -
> $(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE
> $(call cmd,force_checksrc)
> $(call if_changed_rule,cc_o_c)
> @{ echo $(@:.o=.ko); echo $@; \
> - $(cmd_undef_syms); } > $(MODVERDIR)/$(@F:.o=.mod)
> + $(cmd_undef_syms); } > $(patsubst %.o,%.mod,$@)
>
> quiet_cmd_cc_lst_c = MKLST $@
> cmd_cc_lst_c = $(CC) $(c_flags) -g -c -o $*.o $< && \
> @@ -466,7 +462,7 @@ cmd_link_multi-m = $(LD) $(ld_flags) -r -o $@ $(filter %.o,$^) $(cmd_secanalysis
> $(multi-used-m): FORCE
> $(call if_changed,link_multi-m)
> @{ echo $(@:.o=.ko); echo $(filter %.o,$^); \
> - $(cmd_undef_syms); } > $(MODVERDIR)/$(@F:.o=.mod)
> + $(cmd_undef_syms); } > $(patsubst %.o,%.mod,$@)
> $(call multi_depend, $(multi-used-m), .o, -objs -y -m)
>
> targets += $(multi-used-m)
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index 2ab1694a7df3..3e229d4f4d72 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -6,8 +6,8 @@
> # Stage one of module building created the following:
> # a) The individual .o files used for the module
> # b) A <module>.o file which is the .o files above linked together
> -# c) A <module>.mod file in $(MODVERDIR)/, listing the name of the
> -# the preliminary <module>.o file, plus all .o files
> +# c) A <module>.mod file, listing the name of the preliminary <module>.o file,
> +# plus all .o files
> # d) modules.order, which lists all the modules
>
> # Stage 2 is handled by this file and does the following
> diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
> index aab4e299d7a2..8b44bb80a451 100755
> --- a/scripts/adjust_autoksyms.sh
> +++ b/scripts/adjust_autoksyms.sh
> @@ -47,13 +47,10 @@ cat > "$new_ksyms_file" << EOT
> */
>
> EOT
> -[ "$(ls -A "$MODVERDIR")" ] &&
> -for mod in "$MODVERDIR"/*.mod; do
> - sed -n -e '3{s/ /\n/g;/^$/!p;}' "$mod"
> -done | sort -u |
> -while read sym; do
> - echo "#define __KSYM_${sym} 1"
> -done >> "$new_ksyms_file"
> +sed 's/ko$/mod/' modules.order |
> +xargs -n1 sed -n -e '3{s/ /\n/g;/^$/!p;}' -- |
> +sort -u |
> +sed -e 's/\(.*\)/#define __KSYM_\1 1/' >> "$new_ksyms_file"
>
> # Special case for modversions (see modpost.c)
> if [ -n "$CONFIG_MODVERSIONS" ]; then
> diff --git a/scripts/mod/sumversion.c b/scripts/mod/sumversion.c
> index 0f6dcb4011a8..166f3fa247a9 100644
> --- a/scripts/mod/sumversion.c
> +++ b/scripts/mod/sumversion.c
> @@ -396,21 +396,11 @@ void get_src_version(const char *modname, char sum[], unsigned sumlen)
> unsigned long len;
> struct md4_ctx md;
> char *sources, *end, *fname;
> - const char *basename;
> char filelist[PATH_MAX + 1];
> - char *modverdir = getenv("MODVERDIR");
>
> - if (!modverdir)
> - modverdir = ".";
> -
> - /* Source files for module are in .tmp_versions/modname.mod,
> - after the first line. */
> - if (strrchr(modname, '/'))
> - basename = strrchr(modname, '/') + 1;
> - else
> - basename = modname;
> - snprintf(filelist, sizeof(filelist), "%s/%.*s.mod", modverdir,
> - (int) strlen(basename) - 2, basename);
> + /* objects for a module are listed in the second line of *.mod file. */
> + snprintf(filelist, sizeof(filelist), "%.*smod",
> + (int)strlen(modname) - 1, modname);
>
> file = grab_file(filelist, &len);
> if (!file)
> diff --git a/scripts/package/mkspec b/scripts/package/mkspec
> index 2d29df4a0a53..8640c278f1aa 100755
> --- a/scripts/package/mkspec
> +++ b/scripts/package/mkspec
> @@ -29,7 +29,7 @@ fi
>
> PROVIDES="$PROVIDES kernel-$KERNELRELEASE"
> __KERNELRELEASE=$(echo $KERNELRELEASE | sed -e "s/-/_/g")
> -EXCLUDES="$RCS_TAR_IGNORE --exclude=.tmp_versions --exclude=*vmlinux* \
> +EXCLUDES="$RCS_TAR_IGNORE --exclude=*vmlinux* --exclude=*.mod \
> --exclude=*.o --exclude=*.ko --exclude=*.cmd --exclude=Documentation \
> --exclude=.config.old --exclude=.missing-syscalls.d --exclude=*.s"
>
> --
> 2.17.1
>
Hi Masahiro,
I'm following this patchset changes as they will affect the klp-convert
series [1] that the livepatching folks have been working on...
Just wondering if these other files should be checked for more MODVERDIR
fallout:
% grep -R 'tmp_versions'
tools/power/cpupower/debug/kernel/Makefile: - rm -rf .tmp_versions* Module.symvers modules.order
scripts/export_report.pl: while (<.tmp_versions/*.mod>) {
scripts/adjust_autoksyms.sh:# .tmp_versions/*.mod files.
export_report.pl is probably the only interesting one on this list.
Also, can you cc me on subsequent patchset versions?
Thanks,
-- Joe
[1] https://lore.kernel.org/lkml/20190509143859.9050-1-joe.lawrence@redhat.com/
^ permalink raw reply
* Re: [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
From: Joel Fernandes @ 2019-07-16 22:02 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716185303.GM14271@linux.ibm.com>
On Tue, Jul 16, 2019 at 11:53:03AM -0700, Paul E. McKenney wrote:
[snip]
> > > A few more things below.
> > > > ---
> > > > include/linux/rculist.h | 28 ++++++++++++++++++++-----
> > > > include/linux/rcupdate.h | 7 +++++++
> > > > kernel/rcu/Kconfig.debug | 11 ++++++++++
> > > > kernel/rcu/update.c | 44 ++++++++++++++++++++++++----------------
> > > > 4 files changed, 67 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > > > index e91ec9ddcd30..1048160625bb 100644
> > > > --- a/include/linux/rculist.h
> > > > +++ b/include/linux/rculist.h
> > > > @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> > > > */
> > > > #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next)))
> > > >
> > > > +/*
> > > > + * Check during list traversal that we are within an RCU reader
> > > > + */
> > > > +
> > > > +#ifdef CONFIG_PROVE_RCU_LIST
> > >
> > > This new Kconfig option is OK temporarily, but unless there is reason to
> > > fear malfunction that a few weeks of rcutorture, 0day, and -next won't
> > > find, it would be better to just use CONFIG_PROVE_RCU. The overall goal
> > > is to reduce the number of RCU knobs rather than grow them, must though
> > > history might lead one to believe otherwise. :-/
> >
> > If you want, we can try to drop this option and just use PROVE_RCU however I
> > must say there may be several warnings that need to be fixed in a short
> > period of time (even a few weeks may be too short) considering the 1000+
> > uses of RCU lists.
> Do many people other than me build with CONFIG_PROVE_RCU? If so, then
> that would be a good reason for a temporary CONFIG_PROVE_RCU_LIST,
> as in going away in a release or two once the warnings get fixed.
PROVE_RCU is enabled by default with PROVE_LOCKING, so it is used quite
heavilty.
> > But I don't mind dropping it and it may just accelerate the fixing up of all
> > callers.
>
> I will let you decide based on the above question. But if you have
> CONFIG_PROVE_RCU_LIST, as noted below, it needs to depend on RCU_EXPERT.
Ok, will make it depend. But yes for temporary purpose, I will leave it as a
config and remove it later.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v5 02/11] of/platform: Add functional dependency link from DT bindings
From: Rob Herring @ 2019-07-16 23:43 UTC (permalink / raw)
To: Saravana Kannan
Cc: Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki, Frank Rowand,
Jonathan Corbet, devicetree, linux-kernel@vger.kernel.org,
David Collins, Android Kernel Team, Linux Doc Mailing List
In-Reply-To: <20190712235245.202558-3-saravanak@google.com>
On Fri, Jul 12, 2019 at 5:52 PM Saravana Kannan <saravanak@google.com> wrote:
>
> Add device-links after the devices are created (but before they are
> probed) by looking at common DT bindings like clocks and
> interconnects.
>
> Automatically adding device-links for functional dependencies at the
> framework level provides the following benefits:
>
> - Optimizes device probe order and avoids the useless work of
> attempting probes of devices that will not probe successfully
> (because their suppliers aren't present or haven't probed yet).
>
> For example, in a commonly available mobile SoC, registering just
> one consumer device's driver at an initcall level earlier than the
> supplier device's driver causes 11 failed probe attempts before the
> consumer device probes successfully. This was with a kernel with all
> the drivers statically compiled in. This problem gets a lot worse if
> all the drivers are loaded as modules without direct symbol
> dependencies.
>
> - Supplier devices like clock providers, interconnect providers, etc
> need to keep the resources they provide active and at a particular
> state(s) during boot up even if their current set of consumers don't
> request the resource to be active. This is because the rest of the
> consumers might not have probed yet and turning off the resource
> before all the consumers have probed could lead to a hang or
> undesired user experience.
>
> Some frameworks (Eg: regulator) handle this today by turning off
> "unused" resources at late_initcall_sync and hoping all the devices
> have probed by then. This is not a valid assumption for systems with
> loadable modules. Other frameworks (Eg: clock) just don't handle
> this due to the lack of a clear signal for when they can turn off
> resources. This leads to downstream hacks to handle cases like this
> that can easily be solved in the upstream kernel.
>
> By linking devices before they are probed, we give suppliers a clear
> count of the number of dependent consumers. Once all of the
> consumers are active, the suppliers can turn off the unused
> resources without making assumptions about the number of consumers.
>
> By default we just add device-links to track "driver presence" (probe
> succeeded) of the supplier device. If any other functionality provided
> by device-links are needed, it is left to the consumer/supplier
> devices to change the link when they probe.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> .../admin-guide/kernel-parameters.txt | 5 ++
> drivers/of/platform.c | 57 +++++++++++++++++++
> 2 files changed, 62 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 138f6664b2e2..109b4310844f 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3141,6 +3141,11 @@
> This can be set from sysctl after boot.
> See Documentation/sysctl/vm.txt for details.
>
> + of_devlink [KNL] Make device links from common DT bindings. Useful
> + for optimizing probe order and making sure resources
> + aren't turned off before the consumer devices have
> + probed.
> +
> ohci1394_dma=early [HW] enable debugging via the ohci1394 driver.
> See Documentation/debugging-via-ohci1394.txt for more
> info.
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 04ad312fd85b..0930f9f89571 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -509,6 +509,62 @@ int of_platform_default_populate(struct device_node *root,
> }
> EXPORT_SYMBOL_GPL(of_platform_default_populate);
>
> +static int of_link_binding(struct device *dev,
> + const char *binding, const char *cell)
> +{
> + struct of_phandle_args sup_args;
> + struct platform_device *sup_dev;
> + unsigned int i = 0, links = 0;
> + u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> +
> + while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
> + &sup_args)) {
> + i++;
> + sup_dev = of_find_device_by_node(sup_args.np);
> + of_node_put(sup_args.np);
> + if (!sup_dev)
> + continue;
> + if (device_link_add(dev, &sup_dev->dev, dl_flags))
> + links++;
> + put_device(&sup_dev->dev);
> + }
> + if (links < i)
> + return -ENODEV;
> + return 0;
> +}
> +
> +static bool of_devlink;
> +core_param(of_devlink, of_devlink, bool, 0);
> +
> +/*
> + * List of bindings and their cell names (use NULL if no cell names) from which
> + * device links need to be created.
> + */
> +static const char * const link_bindings[] = {
> + "clocks", "#clock-cells",
> + "interconnects", "#interconnect-cells",
> +};
> +
> +static int of_link_to_suppliers(struct device *dev)
> +{
> + unsigned int i = 0;
> + bool done = true;
> +
> + if (!of_devlink)
> + return 0;
> + if (unlikely(!dev->of_node))
> + return 0;
> +
> + for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
> + if (of_link_binding(dev, link_bindings[i * 2],
> + link_bindings[i * 2 + 1]))
> + done = false;
Given the pending addition of regulators I think this should be
structured a bit differently so that we abstract out the matching and
phandle look-up so there's a clean separation of binding specifics.
It's kind of messy with 2 patterns to parse already and if we added a
3rd? I would iterate over the properties as you do for regulators in
both cases and for each property call a binding specific match
function. The common pattern can of course be a common function. Let
me know if that makes sense. If not I can try to flesh it out some
more.
Rob
^ permalink raw reply
* Re: [PATCH v5 02/11] of/platform: Add functional dependency link from DT bindings
From: Saravana Kannan @ 2019-07-16 23:53 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki, Frank Rowand,
Jonathan Corbet,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel@vger.kernel.org, David Collins, Android Kernel Team,
Linux Doc Mailing List
In-Reply-To: <CAL_JsqJEmC5cttFavGH4iMh=3z2K4r4kjG44AFJCpxQZ9hPwQA@mail.gmail.com>
On Tue, Jul 16, 2019 at 4:43 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Fri, Jul 12, 2019 at 5:52 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > Add device-links after the devices are created (but before they are
> > probed) by looking at common DT bindings like clocks and
> > interconnects.
> >
> > Automatically adding device-links for functional dependencies at the
> > framework level provides the following benefits:
> >
> > - Optimizes device probe order and avoids the useless work of
> > attempting probes of devices that will not probe successfully
> > (because their suppliers aren't present or haven't probed yet).
> >
> > For example, in a commonly available mobile SoC, registering just
> > one consumer device's driver at an initcall level earlier than the
> > supplier device's driver causes 11 failed probe attempts before the
> > consumer device probes successfully. This was with a kernel with all
> > the drivers statically compiled in. This problem gets a lot worse if
> > all the drivers are loaded as modules without direct symbol
> > dependencies.
> >
> > - Supplier devices like clock providers, interconnect providers, etc
> > need to keep the resources they provide active and at a particular
> > state(s) during boot up even if their current set of consumers don't
> > request the resource to be active. This is because the rest of the
> > consumers might not have probed yet and turning off the resource
> > before all the consumers have probed could lead to a hang or
> > undesired user experience.
> >
> > Some frameworks (Eg: regulator) handle this today by turning off
> > "unused" resources at late_initcall_sync and hoping all the devices
> > have probed by then. This is not a valid assumption for systems with
> > loadable modules. Other frameworks (Eg: clock) just don't handle
> > this due to the lack of a clear signal for when they can turn off
> > resources. This leads to downstream hacks to handle cases like this
> > that can easily be solved in the upstream kernel.
> >
> > By linking devices before they are probed, we give suppliers a clear
> > count of the number of dependent consumers. Once all of the
> > consumers are active, the suppliers can turn off the unused
> > resources without making assumptions about the number of consumers.
> >
> > By default we just add device-links to track "driver presence" (probe
> > succeeded) of the supplier device. If any other functionality provided
> > by device-links are needed, it is left to the consumer/supplier
> > devices to change the link when they probe.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> > .../admin-guide/kernel-parameters.txt | 5 ++
> > drivers/of/platform.c | 57 +++++++++++++++++++
> > 2 files changed, 62 insertions(+)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 138f6664b2e2..109b4310844f 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -3141,6 +3141,11 @@
> > This can be set from sysctl after boot.
> > See Documentation/sysctl/vm.txt for details.
> >
> > + of_devlink [KNL] Make device links from common DT bindings. Useful
> > + for optimizing probe order and making sure resources
> > + aren't turned off before the consumer devices have
> > + probed.
> > +
> > ohci1394_dma=early [HW] enable debugging via the ohci1394 driver.
> > See Documentation/debugging-via-ohci1394.txt for more
> > info.
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index 04ad312fd85b..0930f9f89571 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -509,6 +509,62 @@ int of_platform_default_populate(struct device_node *root,
> > }
> > EXPORT_SYMBOL_GPL(of_platform_default_populate);
> >
> > +static int of_link_binding(struct device *dev,
> > + const char *binding, const char *cell)
> > +{
> > + struct of_phandle_args sup_args;
> > + struct platform_device *sup_dev;
> > + unsigned int i = 0, links = 0;
> > + u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> > +
> > + while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
> > + &sup_args)) {
> > + i++;
> > + sup_dev = of_find_device_by_node(sup_args.np);
> > + of_node_put(sup_args.np);
> > + if (!sup_dev)
> > + continue;
> > + if (device_link_add(dev, &sup_dev->dev, dl_flags))
> > + links++;
> > + put_device(&sup_dev->dev);
> > + }
> > + if (links < i)
> > + return -ENODEV;
> > + return 0;
> > +}
> > +
> > +static bool of_devlink;
> > +core_param(of_devlink, of_devlink, bool, 0);
> > +
> > +/*
> > + * List of bindings and their cell names (use NULL if no cell names) from which
> > + * device links need to be created.
> > + */
> > +static const char * const link_bindings[] = {
> > + "clocks", "#clock-cells",
> > + "interconnects", "#interconnect-cells",
> > +};
> > +
> > +static int of_link_to_suppliers(struct device *dev)
> > +{
> > + unsigned int i = 0;
> > + bool done = true;
> > +
> > + if (!of_devlink)
> > + return 0;
> > + if (unlikely(!dev->of_node))
> > + return 0;
> > +
> > + for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
> > + if (of_link_binding(dev, link_bindings[i * 2],
> > + link_bindings[i * 2 + 1]))
> > + done = false;
>
> Given the pending addition of regulators I think this should be
> structured a bit differently so that we abstract out the matching and
> phandle look-up so there's a clean separation of binding specifics.
> It's kind of messy with 2 patterns to parse already and if we added a
> 3rd? I would iterate over the properties as you do for regulators in
> both cases and for each property call a binding specific match
> function. The common pattern can of course be a common function. Let
> me know if that makes sense. If not I can try to flesh it out some
> more.
I've added regulator support in this series and I've refactored this
code as I went along. I fully expect to squash some of the refactors
once the final result of the series is acceptable.
It's not clear to me if you got to the end of the series and still
think the final result needs to be refactored. Let me know what you
think about this towards the end of the series and I can clean it up
if you still think it needs some clean up.
-Saravana
^ permalink raw reply
* Re: [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
From: Paul E. McKenney @ 2019-07-17 0:07 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716220205.GB172157@google.com>
On Tue, Jul 16, 2019 at 06:02:05PM -0400, Joel Fernandes wrote:
> On Tue, Jul 16, 2019 at 11:53:03AM -0700, Paul E. McKenney wrote:
> [snip]
> > > > A few more things below.
> > > > > ---
> > > > > include/linux/rculist.h | 28 ++++++++++++++++++++-----
> > > > > include/linux/rcupdate.h | 7 +++++++
> > > > > kernel/rcu/Kconfig.debug | 11 ++++++++++
> > > > > kernel/rcu/update.c | 44 ++++++++++++++++++++++++----------------
> > > > > 4 files changed, 67 insertions(+), 23 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > > > > index e91ec9ddcd30..1048160625bb 100644
> > > > > --- a/include/linux/rculist.h
> > > > > +++ b/include/linux/rculist.h
> > > > > @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> > > > > */
> > > > > #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next)))
> > > > >
> > > > > +/*
> > > > > + * Check during list traversal that we are within an RCU reader
> > > > > + */
> > > > > +
> > > > > +#ifdef CONFIG_PROVE_RCU_LIST
> > > >
> > > > This new Kconfig option is OK temporarily, but unless there is reason to
> > > > fear malfunction that a few weeks of rcutorture, 0day, and -next won't
> > > > find, it would be better to just use CONFIG_PROVE_RCU. The overall goal
> > > > is to reduce the number of RCU knobs rather than grow them, must though
> > > > history might lead one to believe otherwise. :-/
> > >
> > > If you want, we can try to drop this option and just use PROVE_RCU however I
> > > must say there may be several warnings that need to be fixed in a short
> > > period of time (even a few weeks may be too short) considering the 1000+
> > > uses of RCU lists.
> > Do many people other than me build with CONFIG_PROVE_RCU? If so, then
> > that would be a good reason for a temporary CONFIG_PROVE_RCU_LIST,
> > as in going away in a release or two once the warnings get fixed.
>
> PROVE_RCU is enabled by default with PROVE_LOCKING, so it is used quite
> heavilty.
>
> > > But I don't mind dropping it and it may just accelerate the fixing up of all
> > > callers.
> >
> > I will let you decide based on the above question. But if you have
> > CONFIG_PROVE_RCU_LIST, as noted below, it needs to depend on RCU_EXPERT.
>
> Ok, will make it depend. But yes for temporary purpose, I will leave it as a
> config and remove it later.
Very good, thank you! Plus you got another ack. ;-)
Thanx, Paul
^ permalink raw reply
* [PATCH v11 00/18] kunit: introduce KUnit, the Linux kernel unit testing framework
From: Brendan Higgins @ 2019-07-17 1:55 UTC (permalink / raw)
To: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, sboyd, shuah, tytso, yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins, Michal Marek, Jonathan Corbet, Iurii Zaikin
## TL;DR
This patchset addresses comments from Stephen Boyd. No changes affect
the API, and all changes are specific to patches 02, 03, and 04;
however, there were some significant changes to how string_stream and
kunit_stream work under the hood.
## Background
This patch set proposes KUnit, a lightweight unit testing and mocking
framework for the Linux kernel.
Unlike Autotest and kselftest, KUnit is a true unit testing framework;
it does not require installing the kernel on a test machine or in a VM
(however, KUnit still allows you to run tests on test machines or in VMs
if you want[1]) and does not require tests to be written in userspace
running on a host kernel. Additionally, KUnit is fast: From invocation
to completion KUnit can run several dozen tests in about a second.
Currently, the entire KUnit test suite for KUnit runs in under a second
from the initial invocation (build time excluded).
KUnit is heavily inspired by JUnit, Python's unittest.mock, and
Googletest/Googlemock for C++. KUnit provides facilities for defining
unit test cases, grouping related test cases into test suites, providing
common infrastructure for running tests, mocking, spying, and much more.
### What's so special about unit testing?
A unit test is supposed to test a single unit of code in isolation,
hence the name. There should be no dependencies outside the control of
the test; this means no external dependencies, which makes tests orders
of magnitudes faster. Likewise, since there are no external dependencies,
there are no hoops to jump through to run the tests. Additionally, this
makes unit tests deterministic: a failing unit test always indicates a
problem. Finally, because unit tests necessarily have finer granularity,
they are able to test all code paths easily solving the classic problem
of difficulty in exercising error handling code.
### Is KUnit trying to replace other testing frameworks for the kernel?
No. Most existing tests for the Linux kernel are end-to-end tests, which
have their place. A well tested system has lots of unit tests, a
reasonable number of integration tests, and some end-to-end tests. KUnit
is just trying to address the unit test space which is currently not
being addressed.
### More information on KUnit
There is a bunch of documentation near the end of this patch set that
describes how to use KUnit and best practices for writing unit tests.
For convenience I am hosting the compiled docs here[2].
Additionally for convenience, I have applied these patches to a
branch[3]. The repo may be cloned with:
git clone https://kunit.googlesource.com/linux
This patchset is on the kunit/rfc/v5.2/v11 branch.
## Changes Since Last Version
- Went back to using spinlock in `struct string_stream`. Needed for so
that it is compatible with different GFP flags to address comment from
Stephen.
- Added string_stream_append function to string_stream API. - suggested
by Stephen.
- Made all string fragments and other allocations internal to
string_stream and kunit_stream managed by the KUnit resource
management API.
[1] https://google.github.io/kunit-docs/third_party/kernel/docs/usage.html#kunit-on-non-uml-architectures
[2] https://google.github.io/kunit-docs/third_party/kernel/docs/
[3] https://kunit.googlesource.com/linux/+/kunit/rfc/v5.2/v11
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply
* [PATCH v11 01/18] kunit: test: add KUnit test runner core
From: Brendan Higgins @ 2019-07-17 1:55 UTC (permalink / raw)
To: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, sboyd, shuah, tytso, yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190717015543.152251-1-brendanhiggins@google.com>
Add core facilities for defining unit tests; this provides a common way
to define test cases, functions that execute code which is under test
and determine whether the code under test behaves as expected; this also
provides a way to group together related test cases in test suites (here
we call them test_modules).
Just define test cases and how to execute them for now; setting
expectations on code will be defined later.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
include/kunit/test.h | 179 ++++++++++++++++++++++++++++++++++++++++
kunit/Kconfig | 17 ++++
kunit/Makefile | 1 +
kunit/test.c | 191 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 388 insertions(+)
create mode 100644 include/kunit/test.h
create mode 100644 kunit/Kconfig
create mode 100644 kunit/Makefile
create mode 100644 kunit/test.c
diff --git a/include/kunit/test.h b/include/kunit/test.h
new file mode 100644
index 0000000000000..e0b34acb9ee4e
--- /dev/null
+++ b/include/kunit/test.h
@@ -0,0 +1,179 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Base unit test (KUnit) API.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#ifndef _KUNIT_TEST_H
+#define _KUNIT_TEST_H
+
+#include <linux/types.h>
+
+struct kunit;
+
+/**
+ * struct kunit_case - represents an individual test case.
+ * @run_case: the function representing the actual test case.
+ * @name: the name of the test case.
+ *
+ * A test case is a function with the signature, ``void (*)(struct kunit *)``
+ * that makes expectations (see KUNIT_EXPECT_TRUE()) about code under test. Each
+ * test case is associated with a &struct kunit_suite and will be run after the
+ * suite's init function and followed by the suite's exit function.
+ *
+ * A test case should be static and should only be created with the KUNIT_CASE()
+ * macro; additionally, every array of test cases should be terminated with an
+ * empty test case.
+ *
+ * Example:
+ *
+ * .. code-block:: c
+ *
+ * void add_test_basic(struct kunit *test)
+ * {
+ * KUNIT_EXPECT_EQ(test, 1, add(1, 0));
+ * KUNIT_EXPECT_EQ(test, 2, add(1, 1));
+ * KUNIT_EXPECT_EQ(test, 0, add(-1, 1));
+ * KUNIT_EXPECT_EQ(test, INT_MAX, add(0, INT_MAX));
+ * KUNIT_EXPECT_EQ(test, -1, add(INT_MAX, INT_MIN));
+ * }
+ *
+ * static struct kunit_case example_test_cases[] = {
+ * KUNIT_CASE(add_test_basic),
+ * {}
+ * };
+ *
+ */
+struct kunit_case {
+ void (*run_case)(struct kunit *test);
+ const char *name;
+
+ /* private: internal use only. */
+ bool success;
+};
+
+/**
+ * KUNIT_CASE - A helper for creating a &struct kunit_case
+ * @test_name: a reference to a test case function.
+ *
+ * Takes a symbol for a function representing a test case and creates a
+ * &struct kunit_case object from it. See the documentation for
+ * &struct kunit_case for an example on how to use it.
+ */
+#define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
+
+/**
+ * struct kunit_suite - describes a related collection of &struct kunit_case s.
+ * @name: the name of the test. Purely informational.
+ * @init: called before every test case.
+ * @exit: called after every test case.
+ * @test_cases: a null terminated array of test cases.
+ *
+ * A kunit_suite is a collection of related &struct kunit_case s, such that
+ * @init is called before every test case and @exit is called after every test
+ * case, similar to the notion of a *test fixture* or a *test class* in other
+ * unit testing frameworks like JUnit or Googletest.
+ *
+ * Every &struct kunit_case must be associated with a kunit_suite for KUnit to
+ * run it.
+ */
+struct kunit_suite {
+ const char name[256];
+ int (*init)(struct kunit *test);
+ void (*exit)(struct kunit *test);
+ struct kunit_case *test_cases;
+};
+
+/**
+ * struct kunit - represents a running instance of a test.
+ * @priv: for user to store arbitrary data. Commonly used to pass data created
+ * in the init function (see &struct kunit_suite).
+ *
+ * Used to store information about the current context under which the test is
+ * running. Most of this data is private and should only be accessed indirectly
+ * via public functions; the one exception is @priv which can be used by the
+ * test writer to store arbitrary data.
+ */
+struct kunit {
+ void *priv;
+
+ /* private: internal use only. */
+ const char *name; /* Read only after initialization! */
+ /*
+ * success starts as true, and may only be set to false during a test
+ * case; thus, it is safe to update this across multiple threads using
+ * WRITE_ONCE; however, as a consequence, it may only be read after the
+ * test case finishes once all threads associated with the test case
+ * have terminated.
+ */
+ bool success; /* Read only after test_case finishes! */
+};
+
+void kunit_init_test(struct kunit *test, const char *name);
+
+int kunit_run_tests(struct kunit_suite *suite);
+
+/**
+ * kunit_test_suite() - used to register a &struct kunit_suite with KUnit.
+ * @suite: a statically allocated &struct kunit_suite.
+ *
+ * Registers @suite with the test framework. See &struct kunit_suite for more
+ * information.
+ *
+ * NOTE: Currently KUnit tests are all run as late_initcalls; this means that
+ * they cannot test anything where tests must run at a different init phase. One
+ * significant restriction resulting from this is that KUnit cannot reliably
+ * test anything that is initialize in the late_init phase; another is that
+ * KUnit is useless to test things that need to be run in an earlier init phase.
+ */
+#define kunit_test_suite(suite) \
+ /*
+ * TODO(brendanhiggins@google.com): Don't run all KUnit tests as
+ * late_initcalls. I have some future work planned to dispatch
+ * all KUnit tests from the same place, and at the very least to
+ * do so after everything else is definitely initialized.
+ */ \
+ static int kunit_suite_init##suite(void) \
+ { \
+ return kunit_run_tests(&suite); \
+ } \
+ late_initcall(kunit_suite_init##suite)
+
+void __printf(3, 4) kunit_printk(const char *level,
+ const struct kunit *test,
+ const char *fmt, ...);
+
+/**
+ * kunit_info() - Prints an INFO level message associated with the current test.
+ * @test: The test context object.
+ * @fmt: A printk() style format string.
+ *
+ * Prints an info level message associated with the test suite being run. Takes
+ * a variable number of format parameters just like printk().
+ */
+#define kunit_info(test, fmt, ...) \
+ kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__)
+
+/**
+ * kunit_warn() - Prints a WARN level message associated with the current test.
+ * @test: The test context object.
+ * @fmt: A printk() style format string.
+ *
+ * Prints a warning level message.
+ */
+#define kunit_warn(test, fmt, ...) \
+ kunit_printk(KERN_WARNING, test, fmt, ##__VA_ARGS__)
+
+/**
+ * kunit_err() - Prints an ERROR level message associated with the current test.
+ * @test: The test context object.
+ * @fmt: A printk() style format string.
+ *
+ * Prints an error level message.
+ */
+#define kunit_err(test, fmt, ...) \
+ kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
+
+#endif /* _KUNIT_TEST_H */
diff --git a/kunit/Kconfig b/kunit/Kconfig
new file mode 100644
index 0000000000000..330ae83527c23
--- /dev/null
+++ b/kunit/Kconfig
@@ -0,0 +1,17 @@
+#
+# KUnit base configuration
+#
+
+menu "KUnit support"
+
+config KUNIT
+ bool "Enable support for unit tests (KUnit)"
+ help
+ Enables support for kernel unit tests (KUnit), a lightweight unit
+ testing and mocking framework for the Linux kernel. These tests are
+ able to be run locally on a developer's workstation without a VM or
+ special hardware when using UML. Can also be used on most other
+ architectures. For more information, please see
+ Documentation/dev-tools/kunit/.
+
+endmenu
diff --git a/kunit/Makefile b/kunit/Makefile
new file mode 100644
index 0000000000000..5efdc4dea2c08
--- /dev/null
+++ b/kunit/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_KUNIT) += test.o
diff --git a/kunit/test.c b/kunit/test.c
new file mode 100644
index 0000000000000..d302cff0f1dc7
--- /dev/null
+++ b/kunit/test.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Base unit test (KUnit) API.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#include <linux/kernel.h>
+#include <kunit/test.h>
+
+static void kunit_set_failure(struct kunit *test)
+{
+ WRITE_ONCE(test->success, false);
+}
+
+static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
+{
+ return vprintk_emit(0, level, NULL, 0, fmt, args);
+}
+
+static int kunit_printk_emit(int level, const char *fmt, ...)
+{
+ va_list args;
+ int ret;
+
+ va_start(args, fmt);
+ ret = kunit_vprintk_emit(level, fmt, args);
+ va_end(args);
+
+ return ret;
+}
+
+static void kunit_vprintk(const struct kunit *test,
+ const char *level,
+ struct va_format *vaf)
+{
+ kunit_printk_emit(level[1] - '0', "\t# %s: %pV", test->name, vaf);
+}
+
+static void kunit_print_tap_version(void)
+{
+ static bool kunit_has_printed_tap_version;
+
+ if (!kunit_has_printed_tap_version) {
+ kunit_printk_emit(LOGLEVEL_INFO, "TAP version 14\n");
+ kunit_has_printed_tap_version = true;
+ }
+}
+
+static size_t kunit_test_cases_len(struct kunit_case *test_cases)
+{
+ struct kunit_case *test_case;
+ size_t len = 0;
+
+ for (test_case = test_cases; test_case->run_case; test_case++)
+ len++;
+
+ return len;
+}
+
+static void kunit_print_subtest_start(struct kunit_suite *suite)
+{
+ kunit_print_tap_version();
+ kunit_printk_emit(LOGLEVEL_INFO, "\t# Subtest: %s\n", suite->name);
+ kunit_printk_emit(LOGLEVEL_INFO,
+ "\t1..%zd\n",
+ kunit_test_cases_len(suite->test_cases));
+}
+
+static void kunit_print_ok_not_ok(bool should_indent,
+ bool is_ok,
+ size_t test_number,
+ const char *description)
+{
+ const char *indent, *ok_not_ok;
+
+ if (should_indent)
+ indent = "\t";
+ else
+ indent = "";
+
+ if (is_ok)
+ ok_not_ok = "ok";
+ else
+ ok_not_ok = "not ok";
+
+ kunit_printk_emit(LOGLEVEL_INFO,
+ "%s%s %zd - %s\n",
+ indent, ok_not_ok, test_number, description);
+}
+
+static bool kunit_suite_has_succeeded(struct kunit_suite *suite)
+{
+ const struct kunit_case *test_case;
+
+ for (test_case = suite->test_cases; test_case->run_case; test_case++)
+ if (!test_case->success)
+ return false;
+
+ return true;
+}
+
+static void kunit_print_subtest_end(struct kunit_suite *suite)
+{
+ static size_t kunit_suite_counter = 1;
+
+ kunit_print_ok_not_ok(false,
+ kunit_suite_has_succeeded(suite),
+ kunit_suite_counter++,
+ suite->name);
+}
+
+static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case,
+ size_t test_number)
+{
+ kunit_print_ok_not_ok(true,
+ test_case->success,
+ test_number,
+ test_case->name);
+}
+
+void kunit_init_test(struct kunit *test, const char *name)
+{
+ test->name = name;
+ test->success = true;
+}
+
+/*
+ * Performs all logic to run a test case.
+ */
+static void kunit_run_case(struct kunit_suite *suite,
+ struct kunit_case *test_case)
+{
+ struct kunit test;
+
+ kunit_init_test(&test, test_case->name);
+
+ if (suite->init) {
+ int ret;
+
+ ret = suite->init(&test);
+ if (ret) {
+ kunit_err(&test, "failed to initialize: %d\n", ret);
+ kunit_set_failure(&test);
+ test_case->success = test.success;
+ return;
+ }
+ }
+
+ test_case->run_case(&test);
+
+ if (suite->exit)
+ suite->exit(&test);
+
+ test_case->success = test.success;
+}
+
+int kunit_run_tests(struct kunit_suite *suite)
+{
+ struct kunit_case *test_case;
+ size_t test_case_count = 1;
+
+ kunit_print_subtest_start(suite);
+
+ for (test_case = suite->test_cases; test_case->run_case; test_case++) {
+ kunit_run_case(suite, test_case);
+ kunit_print_test_case_ok_not_ok(test_case, test_case_count++);
+ }
+
+ kunit_print_subtest_end(suite);
+
+ return 0;
+}
+
+void kunit_printk(const char *level,
+ const struct kunit *test,
+ const char *fmt, ...)
+{
+ struct va_format vaf;
+ va_list args;
+
+ va_start(args, fmt);
+
+ vaf.fmt = fmt;
+ vaf.va = &args;
+
+ kunit_vprintk(test, level, &vaf);
+
+ va_end(args);
+}
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
* [PATCH v11 02/18] kunit: test: add test resource management API
From: Brendan Higgins @ 2019-07-17 1:55 UTC (permalink / raw)
To: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, sboyd, shuah, tytso, yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190717015543.152251-1-brendanhiggins@google.com>
Create a common API for test managed resources like memory and test
objects. A lot of times a test will want to set up infrastructure to be
used in test cases; this could be anything from just wanting to allocate
some memory to setting up a driver stack; this defines facilities for
creating "test resources" which are managed by the test infrastructure
and are automatically cleaned up at the conclusion of the test.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
include/kunit/test.h | 143 +++++++++++++++++++++++++++++++++++++++++++
kunit/test.c | 92 ++++++++++++++++++++++++++++
2 files changed, 235 insertions(+)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index e0b34acb9ee4e..d0bf112910caf 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -10,6 +10,70 @@
#define _KUNIT_TEST_H
#include <linux/types.h>
+#include <linux/slab.h>
+
+struct kunit_resource;
+
+typedef int (*kunit_resource_init_t)(struct kunit_resource *, void *);
+typedef void (*kunit_resource_free_t)(struct kunit_resource *);
+
+/**
+ * struct kunit_resource - represents a *test managed resource*
+ * @allocation: for the user to store arbitrary data.
+ * @free: a user supplied function to free the resource. Populated by
+ * kunit_alloc_resource().
+ *
+ * Represents a *test managed resource*, a resource which will automatically be
+ * cleaned up at the end of a test case.
+ *
+ * Example:
+ *
+ * .. code-block:: c
+ *
+ * struct kunit_kmalloc_params {
+ * size_t size;
+ * gfp_t gfp;
+ * };
+ *
+ * static int kunit_kmalloc_init(struct kunit_resource *res, void *context)
+ * {
+ * struct kunit_kmalloc_params *params = context;
+ * res->allocation = kmalloc(params->size, params->gfp);
+ *
+ * if (!res->allocation)
+ * return -ENOMEM;
+ *
+ * return 0;
+ * }
+ *
+ * static void kunit_kmalloc_free(struct kunit_resource *res)
+ * {
+ * kfree(res->allocation);
+ * }
+ *
+ * void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp)
+ * {
+ * struct kunit_kmalloc_params params;
+ * struct kunit_resource *res;
+ *
+ * params.size = size;
+ * params.gfp = gfp;
+ *
+ * res = kunit_alloc_resource(test, kunit_kmalloc_init,
+ * kunit_kmalloc_free, ¶ms);
+ * if (res)
+ * return res->allocation;
+ *
+ * return NULL;
+ * }
+ */
+struct kunit_resource {
+ void *allocation;
+ kunit_resource_free_t free;
+
+ /* private: internal use only. */
+ struct list_head node;
+};
struct kunit;
@@ -109,6 +173,13 @@ struct kunit {
* have terminated.
*/
bool success; /* Read only after test_case finishes! */
+ spinlock_t lock; /* Gaurds all mutable test state. */
+ /*
+ * Because resources is a list that may be updated multiple times (with
+ * new resources) from any thread associated with a test case, we must
+ * protect it with some type of lock.
+ */
+ struct list_head resources; /* Protected by lock. */
};
void kunit_init_test(struct kunit *test, const char *name);
@@ -141,6 +212,78 @@ int kunit_run_tests(struct kunit_suite *suite);
} \
late_initcall(kunit_suite_init##suite)
+/*
+ * Like kunit_alloc_resource() below, but returns the struct kunit_resource
+ * object that contains the allocation. This is mostly for testing purposes.
+ */
+struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test,
+ kunit_resource_init_t init,
+ kunit_resource_free_t free,
+ gfp_t internal_gfp,
+ void *context);
+
+/**
+ * kunit_alloc_resource() - Allocates a *test managed resource*.
+ * @test: The test context object.
+ * @init: a user supplied function to initialize the resource.
+ * @free: a user supplied function to free the resource.
+ * @internal_gfp: gfp to use for internal allocations, if unsure, use GFP_KERNEL
+ * @context: for the user to pass in arbitrary data to the init function.
+ *
+ * Allocates a *test managed resource*, a resource which will automatically be
+ * cleaned up at the end of a test case. See &struct kunit_resource for an
+ * example.
+ *
+ * NOTE: KUnit needs to allocate memory for each kunit_resource object. You must
+ * specify an @internal_gfp that is compatible with the use context of your
+ * resource.
+ */
+static inline void *kunit_alloc_resource(struct kunit *test,
+ kunit_resource_init_t init,
+ kunit_resource_free_t free,
+ gfp_t internal_gfp,
+ void *context)
+{
+ struct kunit_resource *res;
+
+ res = kunit_alloc_and_get_resource(test, init, free, internal_gfp,
+ context);
+
+ if (res)
+ return res->allocation;
+
+ return NULL;
+}
+
+void kunit_free_resource(struct kunit *test, struct kunit_resource *res);
+
+/**
+ * kunit_kmalloc() - Like kmalloc() except the allocation is *test managed*.
+ * @test: The test context object.
+ * @size: The size in bytes of the desired memory.
+ * @gfp: flags passed to underlying kmalloc().
+ *
+ * Just like `kmalloc(...)`, except the allocation is managed by the test case
+ * and is automatically cleaned up after the test case concludes. See &struct
+ * kunit_resource for more information.
+ */
+void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp);
+
+/**
+ * kunit_kzalloc() - Just like kunit_kmalloc(), but zeroes the allocation.
+ * @test: The test context object.
+ * @size: The size in bytes of the desired memory.
+ * @gfp: flags passed to underlying kmalloc().
+ *
+ * See kzalloc() and kunit_kmalloc() for more information.
+ */
+static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
+{
+ return kunit_kmalloc(test, size, gfp | __GFP_ZERO);
+}
+
+void kunit_cleanup(struct kunit *test);
+
void __printf(3, 4) kunit_printk(const char *level,
const struct kunit *test,
const char *fmt, ...);
diff --git a/kunit/test.c b/kunit/test.c
index d302cff0f1dc7..4c178a817f2fe 100644
--- a/kunit/test.c
+++ b/kunit/test.c
@@ -122,6 +122,8 @@ static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case,
void kunit_init_test(struct kunit *test, const char *name)
{
+ spin_lock_init(&test->lock);
+ INIT_LIST_HEAD(&test->resources);
test->name = name;
test->success = true;
}
@@ -153,6 +155,8 @@ static void kunit_run_case(struct kunit_suite *suite,
if (suite->exit)
suite->exit(&test);
+ kunit_cleanup(&test);
+
test_case->success = test.success;
}
@@ -173,6 +177,94 @@ int kunit_run_tests(struct kunit_suite *suite)
return 0;
}
+struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test,
+ kunit_resource_init_t init,
+ kunit_resource_free_t free,
+ gfp_t internal_gfp,
+ void *context)
+{
+ struct kunit_resource *res;
+ int ret;
+
+ res = kzalloc(sizeof(*res), internal_gfp);
+ if (!res)
+ return NULL;
+
+ ret = init(res, context);
+ if (ret)
+ return NULL;
+
+ res->free = free;
+ spin_lock(&test->lock);
+ list_add_tail(&res->node, &test->resources);
+ spin_unlock(&test->lock);
+
+ return res;
+}
+
+void kunit_free_resource(struct kunit *test, struct kunit_resource *res)
+{
+ lockdep_assert_held(&test->lock);
+
+ res->free(res);
+ list_del(&res->node);
+ kfree(res);
+}
+
+struct kunit_kmalloc_params {
+ size_t size;
+ gfp_t gfp;
+};
+
+static int kunit_kmalloc_init(struct kunit_resource *res, void *context)
+{
+ struct kunit_kmalloc_params *params = context;
+
+ res->allocation = kmalloc(params->size, params->gfp);
+ if (!res->allocation)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static void kunit_kmalloc_free(struct kunit_resource *res)
+{
+ kfree(res->allocation);
+}
+
+void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp)
+{
+ struct kunit_kmalloc_params params = {
+ .size = size,
+ .gfp = gfp
+ };
+
+ return kunit_alloc_resource(test,
+ kunit_kmalloc_init,
+ kunit_kmalloc_free,
+ gfp,
+ ¶ms);
+}
+
+void kunit_cleanup(struct kunit *test)
+{
+ struct kunit_resource *resource, *resource_safe;
+
+ spin_lock(&test->lock);
+ /*
+ * test->resources is a stack - each allocation must be freed in the
+ * reverse order from which it was added since one resource may depend
+ * on another for its entire lifetime.
+ */
+ list_for_each_entry_safe_reverse(resource,
+ resource_safe,
+ &test->resources,
+ node) {
+ kunit_free_resource(test, resource);
+ }
+ spin_unlock(&test->lock);
+}
+
void kunit_printk(const char *level,
const struct kunit *test,
const char *fmt, ...)
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
* [PATCH v11 03/18] kunit: test: add string_stream a std::stream like string builder
From: Brendan Higgins @ 2019-07-17 1:55 UTC (permalink / raw)
To: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, sboyd, shuah, tytso, yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190717015543.152251-1-brendanhiggins@google.com>
A number of test features need to do pretty complicated string printing
where it may not be possible to rely on a single preallocated string
with parameters.
So provide a library for constructing the string as you go similar to
C++'s std::string. string_stream is really just a string builder,
nothing more.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
include/kunit/string-stream.h | 49 +++++++++++
kunit/Makefile | 3 +-
kunit/string-stream.c | 152 ++++++++++++++++++++++++++++++++++
3 files changed, 203 insertions(+), 1 deletion(-)
create mode 100644 include/kunit/string-stream.h
create mode 100644 kunit/string-stream.c
diff --git a/include/kunit/string-stream.h b/include/kunit/string-stream.h
new file mode 100644
index 0000000000000..4fa107e38deb5
--- /dev/null
+++ b/include/kunit/string-stream.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * C++ stream style string builder used in KUnit for building messages.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#ifndef _KUNIT_STRING_STREAM_H
+#define _KUNIT_STRING_STREAM_H
+
+#include <linux/types.h>
+#include <linux/spinlock.h>
+#include <stdarg.h>
+
+struct string_stream_fragment {
+ struct list_head node;
+ char *fragment;
+};
+
+struct string_stream {
+ size_t length;
+ struct list_head fragments;
+ /* length and fragments are protected by this lock */
+ spinlock_t lock;
+ struct kunit *test;
+ gfp_t gfp;
+};
+
+struct kunit;
+
+struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp);
+
+int string_stream_add(struct string_stream *stream, const char *fmt, ...);
+
+int string_stream_vadd(struct string_stream *stream,
+ const char *fmt,
+ va_list args);
+
+char *string_stream_get_string(struct string_stream *stream);
+
+int string_stream_append(struct string_stream *stream,
+ struct string_stream *other);
+
+void string_stream_clear(struct string_stream *stream);
+
+bool string_stream_is_empty(struct string_stream *stream);
+
+#endif /* _KUNIT_STRING_STREAM_H */
diff --git a/kunit/Makefile b/kunit/Makefile
index 5efdc4dea2c08..275b565a0e81f 100644
--- a/kunit/Makefile
+++ b/kunit/Makefile
@@ -1 +1,2 @@
-obj-$(CONFIG_KUNIT) += test.o
+obj-$(CONFIG_KUNIT) += test.o \
+ string-stream.o
diff --git a/kunit/string-stream.c b/kunit/string-stream.c
new file mode 100644
index 0000000000000..bcd56d6755544
--- /dev/null
+++ b/kunit/string-stream.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * C++ stream style string builder used in KUnit for building messages.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <kunit/string-stream.h>
+#include <kunit/test.h>
+
+int string_stream_vadd(struct string_stream *stream,
+ const char *fmt,
+ va_list args)
+{
+ struct string_stream_fragment *frag_container;
+ int len;
+ va_list args_for_counting;
+
+ /* Make a copy because `vsnprintf` could change it */
+ va_copy(args_for_counting, args);
+
+ /* Need space for null byte. */
+ len = vsnprintf(NULL, 0, fmt, args_for_counting) + 1;
+
+ va_end(args_for_counting);
+
+ frag_container = kunit_kmalloc(stream->test, sizeof(*frag_container),
+ stream->gfp);
+ if (!frag_container)
+ return -ENOMEM;
+
+ frag_container->fragment = kunit_kmalloc(stream->test, len,
+ stream->gfp);
+ if (!frag_container->fragment)
+ return -ENOMEM;
+
+ len = vsnprintf(frag_container->fragment, len, fmt, args);
+ spin_lock(&stream->lock);
+ stream->length += len;
+ list_add_tail(&frag_container->node, &stream->fragments);
+ spin_unlock(&stream->lock);
+
+ return 0;
+}
+
+int string_stream_add(struct string_stream *stream, const char *fmt, ...)
+{
+ va_list args;
+ int result;
+
+ va_start(args, fmt);
+ result = string_stream_vadd(stream, fmt, args);
+ va_end(args);
+
+ return result;
+}
+
+void string_stream_clear(struct string_stream *stream)
+{
+ struct string_stream_fragment *frag_container, *frag_container_safe;
+
+ spin_lock(&stream->lock);
+ list_for_each_entry_safe(frag_container,
+ frag_container_safe,
+ &stream->fragments,
+ node) {
+ list_del(&frag_container->node);
+ }
+ stream->length = 0;
+ spin_unlock(&stream->lock);
+}
+
+char *string_stream_get_string(struct string_stream *stream)
+{
+ struct string_stream_fragment *frag_container;
+ size_t buf_len = stream->length + 1; /* +1 for null byte. */
+ char *buf;
+
+ buf = kunit_kzalloc(stream->test, buf_len, stream->gfp);
+ if (!buf)
+ return NULL;
+
+ spin_lock(&stream->lock);
+ list_for_each_entry(frag_container, &stream->fragments, node)
+ strlcat(buf, frag_container->fragment, buf_len);
+ spin_unlock(&stream->lock);
+
+ return buf;
+}
+
+int string_stream_append(struct string_stream *stream,
+ struct string_stream *other)
+{
+ const char *other_content;
+
+ other_content = string_stream_get_string(other);
+
+ if (!other_content)
+ return -ENOMEM;
+
+ return string_stream_add(stream, other_content);
+}
+
+bool string_stream_is_empty(struct string_stream *stream)
+{
+ return list_empty(&stream->fragments);
+}
+
+struct string_stream_alloc_context {
+ struct kunit *test;
+ gfp_t gfp;
+};
+
+static int string_stream_init(struct kunit_resource *res, void *context)
+{
+ struct string_stream *stream;
+ struct string_stream_alloc_context *ctx = context;
+
+ stream = kunit_kzalloc(ctx->test, sizeof(*stream), ctx->gfp);
+ if (!stream)
+ return -ENOMEM;
+
+ res->allocation = stream;
+ stream->gfp = ctx->gfp;
+ stream->test = ctx->test;
+ INIT_LIST_HEAD(&stream->fragments);
+ spin_lock_init(&stream->lock);
+
+ return 0;
+}
+
+static void string_stream_free(struct kunit_resource *res)
+{
+ /* Nothing to do since everything is already a KUnit managed resource */
+}
+
+struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp)
+{
+ struct string_stream_alloc_context context = {
+ .test = test,
+ .gfp = gfp
+ };
+
+ return kunit_alloc_resource(test,
+ string_stream_init,
+ string_stream_free,
+ gfp,
+ &context);
+}
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
* [PATCH v11 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Brendan Higgins @ 2019-07-17 1:55 UTC (permalink / raw)
To: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, sboyd, shuah, tytso, yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190717015543.152251-1-brendanhiggins@google.com>
A lot of the expectation and assertion infrastructure prints out fairly
complicated test failure messages, so add a C++ style log library for
for logging test results called `struct kunit_stream`.
kunit_stream allows us to construct a message before we know whether we
want to print it out; this can be extremely handy if there is
information you might need for a failure message that is easiest to
collect in the steps leading up to the actual check.
Note that kunit_stream is distinct from string_stream: whereas
string_stream is really just a string builder, kunit_stream is
specifically used for constructing expectation/assertion (introduced
later in this series) messages to a user of KUnit.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
include/kunit/kunit-stream.h | 97 +++++++++++++++++++++++++++++
include/kunit/test.h | 3 +
kunit/Makefile | 3 +-
kunit/kunit-stream.c | 115 +++++++++++++++++++++++++++++++++++
kunit/test.c | 6 ++
5 files changed, 223 insertions(+), 1 deletion(-)
create mode 100644 include/kunit/kunit-stream.h
create mode 100644 kunit/kunit-stream.c
diff --git a/include/kunit/kunit-stream.h b/include/kunit/kunit-stream.h
new file mode 100644
index 0000000000000..ba0ff553b8301
--- /dev/null
+++ b/include/kunit/kunit-stream.h
@@ -0,0 +1,97 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * C++ stream style string formatter and printer used in KUnit for outputting
+ * KUnit messages.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#ifndef _KUNIT_KUNIT_STREAM_H
+#define _KUNIT_KUNIT_STREAM_H
+
+#include <linux/types.h>
+#include <kunit/string-stream.h>
+
+struct kunit;
+
+/**
+ * struct kunit_stream - a std::stream style string builder.
+ *
+ * A std::stream style string builder. Allows messages to be built up and
+ * printed all at once. Note that the intention is to only use
+ * &struct kunit_stream to communicate with a user of KUnit, most often to
+ * communicate something about an expectation or an assertion to the user. If
+ * you want a similar interface, but aren't sure if this is the right class for
+ * you to use, you probably want to use the related string_stream class, which
+ * is allowed for generic string construction in a similar manner. This class is
+ * really only for the KUnit library to communicate certain kinds of information
+ * to KUnit users and should not be used by anyone else.
+ *
+ * A note on &struct kunit_stream's usage: a kunit_stream will generally
+ * accompany *one* expectation or assertion. Multiple expectations/assertions
+ * may be validated concurrently at any given time, even within a single test
+ * case, so sharing a kunit_stream between expectations/assertions may result in
+ * unintended consequences.
+ */
+struct kunit_stream {
+ /* private: internal use only. */
+ struct kunit *test;
+ const char *level;
+ struct string_stream *internal_stream;
+};
+
+/**
+ * alloc_kunit_stream() - constructs a new &struct kunit_stream.
+ * @test: The test context object.
+ * @level: The log level at which to print out the message.
+ * @gfp: The GFP flags to use for internal allocations.
+ *
+ * Constructs a new test managed &struct kunit_stream.
+ */
+struct kunit_stream *alloc_kunit_stream(struct kunit *test,
+ const char *level,
+ gfp_t gfp);
+
+/**
+ * kunit_stream_add(): adds the formatted input to the internal buffer.
+ * @kstream: the stream being operated on.
+ * @fmt: printf style format string to append to stream.
+ *
+ * Appends the formatted string, @fmt, to the internal buffer.
+ */
+void __printf(2, 3) kunit_stream_add(struct kunit_stream *kstream,
+ const char *fmt, ...);
+
+/**
+ * kunit_stream_append(): appends the contents of @other to @kstream.
+ * @kstream: the stream to which @other is appended.
+ * @other: the stream whose contents are appended to @kstream.
+ *
+ * Appends the contents of @other to @kstream.
+ */
+void kunit_stream_append(struct kunit_stream *kstream,
+ struct kunit_stream *other);
+
+/**
+ * kunit_stream_commit(): prints out the internal buffer to the user.
+ * @kstream: the stream being operated on.
+ *
+ * Outputs the contents of the internal buffer as a kunit_printk formatted
+ * output. KUNIT_STREAM ONLY OUTPUTS ITS BUFFER TO THE USER IF COMMIT IS
+ * CALLED!!! The reason for this is that it allows us to construct a message
+ * before we know whether we want to print it out; this can be extremely handy
+ * if there is information you might need for a failure message that is easiest
+ * to collect in the steps leading up to the actual check.
+ */
+void kunit_stream_commit(struct kunit_stream *kstream);
+
+/**
+ * kunit_stream_clear(): clears the internal buffer.
+ * @kstream: the stream being operated on.
+ *
+ * Clears the contents of the internal buffer.
+ */
+void kunit_stream_clear(struct kunit_stream *kstream);
+
+#endif /* _KUNIT_KUNIT_STREAM_H */
diff --git a/include/kunit/test.h b/include/kunit/test.h
index d0bf112910caf..4b6e7d91afd34 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -11,6 +11,7 @@
#include <linux/types.h>
#include <linux/slab.h>
+#include <kunit/kunit-stream.h>
struct kunit_resource;
@@ -184,6 +185,8 @@ struct kunit {
void kunit_init_test(struct kunit *test, const char *name);
+void kunit_fail(struct kunit *test, struct kunit_stream *stream);
+
int kunit_run_tests(struct kunit_suite *suite);
/**
diff --git a/kunit/Makefile b/kunit/Makefile
index 275b565a0e81f..6ddc622ee6b1c 100644
--- a/kunit/Makefile
+++ b/kunit/Makefile
@@ -1,2 +1,3 @@
obj-$(CONFIG_KUNIT) += test.o \
- string-stream.o
+ string-stream.o \
+ kunit-stream.o
diff --git a/kunit/kunit-stream.c b/kunit/kunit-stream.c
new file mode 100644
index 0000000000000..42fcef8188b3a
--- /dev/null
+++ b/kunit/kunit-stream.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * C++ stream style string formatter and printer used in KUnit for outputting
+ * KUnit messages.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#include <kunit/test.h>
+#include <kunit/kunit-stream.h>
+#include <kunit/string-stream.h>
+
+void kunit_stream_add(struct kunit_stream *kstream, const char *fmt, ...)
+{
+ va_list args;
+ struct string_stream *stream = kstream->internal_stream;
+
+ va_start(args, fmt);
+
+ if (string_stream_vadd(stream, fmt, args))
+ kunit_err(kstream->test,
+ "Failed to allocate fragment: %s\n",
+ fmt);
+
+ va_end(args);
+}
+
+void kunit_stream_append(struct kunit_stream *kstream,
+ struct kunit_stream *other)
+{
+ int ret;
+
+ ret = string_stream_append(kstream->internal_stream,
+ other->internal_stream);
+
+ if (ret)
+ kunit_err(kstream->test,
+ "Failed to append other stream: %d\n", ret);
+}
+
+void kunit_stream_clear(struct kunit_stream *kstream)
+{
+ string_stream_clear(kstream->internal_stream);
+}
+
+void kunit_stream_commit(struct kunit_stream *kstream)
+{
+ struct string_stream *stream = kstream->internal_stream;
+ struct string_stream_fragment *fragment;
+ struct kunit *test = kstream->test;
+ char *buf;
+
+ buf = string_stream_get_string(stream);
+ if (!buf) {
+ kunit_err(test,
+ "Could not allocate buffer, dumping stream:\n");
+ list_for_each_entry(fragment, &stream->fragments, node) {
+ kunit_err(test, fragment->fragment);
+ }
+ kunit_err(test, "\n");
+ } else {
+ kunit_printk(kstream->level, test, buf);
+ }
+
+ kunit_stream_clear(kstream);
+}
+
+struct kunit_stream_alloc_context {
+ struct kunit *test;
+ const char *level;
+ gfp_t gfp;
+};
+
+static int kunit_stream_init(struct kunit_resource *res, void *context)
+{
+ struct kunit_stream_alloc_context *ctx = context;
+ struct kunit_stream *stream;
+
+ stream = kunit_kzalloc(ctx->test, sizeof(*stream), ctx->gfp);
+ if (!stream)
+ return -ENOMEM;
+
+ res->allocation = stream;
+ stream->test = ctx->test;
+ stream->level = ctx->level;
+ stream->internal_stream = alloc_string_stream(ctx->test, ctx->gfp);
+
+ if (!stream->internal_stream)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static void kunit_stream_free(struct kunit_resource *res)
+{
+ /* Do nothing because cleanup is handled by KUnit managed resources */
+}
+
+struct kunit_stream *alloc_kunit_stream(struct kunit *test,
+ const char *level,
+ gfp_t gfp)
+{
+ struct kunit_stream_alloc_context ctx = {
+ .test = test,
+ .level = level,
+ .gfp = gfp
+ };
+
+ return kunit_alloc_resource(test,
+ kunit_stream_init,
+ kunit_stream_free,
+ gfp,
+ &ctx);
+}
diff --git a/kunit/test.c b/kunit/test.c
index 4c178a817f2fe..fdab07bb0b529 100644
--- a/kunit/test.c
+++ b/kunit/test.c
@@ -120,6 +120,12 @@ static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case,
test_case->name);
}
+void kunit_fail(struct kunit *test, struct kunit_stream *stream)
+{
+ kunit_set_failure(test);
+ kunit_stream_commit(stream);
+}
+
void kunit_init_test(struct kunit *test, const char *name)
{
spin_lock_init(&test->lock);
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
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