[v2] printk: Make linux/printk.h self-contained
diff mbox series

Message ID 20200612043634.GA30181@gondor.apana.org.au
State New, archived
Headers show
Series
  • [v2] printk: Make linux/printk.h self-contained
Related show

Commit Message

Herbert Xu June 12, 2020, 4:36 a.m. UTC
As it stands if you include printk.h by itself it will fail to
compile because it requires definitions from ratelimit.h.  However,
simply including ratelimit.h from printk.h does not work due to
inclusion loops involving sched.h and kernel.h.

This patch solves this by moving bits from ratelimit.h into a new
header file which can then be included by printk.h without any
worries about header loops.

The build bot then revealed some intriguing failures arising out
of this patch.  On s390 there is an inclusion loop with asm/bug.h
and linux/kernel.h that triggers a compile failure, because kernel.h
will cause asm-generic/bug.h to be included before s390's own
asm/bug.h has finished processing.  This has been fixed by not
including kernel.h in arch/s390/include/asm/bug.h.

A related failure was seen on powerpc where asm/bug.h leads to
the inclusion of linux/kernel.h via asm-generic/bug.h which then
prematurely tries to use the very macros defined in asm/bug.h.
The particular inclusion path which led to this involves lockdep.h.
I have fixed this moving the type definitions lockdep.h into the
new lockdep_types.h.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Comments

Andy Shevchenko June 12, 2020, 9:49 a.m. UTC | #1
On Fri, Jun 12, 2020 at 7:39 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> As it stands if you include printk.h by itself it will fail to
> compile because it requires definitions from ratelimit.h.  However,
> simply including ratelimit.h from printk.h does not work due to
> inclusion loops involving sched.h and kernel.h.
>
> This patch solves this by moving bits from ratelimit.h into a new
> header file which can then be included by printk.h without any
> worries about header loops.
>
> The build bot then revealed some intriguing failures arising out
> of this patch.  On s390 there is an inclusion loop with asm/bug.h
> and linux/kernel.h that triggers a compile failure, because kernel.h
> will cause asm-generic/bug.h to be included before s390's own
> asm/bug.h has finished processing.  This has been fixed by not
> including kernel.h in arch/s390/include/asm/bug.h.
>
> A related failure was seen on powerpc where asm/bug.h leads to
> the inclusion of linux/kernel.h via asm-generic/bug.h which then
> prematurely tries to use the very macros defined in asm/bug.h.
> The particular inclusion path which led to this involves lockdep.h.
> I have fixed this moving the type definitions lockdep.h into the
> new lockdep_types.h.

This change is a step to the right direction, thanks! FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

It seems you are opening a can of worms :-)
Have you a chance to look at [1]? I think it's a long road, but maybe
you are interested to help with?

[1]: https://lore.kernel.org/lkml/20200422125201.37618-1-andriy.shevchenko@linux.intel.com/


> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/include/linux/ratelimit_types.h b/include/linux/ratelimit_types.h
> new file mode 100644
> index 000000000000..b676aa419eef
> --- /dev/null
> +++ b/include/linux/ratelimit_types.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_RATELIMIT_TYPES_H
> +#define _LINUX_RATELIMIT_TYPES_H
> +
> +#include <linux/bits.h>
> +#include <linux/param.h>
> +#include <linux/spinlock_types.h>
> +
> +#define DEFAULT_RATELIMIT_INTERVAL     (5 * HZ)
> +#define DEFAULT_RATELIMIT_BURST                10
> +
> +/* issue num suppressed message on exit */
> +#define RATELIMIT_MSG_ON_RELEASE       BIT(0)
> +
> +struct ratelimit_state {
> +       raw_spinlock_t  lock;           /* protect the state */
> +
> +       int             interval;
> +       int             burst;
> +       int             printed;
> +       int             missed;
> +       unsigned long   begin;
> +       unsigned long   flags;
> +};
> +
> +#define RATELIMIT_STATE_INIT(name, interval_init, burst_init) {                \
> +               .lock           = __RAW_SPIN_LOCK_UNLOCKED(name.lock),  \
> +               .interval       = interval_init,                        \
> +               .burst          = burst_init,                           \
> +       }
> +
> +#define RATELIMIT_STATE_INIT_DISABLED                                  \
> +       RATELIMIT_STATE_INIT(ratelimit_state, 0, DEFAULT_RATELIMIT_BURST)
> +
> +#define DEFINE_RATELIMIT_STATE(name, interval_init, burst_init)                \
> +                                                                       \
> +       struct ratelimit_state name =                                   \
> +               RATELIMIT_STATE_INIT(name, interval_init, burst_init)   \
> +
> +extern int ___ratelimit(struct ratelimit_state *rs, const char *func);
> +#define __ratelimit(state) ___ratelimit(state, __func__)
> +
> +#endif /* _LINUX_RATELIMIT_TYPES_H */
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index e061635e0409..1cd862cfd2f4 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -7,6 +7,7 @@
>  #include <linux/kern_levels.h>
>  #include <linux/linkage.h>
>  #include <linux/cache.h>
> +#include <linux/ratelimit_types.h>
>
>  extern const char linux_banner[];
>  extern const char linux_proc_banner[];
> diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h
> index 8ddf79e9207a..b17e0cd0a30c 100644
> --- a/include/linux/ratelimit.h
> +++ b/include/linux/ratelimit.h
> @@ -2,41 +2,10 @@
>  #ifndef _LINUX_RATELIMIT_H
>  #define _LINUX_RATELIMIT_H
>
> -#include <linux/param.h>
> +#include <linux/ratelimit_types.h>
>  #include <linux/sched.h>
>  #include <linux/spinlock.h>
>
> -#define DEFAULT_RATELIMIT_INTERVAL     (5 * HZ)
> -#define DEFAULT_RATELIMIT_BURST                10
> -
> -/* issue num suppressed message on exit */
> -#define RATELIMIT_MSG_ON_RELEASE       BIT(0)
> -
> -struct ratelimit_state {
> -       raw_spinlock_t  lock;           /* protect the state */
> -
> -       int             interval;
> -       int             burst;
> -       int             printed;
> -       int             missed;
> -       unsigned long   begin;
> -       unsigned long   flags;
> -};
> -
> -#define RATELIMIT_STATE_INIT(name, interval_init, burst_init) {                \
> -               .lock           = __RAW_SPIN_LOCK_UNLOCKED(name.lock),  \
> -               .interval       = interval_init,                        \
> -               .burst          = burst_init,                           \
> -       }
> -
> -#define RATELIMIT_STATE_INIT_DISABLED                                  \
> -       RATELIMIT_STATE_INIT(ratelimit_state, 0, DEFAULT_RATELIMIT_BURST)
> -
> -#define DEFINE_RATELIMIT_STATE(name, interval_init, burst_init)                \
> -                                                                       \
> -       struct ratelimit_state name =                                   \
> -               RATELIMIT_STATE_INIT(name, interval_init, burst_init)   \
> -
>  static inline void ratelimit_state_init(struct ratelimit_state *rs,
>                                         int interval, int burst)
>  {
> @@ -73,9 +42,6 @@ ratelimit_set_flags(struct ratelimit_state *rs, unsigned long flags)
>
>  extern struct ratelimit_state printk_ratelimit_state;
>
> -extern int ___ratelimit(struct ratelimit_state *rs, const char *func);
> -#define __ratelimit(state) ___ratelimit(state, __func__)
> -
>  #ifdef CONFIG_PRINTK
>
>  #define WARN_ON_RATELIMIT(condition, state)    ({              \
> diff --git a/arch/s390/include/asm/bug.h b/arch/s390/include/asm/bug.h
> index 7725f8006fdf..0b25f28351ed 100644
> --- a/arch/s390/include/asm/bug.h
> +++ b/arch/s390/include/asm/bug.h
> @@ -2,7 +2,7 @@
>  #ifndef _ASM_S390_BUG_H
>  #define _ASM_S390_BUG_H
>
> -#include <linux/kernel.h>
> +#include <linux/compiler.h>
>
>  #ifdef CONFIG_BUG
>
> diff --git a/include/linux/lockdep_types.h b/include/linux/lockdep_types.h
> new file mode 100644
> index 000000000000..7b9350624577
> --- /dev/null
> +++ b/include/linux/lockdep_types.h
> @@ -0,0 +1,196 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Runtime locking correctness validator
> + *
> + *  Copyright (C) 2006,2007 Red Hat, Inc., Ingo Molnar <mingo@redhat.com>
> + *  Copyright (C) 2007 Red Hat, Inc., Peter Zijlstra
> + *
> + * see Documentation/locking/lockdep-design.rst for more details.
> + */
> +#ifndef __LINUX_LOCKDEP_TYPES_H
> +#define __LINUX_LOCKDEP_TYPES_H
> +
> +#include <linux/types.h>
> +
> +#define MAX_LOCKDEP_SUBCLASSES         8UL
> +
> +enum lockdep_wait_type {
> +       LD_WAIT_INV = 0,        /* not checked, catch all */
> +
> +       LD_WAIT_FREE,           /* wait free, rcu etc.. */
> +       LD_WAIT_SPIN,           /* spin loops, raw_spinlock_t etc.. */
> +
> +#ifdef CONFIG_PROVE_RAW_LOCK_NESTING
> +       LD_WAIT_CONFIG,         /* CONFIG_PREEMPT_LOCK, spinlock_t etc.. */
> +#else
> +       LD_WAIT_CONFIG = LD_WAIT_SPIN,
> +#endif
> +       LD_WAIT_SLEEP,          /* sleeping locks, mutex_t etc.. */
> +
> +       LD_WAIT_MAX,            /* must be last */
> +};
> +
> +#ifdef CONFIG_LOCKDEP
> +
> +#include <linux/list.h>
> +
> +/*
> + * We'd rather not expose kernel/lockdep_states.h this wide, but we do need
> + * the total number of states... :-(
> + */
> +#define XXX_LOCK_USAGE_STATES          (1+2*4)
> +
> +/*
> + * NR_LOCKDEP_CACHING_CLASSES ... Number of classes
> + * cached in the instance of lockdep_map
> + *
> + * Currently main class (subclass == 0) and signle depth subclass
> + * are cached in lockdep_map. This optimization is mainly targeting
> + * on rq->lock. double_rq_lock() acquires this highly competitive with
> + * single depth.
> + */
> +#define NR_LOCKDEP_CACHING_CLASSES     2
> +
> +/*
> + * A lockdep key is associated with each lock object. For static locks we use
> + * the lock address itself as the key. Dynamically allocated lock objects can
> + * have a statically or dynamically allocated key. Dynamically allocated lock
> + * keys must be registered before being used and must be unregistered before
> + * the key memory is freed.
> + */
> +struct lockdep_subclass_key {
> +       char __one_byte;
> +} __attribute__ ((__packed__));
> +
> +/* hash_entry is used to keep track of dynamically allocated keys. */
> +struct lock_class_key {
> +       union {
> +               struct hlist_node               hash_entry;
> +               struct lockdep_subclass_key     subkeys[MAX_LOCKDEP_SUBCLASSES];
> +       };
> +};
> +
> +extern struct lock_class_key __lockdep_no_validate__;
> +
> +struct lock_trace;
> +
> +#define LOCKSTAT_POINTS                4
> +
> +/*
> + * The lock-class itself. The order of the structure members matters.
> + * reinit_class() zeroes the key member and all subsequent members.
> + */
> +struct lock_class {
> +       /*
> +        * class-hash:
> +        */
> +       struct hlist_node               hash_entry;
> +
> +       /*
> +        * Entry in all_lock_classes when in use. Entry in free_lock_classes
> +        * when not in use. Instances that are being freed are on one of the
> +        * zapped_classes lists.
> +        */
> +       struct list_head                lock_entry;
> +
> +       /*
> +        * These fields represent a directed graph of lock dependencies,
> +        * to every node we attach a list of "forward" and a list of
> +        * "backward" graph nodes.
> +        */
> +       struct list_head                locks_after, locks_before;
> +
> +       const struct lockdep_subclass_key *key;
> +       unsigned int                    subclass;
> +       unsigned int                    dep_gen_id;
> +
> +       /*
> +        * IRQ/softirq usage tracking bits:
> +        */
> +       unsigned long                   usage_mask;
> +       const struct lock_trace         *usage_traces[XXX_LOCK_USAGE_STATES];
> +
> +       /*
> +        * Generation counter, when doing certain classes of graph walking,
> +        * to ensure that we check one node only once:
> +        */
> +       int                             name_version;
> +       const char                      *name;
> +
> +       short                           wait_type_inner;
> +       short                           wait_type_outer;
> +
> +#ifdef CONFIG_LOCK_STAT
> +       unsigned long                   contention_point[LOCKSTAT_POINTS];
> +       unsigned long                   contending_point[LOCKSTAT_POINTS];
> +#endif
> +} __no_randomize_layout;
> +
> +#ifdef CONFIG_LOCK_STAT
> +struct lock_time {
> +       s64                             min;
> +       s64                             max;
> +       s64                             total;
> +       unsigned long                   nr;
> +};
> +
> +enum bounce_type {
> +       bounce_acquired_write,
> +       bounce_acquired_read,
> +       bounce_contended_write,
> +       bounce_contended_read,
> +       nr_bounce_types,
> +
> +       bounce_acquired = bounce_acquired_write,
> +       bounce_contended = bounce_contended_write,
> +};
> +
> +struct lock_class_stats {
> +       unsigned long                   contention_point[LOCKSTAT_POINTS];
> +       unsigned long                   contending_point[LOCKSTAT_POINTS];
> +       struct lock_time                read_waittime;
> +       struct lock_time                write_waittime;
> +       struct lock_time                read_holdtime;
> +       struct lock_time                write_holdtime;
> +       unsigned long                   bounces[nr_bounce_types];
> +};
> +
> +struct lock_class_stats lock_stats(struct lock_class *class);
> +void clear_lock_stats(struct lock_class *class);
> +#endif
> +
> +/*
> + * Map the lock object (the lock instance) to the lock-class object.
> + * This is embedded into specific lock instances:
> + */
> +struct lockdep_map {
> +       struct lock_class_key           *key;
> +       struct lock_class               *class_cache[NR_LOCKDEP_CACHING_CLASSES];
> +       const char                      *name;
> +       short                           wait_type_outer; /* can be taken in this context */
> +       short                           wait_type_inner; /* presents this context */
> +#ifdef CONFIG_LOCK_STAT
> +       int                             cpu;
> +       unsigned long                   ip;
> +#endif
> +};
> +
> +struct pin_cookie { unsigned int val; };
> +
> +#else /* !CONFIG_LOCKDEP */
> +
> +/*
> + * The class key takes no space if lockdep is disabled:
> + */
> +struct lock_class_key { };
> +
> +/*
> + * The lockdep_map takes no space if lockdep is disabled:
> + */
> +struct lockdep_map { };
> +
> +struct pin_cookie { };
> +
> +#endif /* !LOCKDEP */
> +
> +#endif /* __LINUX_LOCKDEP_TYPES_H */
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 206774ac6946..1655d767c2c7 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -10,181 +10,20 @@
>  #ifndef __LINUX_LOCKDEP_H
>  #define __LINUX_LOCKDEP_H
>
> +#include <linux/lockdep_types.h>
> +
>  struct task_struct;
> -struct lockdep_map;
>
>  /* for sysctl */
>  extern int prove_locking;
>  extern int lock_stat;
>
> -#define MAX_LOCKDEP_SUBCLASSES         8UL
> -
> -#include <linux/types.h>
> -
> -enum lockdep_wait_type {
> -       LD_WAIT_INV = 0,        /* not checked, catch all */
> -
> -       LD_WAIT_FREE,           /* wait free, rcu etc.. */
> -       LD_WAIT_SPIN,           /* spin loops, raw_spinlock_t etc.. */
> -
> -#ifdef CONFIG_PROVE_RAW_LOCK_NESTING
> -       LD_WAIT_CONFIG,         /* CONFIG_PREEMPT_LOCK, spinlock_t etc.. */
> -#else
> -       LD_WAIT_CONFIG = LD_WAIT_SPIN,
> -#endif
> -       LD_WAIT_SLEEP,          /* sleeping locks, mutex_t etc.. */
> -
> -       LD_WAIT_MAX,            /* must be last */
> -};
> -
>  #ifdef CONFIG_LOCKDEP
>
>  #include <linux/linkage.h>
> -#include <linux/list.h>
>  #include <linux/debug_locks.h>
>  #include <linux/stacktrace.h>
>
> -/*
> - * We'd rather not expose kernel/lockdep_states.h this wide, but we do need
> - * the total number of states... :-(
> - */
> -#define XXX_LOCK_USAGE_STATES          (1+2*4)
> -
> -/*
> - * NR_LOCKDEP_CACHING_CLASSES ... Number of classes
> - * cached in the instance of lockdep_map
> - *
> - * Currently main class (subclass == 0) and signle depth subclass
> - * are cached in lockdep_map. This optimization is mainly targeting
> - * on rq->lock. double_rq_lock() acquires this highly competitive with
> - * single depth.
> - */
> -#define NR_LOCKDEP_CACHING_CLASSES     2
> -
> -/*
> - * A lockdep key is associated with each lock object. For static locks we use
> - * the lock address itself as the key. Dynamically allocated lock objects can
> - * have a statically or dynamically allocated key. Dynamically allocated lock
> - * keys must be registered before being used and must be unregistered before
> - * the key memory is freed.
> - */
> -struct lockdep_subclass_key {
> -       char __one_byte;
> -} __attribute__ ((__packed__));
> -
> -/* hash_entry is used to keep track of dynamically allocated keys. */
> -struct lock_class_key {
> -       union {
> -               struct hlist_node               hash_entry;
> -               struct lockdep_subclass_key     subkeys[MAX_LOCKDEP_SUBCLASSES];
> -       };
> -};
> -
> -extern struct lock_class_key __lockdep_no_validate__;
> -
> -struct lock_trace;
> -
> -#define LOCKSTAT_POINTS                4
> -
> -/*
> - * The lock-class itself. The order of the structure members matters.
> - * reinit_class() zeroes the key member and all subsequent members.
> - */
> -struct lock_class {
> -       /*
> -        * class-hash:
> -        */
> -       struct hlist_node               hash_entry;
> -
> -       /*
> -        * Entry in all_lock_classes when in use. Entry in free_lock_classes
> -        * when not in use. Instances that are being freed are on one of the
> -        * zapped_classes lists.
> -        */
> -       struct list_head                lock_entry;
> -
> -       /*
> -        * These fields represent a directed graph of lock dependencies,
> -        * to every node we attach a list of "forward" and a list of
> -        * "backward" graph nodes.
> -        */
> -       struct list_head                locks_after, locks_before;
> -
> -       const struct lockdep_subclass_key *key;
> -       unsigned int                    subclass;
> -       unsigned int                    dep_gen_id;
> -
> -       /*
> -        * IRQ/softirq usage tracking bits:
> -        */
> -       unsigned long                   usage_mask;
> -       const struct lock_trace         *usage_traces[XXX_LOCK_USAGE_STATES];
> -
> -       /*
> -        * Generation counter, when doing certain classes of graph walking,
> -        * to ensure that we check one node only once:
> -        */
> -       int                             name_version;
> -       const char                      *name;
> -
> -       short                           wait_type_inner;
> -       short                           wait_type_outer;
> -
> -#ifdef CONFIG_LOCK_STAT
> -       unsigned long                   contention_point[LOCKSTAT_POINTS];
> -       unsigned long                   contending_point[LOCKSTAT_POINTS];
> -#endif
> -} __no_randomize_layout;
> -
> -#ifdef CONFIG_LOCK_STAT
> -struct lock_time {
> -       s64                             min;
> -       s64                             max;
> -       s64                             total;
> -       unsigned long                   nr;
> -};
> -
> -enum bounce_type {
> -       bounce_acquired_write,
> -       bounce_acquired_read,
> -       bounce_contended_write,
> -       bounce_contended_read,
> -       nr_bounce_types,
> -
> -       bounce_acquired = bounce_acquired_write,
> -       bounce_contended = bounce_contended_write,
> -};
> -
> -struct lock_class_stats {
> -       unsigned long                   contention_point[LOCKSTAT_POINTS];
> -       unsigned long                   contending_point[LOCKSTAT_POINTS];
> -       struct lock_time                read_waittime;
> -       struct lock_time                write_waittime;
> -       struct lock_time                read_holdtime;
> -       struct lock_time                write_holdtime;
> -       unsigned long                   bounces[nr_bounce_types];
> -};
> -
> -struct lock_class_stats lock_stats(struct lock_class *class);
> -void clear_lock_stats(struct lock_class *class);
> -#endif
> -
> -/*
> - * Map the lock object (the lock instance) to the lock-class object.
> - * This is embedded into specific lock instances:
> - */
> -struct lockdep_map {
> -       struct lock_class_key           *key;
> -       struct lock_class               *class_cache[NR_LOCKDEP_CACHING_CLASSES];
> -       const char                      *name;
> -       short                           wait_type_outer; /* can be taken in this context */
> -       short                           wait_type_inner; /* presents this context */
> -#ifdef CONFIG_LOCK_STAT
> -       int                             cpu;
> -       unsigned long                   ip;
> -#endif
> -};
> -
>  static inline void lockdep_copy_map(struct lockdep_map *to,
>                                     struct lockdep_map *from)
>  {
> @@ -421,8 +260,6 @@ static inline void lock_set_subclass(struct lockdep_map *lock,
>
>  extern void lock_downgrade(struct lockdep_map *lock, unsigned long ip);
>
> -struct pin_cookie { unsigned int val; };
> -
>  #define NIL_COOKIE (struct pin_cookie){ .val = 0U, }
>
>  extern struct pin_cookie lock_pin_lock(struct lockdep_map *lock);
> @@ -501,10 +338,6 @@ static inline void lockdep_set_selftest_task(struct task_struct *task)
>  # define lockdep_reset()               do { debug_locks = 1; } while (0)
>  # define lockdep_free_key_range(start, size)   do { } while (0)
>  # define lockdep_sys_exit()                    do { } while (0)
> -/*
> - * The class key takes no space if lockdep is disabled:
> - */
> -struct lock_class_key { };
>
>  static inline void lockdep_register_key(struct lock_class_key *key)
>  {
> @@ -514,11 +347,6 @@ static inline void lockdep_unregister_key(struct lock_class_key *key)
>  {
>  }
>
> -/*
> - * The lockdep_map takes no space if lockdep is disabled:
> - */
> -struct lockdep_map { };
> -
>  #define lockdep_depth(tsk)     (0)
>
>  #define lockdep_is_held_type(l, r)             (1)
> @@ -530,8 +358,6 @@ struct lockdep_map { };
>
>  #define lockdep_recursing(tsk)                 (0)
>
> -struct pin_cookie { };
> -
>  #define NIL_COOKIE (struct pin_cookie){ }
>
>  #define lockdep_pin_lock(l)                    ({ struct pin_cookie cookie = { }; cookie; })
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Sergey Senozhatsky June 12, 2020, 11:32 a.m. UTC | #2
On (20/06/12 14:36), Herbert Xu wrote:
> As it stands if you include printk.h by itself it will fail to
> compile because it requires definitions from ratelimit.h.  However,
> simply including ratelimit.h from printk.h does not work due to
> inclusion loops involving sched.h and kernel.h.
> 
> This patch solves this by moving bits from ratelimit.h into a new
> header file which can then be included by printk.h without any
> worries about header loops.
> 
> The build bot then revealed some intriguing failures arising out
> of this patch.  On s390 there is an inclusion loop with asm/bug.h
> and linux/kernel.h that triggers a compile failure, because kernel.h
> will cause asm-generic/bug.h to be included before s390's own
> asm/bug.h has finished processing.  This has been fixed by not
> including kernel.h in arch/s390/include/asm/bug.h.
> 
> A related failure was seen on powerpc where asm/bug.h leads to
> the inclusion of linux/kernel.h via asm-generic/bug.h which then
> prematurely tries to use the very macros defined in asm/bug.h.
> The particular inclusion path which led to this involves lockdep.h.
> I have fixed this moving the type definitions lockdep.h into the
> new lockdep_types.h.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

FWIW,
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss
Petr Mladek June 12, 2020, 1:14 p.m. UTC | #3
On Fri 2020-06-12 14:36:35, Herbert Xu wrote:
> As it stands if you include printk.h by itself it will fail to
> compile because it requires definitions from ratelimit.h.  However,
> simply including ratelimit.h from printk.h does not work due to
> inclusion loops involving sched.h and kernel.h.
> 
> This patch solves this by moving bits from ratelimit.h into a new
> header file which can then be included by printk.h without any
> worries about header loops.
> 
> The build bot then revealed some intriguing failures arising out
> of this patch.  On s390 there is an inclusion loop with asm/bug.h
> and linux/kernel.h that triggers a compile failure, because kernel.h
> will cause asm-generic/bug.h to be included before s390's own
> asm/bug.h has finished processing.  This has been fixed by not
> including kernel.h in arch/s390/include/asm/bug.h.
> 
> A related failure was seen on powerpc where asm/bug.h leads to
> the inclusion of linux/kernel.h via asm-generic/bug.h which then
> prematurely tries to use the very macros defined in asm/bug.h.
> The particular inclusion path which led to this involves lockdep.h.
> I have fixed this moving the type definitions lockdep.h into the
> new lockdep_types.h.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

I am fine with the changes as long as the kernel test robot
does not complain ;-)

Acked-by: Petr Mladek <pmladek@suse.com>

Well, I wonder if PeterZ is fine with the lockdep part. It might make
sense to split it into separate patch as a prerequisite.

Best Regards,
Petr
Peter Zijlstra June 12, 2020, 4:52 p.m. UTC | #4
On Fri, Jun 12, 2020 at 03:14:05PM +0200, Petr Mladek wrote:
> On Fri 2020-06-12 14:36:35, Herbert Xu wrote:
> > As it stands if you include printk.h by itself it will fail to
> > compile because it requires definitions from ratelimit.h.  However,
> > simply including ratelimit.h from printk.h does not work due to
> > inclusion loops involving sched.h and kernel.h.
> > 
> > This patch solves this by moving bits from ratelimit.h into a new
> > header file which can then be included by printk.h without any
> > worries about header loops.
> > 
> > The build bot then revealed some intriguing failures arising out
> > of this patch.  On s390 there is an inclusion loop with asm/bug.h
> > and linux/kernel.h that triggers a compile failure, because kernel.h
> > will cause asm-generic/bug.h to be included before s390's own
> > asm/bug.h has finished processing.  This has been fixed by not
> > including kernel.h in arch/s390/include/asm/bug.h.
> > 
> > A related failure was seen on powerpc where asm/bug.h leads to
> > the inclusion of linux/kernel.h via asm-generic/bug.h which then
> > prematurely tries to use the very macros defined in asm/bug.h.
> > The particular inclusion path which led to this involves lockdep.h.
> > I have fixed this moving the type definitions lockdep.h into the
> > new lockdep_types.h.
> > 
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> I am fine with the changes as long as the kernel test robot
> does not complain ;-)
> 
> Acked-by: Petr Mladek <pmladek@suse.com>
> 
> Well, I wonder if PeterZ is fine with the lockdep part. It might make
> sense to split it into separate patch as a prerequisite.

They look fine, but yes, I think it makes sense to split that out.

Patch
diff mbox series

diff --git a/include/linux/ratelimit_types.h b/include/linux/ratelimit_types.h
new file mode 100644
index 000000000000..b676aa419eef
--- /dev/null
+++ b/include/linux/ratelimit_types.h
@@ -0,0 +1,43 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_RATELIMIT_TYPES_H
+#define _LINUX_RATELIMIT_TYPES_H
+
+#include <linux/bits.h>
+#include <linux/param.h>
+#include <linux/spinlock_types.h>
+
+#define DEFAULT_RATELIMIT_INTERVAL	(5 * HZ)
+#define DEFAULT_RATELIMIT_BURST		10
+
+/* issue num suppressed message on exit */
+#define RATELIMIT_MSG_ON_RELEASE	BIT(0)
+
+struct ratelimit_state {
+	raw_spinlock_t	lock;		/* protect the state */
+
+	int		interval;
+	int		burst;
+	int		printed;
+	int		missed;
+	unsigned long	begin;
+	unsigned long	flags;
+};
+
+#define RATELIMIT_STATE_INIT(name, interval_init, burst_init) {		\
+		.lock		= __RAW_SPIN_LOCK_UNLOCKED(name.lock),	\
+		.interval	= interval_init,			\
+		.burst		= burst_init,				\
+	}
+
+#define RATELIMIT_STATE_INIT_DISABLED					\
+	RATELIMIT_STATE_INIT(ratelimit_state, 0, DEFAULT_RATELIMIT_BURST)
+
+#define DEFINE_RATELIMIT_STATE(name, interval_init, burst_init)		\
+									\
+	struct ratelimit_state name =					\
+		RATELIMIT_STATE_INIT(name, interval_init, burst_init)	\
+
+extern int ___ratelimit(struct ratelimit_state *rs, const char *func);
+#define __ratelimit(state) ___ratelimit(state, __func__)
+
+#endif /* _LINUX_RATELIMIT_TYPES_H */
diff --git a/include/linux/printk.h b/include/linux/printk.h
index e061635e0409..1cd862cfd2f4 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -7,6 +7,7 @@ 
 #include <linux/kern_levels.h>
 #include <linux/linkage.h>
 #include <linux/cache.h>
+#include <linux/ratelimit_types.h>
 
 extern const char linux_banner[];
 extern const char linux_proc_banner[];
diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h
index 8ddf79e9207a..b17e0cd0a30c 100644
--- a/include/linux/ratelimit.h
+++ b/include/linux/ratelimit.h
@@ -2,41 +2,10 @@ 
 #ifndef _LINUX_RATELIMIT_H
 #define _LINUX_RATELIMIT_H
 
-#include <linux/param.h>
+#include <linux/ratelimit_types.h>
 #include <linux/sched.h>
 #include <linux/spinlock.h>
 
-#define DEFAULT_RATELIMIT_INTERVAL	(5 * HZ)
-#define DEFAULT_RATELIMIT_BURST		10
-
-/* issue num suppressed message on exit */
-#define RATELIMIT_MSG_ON_RELEASE	BIT(0)
-
-struct ratelimit_state {
-	raw_spinlock_t	lock;		/* protect the state */
-
-	int		interval;
-	int		burst;
-	int		printed;
-	int		missed;
-	unsigned long	begin;
-	unsigned long	flags;
-};
-
-#define RATELIMIT_STATE_INIT(name, interval_init, burst_init) {		\
-		.lock		= __RAW_SPIN_LOCK_UNLOCKED(name.lock),	\
-		.interval	= interval_init,			\
-		.burst		= burst_init,				\
-	}
-
-#define RATELIMIT_STATE_INIT_DISABLED					\
-	RATELIMIT_STATE_INIT(ratelimit_state, 0, DEFAULT_RATELIMIT_BURST)
-
-#define DEFINE_RATELIMIT_STATE(name, interval_init, burst_init)		\
-									\
-	struct ratelimit_state name =					\
-		RATELIMIT_STATE_INIT(name, interval_init, burst_init)	\
-
 static inline void ratelimit_state_init(struct ratelimit_state *rs,
 					int interval, int burst)
 {
@@ -73,9 +42,6 @@  ratelimit_set_flags(struct ratelimit_state *rs, unsigned long flags)
 
 extern struct ratelimit_state printk_ratelimit_state;
 
-extern int ___ratelimit(struct ratelimit_state *rs, const char *func);
-#define __ratelimit(state) ___ratelimit(state, __func__)
-
 #ifdef CONFIG_PRINTK
 
 #define WARN_ON_RATELIMIT(condition, state)	({		\
diff --git a/arch/s390/include/asm/bug.h b/arch/s390/include/asm/bug.h
index 7725f8006fdf..0b25f28351ed 100644
--- a/arch/s390/include/asm/bug.h
+++ b/arch/s390/include/asm/bug.h
@@ -2,7 +2,7 @@ 
 #ifndef _ASM_S390_BUG_H
 #define _ASM_S390_BUG_H
 
-#include <linux/kernel.h>
+#include <linux/compiler.h>
 
 #ifdef CONFIG_BUG
 
diff --git a/include/linux/lockdep_types.h b/include/linux/lockdep_types.h
new file mode 100644
index 000000000000..7b9350624577
--- /dev/null
+++ b/include/linux/lockdep_types.h
@@ -0,0 +1,196 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Runtime locking correctness validator
+ *
+ *  Copyright (C) 2006,2007 Red Hat, Inc., Ingo Molnar <mingo@redhat.com>
+ *  Copyright (C) 2007 Red Hat, Inc., Peter Zijlstra
+ *
+ * see Documentation/locking/lockdep-design.rst for more details.
+ */
+#ifndef __LINUX_LOCKDEP_TYPES_H
+#define __LINUX_LOCKDEP_TYPES_H
+
+#include <linux/types.h>
+
+#define MAX_LOCKDEP_SUBCLASSES		8UL
+
+enum lockdep_wait_type {
+	LD_WAIT_INV = 0,	/* not checked, catch all */
+
+	LD_WAIT_FREE,		/* wait free, rcu etc.. */
+	LD_WAIT_SPIN,		/* spin loops, raw_spinlock_t etc.. */
+
+#ifdef CONFIG_PROVE_RAW_LOCK_NESTING
+	LD_WAIT_CONFIG,		/* CONFIG_PREEMPT_LOCK, spinlock_t etc.. */
+#else
+	LD_WAIT_CONFIG = LD_WAIT_SPIN,
+#endif
+	LD_WAIT_SLEEP,		/* sleeping locks, mutex_t etc.. */
+
+	LD_WAIT_MAX,		/* must be last */
+};
+
+#ifdef CONFIG_LOCKDEP
+
+#include <linux/list.h>
+
+/*
+ * We'd rather not expose kernel/lockdep_states.h this wide, but we do need
+ * the total number of states... :-(
+ */
+#define XXX_LOCK_USAGE_STATES		(1+2*4)
+
+/*
+ * NR_LOCKDEP_CACHING_CLASSES ... Number of classes
+ * cached in the instance of lockdep_map
+ *
+ * Currently main class (subclass == 0) and signle depth subclass
+ * are cached in lockdep_map. This optimization is mainly targeting
+ * on rq->lock. double_rq_lock() acquires this highly competitive with
+ * single depth.
+ */
+#define NR_LOCKDEP_CACHING_CLASSES	2
+
+/*
+ * A lockdep key is associated with each lock object. For static locks we use
+ * the lock address itself as the key. Dynamically allocated lock objects can
+ * have a statically or dynamically allocated key. Dynamically allocated lock
+ * keys must be registered before being used and must be unregistered before
+ * the key memory is freed.
+ */
+struct lockdep_subclass_key {
+	char __one_byte;
+} __attribute__ ((__packed__));
+
+/* hash_entry is used to keep track of dynamically allocated keys. */
+struct lock_class_key {
+	union {
+		struct hlist_node		hash_entry;
+		struct lockdep_subclass_key	subkeys[MAX_LOCKDEP_SUBCLASSES];
+	};
+};
+
+extern struct lock_class_key __lockdep_no_validate__;
+
+struct lock_trace;
+
+#define LOCKSTAT_POINTS		4
+
+/*
+ * The lock-class itself. The order of the structure members matters.
+ * reinit_class() zeroes the key member and all subsequent members.
+ */
+struct lock_class {
+	/*
+	 * class-hash:
+	 */
+	struct hlist_node		hash_entry;
+
+	/*
+	 * Entry in all_lock_classes when in use. Entry in free_lock_classes
+	 * when not in use. Instances that are being freed are on one of the
+	 * zapped_classes lists.
+	 */
+	struct list_head		lock_entry;
+
+	/*
+	 * These fields represent a directed graph of lock dependencies,
+	 * to every node we attach a list of "forward" and a list of
+	 * "backward" graph nodes.
+	 */
+	struct list_head		locks_after, locks_before;
+
+	const struct lockdep_subclass_key *key;
+	unsigned int			subclass;
+	unsigned int			dep_gen_id;
+
+	/*
+	 * IRQ/softirq usage tracking bits:
+	 */
+	unsigned long			usage_mask;
+	const struct lock_trace		*usage_traces[XXX_LOCK_USAGE_STATES];
+
+	/*
+	 * Generation counter, when doing certain classes of graph walking,
+	 * to ensure that we check one node only once:
+	 */
+	int				name_version;
+	const char			*name;
+
+	short				wait_type_inner;
+	short				wait_type_outer;
+
+#ifdef CONFIG_LOCK_STAT
+	unsigned long			contention_point[LOCKSTAT_POINTS];
+	unsigned long			contending_point[LOCKSTAT_POINTS];
+#endif
+} __no_randomize_layout;
+
+#ifdef CONFIG_LOCK_STAT
+struct lock_time {
+	s64				min;
+	s64				max;
+	s64				total;
+	unsigned long			nr;
+};
+
+enum bounce_type {
+	bounce_acquired_write,
+	bounce_acquired_read,
+	bounce_contended_write,
+	bounce_contended_read,
+	nr_bounce_types,
+
+	bounce_acquired = bounce_acquired_write,
+	bounce_contended = bounce_contended_write,
+};
+
+struct lock_class_stats {
+	unsigned long			contention_point[LOCKSTAT_POINTS];
+	unsigned long			contending_point[LOCKSTAT_POINTS];
+	struct lock_time		read_waittime;
+	struct lock_time		write_waittime;
+	struct lock_time		read_holdtime;
+	struct lock_time		write_holdtime;
+	unsigned long			bounces[nr_bounce_types];
+};
+
+struct lock_class_stats lock_stats(struct lock_class *class);
+void clear_lock_stats(struct lock_class *class);
+#endif
+
+/*
+ * Map the lock object (the lock instance) to the lock-class object.
+ * This is embedded into specific lock instances:
+ */
+struct lockdep_map {
+	struct lock_class_key		*key;
+	struct lock_class		*class_cache[NR_LOCKDEP_CACHING_CLASSES];
+	const char			*name;
+	short				wait_type_outer; /* can be taken in this context */
+	short				wait_type_inner; /* presents this context */
+#ifdef CONFIG_LOCK_STAT
+	int				cpu;
+	unsigned long			ip;
+#endif
+};
+
+struct pin_cookie { unsigned int val; };
+
+#else /* !CONFIG_LOCKDEP */
+
+/*
+ * The class key takes no space if lockdep is disabled:
+ */
+struct lock_class_key { };
+
+/*
+ * The lockdep_map takes no space if lockdep is disabled:
+ */
+struct lockdep_map { };
+
+struct pin_cookie { };
+
+#endif /* !LOCKDEP */
+
+#endif /* __LINUX_LOCKDEP_TYPES_H */
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 206774ac6946..1655d767c2c7 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -10,181 +10,20 @@ 
 #ifndef __LINUX_LOCKDEP_H
 #define __LINUX_LOCKDEP_H
 
+#include <linux/lockdep_types.h>
+
 struct task_struct;
-struct lockdep_map;
 
 /* for sysctl */
 extern int prove_locking;
 extern int lock_stat;
 
-#define MAX_LOCKDEP_SUBCLASSES		8UL
-
-#include <linux/types.h>
-
-enum lockdep_wait_type {
-	LD_WAIT_INV = 0,	/* not checked, catch all */
-
-	LD_WAIT_FREE,		/* wait free, rcu etc.. */
-	LD_WAIT_SPIN,		/* spin loops, raw_spinlock_t etc.. */
-
-#ifdef CONFIG_PROVE_RAW_LOCK_NESTING
-	LD_WAIT_CONFIG,		/* CONFIG_PREEMPT_LOCK, spinlock_t etc.. */
-#else
-	LD_WAIT_CONFIG = LD_WAIT_SPIN,
-#endif
-	LD_WAIT_SLEEP,		/* sleeping locks, mutex_t etc.. */
-
-	LD_WAIT_MAX,		/* must be last */
-};
-
 #ifdef CONFIG_LOCKDEP
 
 #include <linux/linkage.h>
-#include <linux/list.h>
 #include <linux/debug_locks.h>
 #include <linux/stacktrace.h>
 
-/*
- * We'd rather not expose kernel/lockdep_states.h this wide, but we do need
- * the total number of states... :-(
- */
-#define XXX_LOCK_USAGE_STATES		(1+2*4)
-
-/*
- * NR_LOCKDEP_CACHING_CLASSES ... Number of classes
- * cached in the instance of lockdep_map
- *
- * Currently main class (subclass == 0) and signle depth subclass
- * are cached in lockdep_map. This optimization is mainly targeting
- * on rq->lock. double_rq_lock() acquires this highly competitive with
- * single depth.
- */
-#define NR_LOCKDEP_CACHING_CLASSES	2
-
-/*
- * A lockdep key is associated with each lock object. For static locks we use
- * the lock address itself as the key. Dynamically allocated lock objects can
- * have a statically or dynamically allocated key. Dynamically allocated lock
- * keys must be registered before being used and must be unregistered before
- * the key memory is freed.
- */
-struct lockdep_subclass_key {
-	char __one_byte;
-} __attribute__ ((__packed__));
-
-/* hash_entry is used to keep track of dynamically allocated keys. */
-struct lock_class_key {
-	union {
-		struct hlist_node		hash_entry;
-		struct lockdep_subclass_key	subkeys[MAX_LOCKDEP_SUBCLASSES];
-	};
-};
-
-extern struct lock_class_key __lockdep_no_validate__;
-
-struct lock_trace;
-
-#define LOCKSTAT_POINTS		4
-
-/*
- * The lock-class itself. The order of the structure members matters.
- * reinit_class() zeroes the key member and all subsequent members.
- */
-struct lock_class {
-	/*
-	 * class-hash:
-	 */
-	struct hlist_node		hash_entry;
-
-	/*
-	 * Entry in all_lock_classes when in use. Entry in free_lock_classes
-	 * when not in use. Instances that are being freed are on one of the
-	 * zapped_classes lists.
-	 */
-	struct list_head		lock_entry;
-
-	/*
-	 * These fields represent a directed graph of lock dependencies,
-	 * to every node we attach a list of "forward" and a list of
-	 * "backward" graph nodes.
-	 */
-	struct list_head		locks_after, locks_before;
-
-	const struct lockdep_subclass_key *key;
-	unsigned int			subclass;
-	unsigned int			dep_gen_id;
-
-	/*
-	 * IRQ/softirq usage tracking bits:
-	 */
-	unsigned long			usage_mask;
-	const struct lock_trace		*usage_traces[XXX_LOCK_USAGE_STATES];
-
-	/*
-	 * Generation counter, when doing certain classes of graph walking,
-	 * to ensure that we check one node only once:
-	 */
-	int				name_version;
-	const char			*name;
-
-	short				wait_type_inner;
-	short				wait_type_outer;
-
-#ifdef CONFIG_LOCK_STAT
-	unsigned long			contention_point[LOCKSTAT_POINTS];
-	unsigned long			contending_point[LOCKSTAT_POINTS];
-#endif
-} __no_randomize_layout;
-
-#ifdef CONFIG_LOCK_STAT
-struct lock_time {
-	s64				min;
-	s64				max;
-	s64				total;
-	unsigned long			nr;
-};
-
-enum bounce_type {
-	bounce_acquired_write,
-	bounce_acquired_read,
-	bounce_contended_write,
-	bounce_contended_read,
-	nr_bounce_types,
-
-	bounce_acquired = bounce_acquired_write,
-	bounce_contended = bounce_contended_write,
-};
-
-struct lock_class_stats {
-	unsigned long			contention_point[LOCKSTAT_POINTS];
-	unsigned long			contending_point[LOCKSTAT_POINTS];
-	struct lock_time		read_waittime;
-	struct lock_time		write_waittime;
-	struct lock_time		read_holdtime;
-	struct lock_time		write_holdtime;
-	unsigned long			bounces[nr_bounce_types];
-};
-
-struct lock_class_stats lock_stats(struct lock_class *class);
-void clear_lock_stats(struct lock_class *class);
-#endif
-
-/*
- * Map the lock object (the lock instance) to the lock-class object.
- * This is embedded into specific lock instances:
- */
-struct lockdep_map {
-	struct lock_class_key		*key;
-	struct lock_class		*class_cache[NR_LOCKDEP_CACHING_CLASSES];
-	const char			*name;
-	short				wait_type_outer; /* can be taken in this context */
-	short				wait_type_inner; /* presents this context */
-#ifdef CONFIG_LOCK_STAT
-	int				cpu;
-	unsigned long			ip;
-#endif
-};
-
 static inline void lockdep_copy_map(struct lockdep_map *to,
 				    struct lockdep_map *from)
 {
@@ -421,8 +260,6 @@  static inline void lock_set_subclass(struct lockdep_map *lock,
 
 extern void lock_downgrade(struct lockdep_map *lock, unsigned long ip);
 
-struct pin_cookie { unsigned int val; };
-
 #define NIL_COOKIE (struct pin_cookie){ .val = 0U, }
 
 extern struct pin_cookie lock_pin_lock(struct lockdep_map *lock);
@@ -501,10 +338,6 @@  static inline void lockdep_set_selftest_task(struct task_struct *task)
 # define lockdep_reset()		do { debug_locks = 1; } while (0)
 # define lockdep_free_key_range(start, size)	do { } while (0)
 # define lockdep_sys_exit() 			do { } while (0)
-/*
- * The class key takes no space if lockdep is disabled:
- */
-struct lock_class_key { };
 
 static inline void lockdep_register_key(struct lock_class_key *key)
 {
@@ -514,11 +347,6 @@  static inline void lockdep_unregister_key(struct lock_class_key *key)
 {
 }
 
-/*
- * The lockdep_map takes no space if lockdep is disabled:
- */
-struct lockdep_map { };
-
 #define lockdep_depth(tsk)	(0)
 
 #define lockdep_is_held_type(l, r)		(1)
@@ -530,8 +358,6 @@  struct lockdep_map { };
 
 #define lockdep_recursing(tsk)			(0)
 
-struct pin_cookie { };
-
 #define NIL_COOKIE (struct pin_cookie){ }
 
 #define lockdep_pin_lock(l)			({ struct pin_cookie cookie = { }; cookie; })