linux-kernel - Re: [PATCH v3] debugobjects: scale the static pool size

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e286be8a-7f0a-65ab-0275-15abcd33eb07@redhat.com>
Date:   Tue, 20 Nov 2018 18:38:46 -0500
From:   Waiman Long <longman@...hat.com>
To:     Qian Cai <cai@....us>, akpm@...ux-foundation.org,
        tglx@...utronix.de
Cc:     yang.shi@...ux.alibaba.com, arnd@...db.de,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] debugobjects: scale the static pool size

On 11/20/2018 06:28 PM, Qian Cai wrote:
> The current value of the early boot static pool size is not big enough
> for systems with large number of CPUs with timer or/and workqueue
> objects selected. As the results, systems have 60+ CPUs with both timer
> and workqueue objects enabled could trigger "ODEBUG: Out of memory.
> ODEBUG disabled". Hence, fixed it by computing it according to
> CONFIG_NR_CPUS and CONFIG_DEBUG_OBJECTS_* options.
>
> Signed-off-by: Qian Cai <cai@....us>
> ---
> Changes since v2:
> * Made the calculation easier to read suggested by longman@...hat.com.
>
> Changes since v1:
> * Removed the CONFIG_NR_CPUS check since it is always defined.
> * Introduced a minimal default pool size to start with. Otherwise, the pool
>   size would be too low for systems only with a small number of CPUs.
> * Adjusted the scale factors according to data.
>
>  lib/debugobjects.c | 82 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 80 insertions(+), 2 deletions(-)
>
> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> index 70935ed91125..02456a1e57cb 100644
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -23,9 +23,82 @@
>  #define ODEBUG_HASH_BITS	14
>  #define ODEBUG_HASH_SIZE	(1 << ODEBUG_HASH_BITS)
>  
> -#define ODEBUG_POOL_SIZE	1024
> +#define ODEBUG_DEFAULT_POOL	512
>  #define ODEBUG_POOL_MIN_LEVEL	256
>  
> +/*
> + * Some debug objects are allocated during the early boot. Enabling some
> + * options like timers or workqueue objects may increase the size required
> + * significantly with large number of CPUs. For example (as today, 20 Nov.
> + * 2018),
> + *
> + * No. CPUs x 2 (worker pool) objects:
> + *
> + * start_kernel
> + *   workqueue_init_early
> + *     init_worker_pool
> + *       init_timer_key
> + *         debug_object_init
> + *
> + * No. CPUs objects (CONFIG_HIGH_RES_TIMERS):
> + *
> + * sched_init
> + *   hrtick_rq_init
> + *     hrtimer_init
> + *
> + * CONFIG_DEBUG_OBJECTS_WORK:
> + * No. CPUs x 6 (workqueue) objects:
> + *
> + * workqueue_init_early
> + *   alloc_workqueue
> + *     __alloc_workqueue_key
> + *       alloc_and_link_pwqs
> + *         init_pwq
> + *
> + * Also, plus No. CPUs objects:
> + *
> + * perf_event_init
> + *    __init_srcu_struct
> + *      init_srcu_struct_fields
> + *        init_srcu_struct_nodes
> + *          __init_work
> + *
> + * Increase the number a bit more in case the implmentatins are changed in
> + * the future, as it is better to avoid OOM than spending a bit more kernel
> + * memory that will/can be freed.
> + *
> + * With all debug objects config options selected except the workqueue and
> + * the timers, kernel reports,
> + * 64-CPU:  ODEBUG: 4 of 4 active objects replaced
> + * 256-CPU: ODEBUG: 4 of 4 active objects replaced
> + *
> + * all the options except the workqueue:
> + * 64-CPU:  ODEBUG: 466 of 466 active objects replaced
> + * 256-CPU: ODEBUG: 1810 of 1810 active objects replaced
> + *
> + * all the options except the timers:
> + * 64-CPU:  ODEBUG: 652 of 652 active objects replaced
> + * 256-CPU: ODEBUG: 2572 of 2572 active objects replaced
> + *
> + * all the options:
> + * 64-CPU:  ODEBUG: 1114 of 1114 active objects replaced
> + * 256-CPU: ODEBUG: 4378 of 4378 active objects replaced
> + */
> +#ifdef CONFIG_DEBUG_OBJECTS_WORK
> +#define ODEBUG_WORK_PCPU_CNT	10
> +#else
> +#define ODEBUG_WORK_PCPU_CNT	0
> +#endif /* CONFIG_DEBUG_OBJECTS_WORK */
> +
> +#ifdef CONFIG_DEBUG_OBJECTS_TIMERS
> +#define ODEBUG_TIMERS_PCPU_CNT	10
> +#else
> +#define ODEBUG_TIMERS_PCPU_CNT	0
> +#endif /* CONFIG_DEBUG_OBJECTS_TIMERS */
> +
> +#define ODEBUG_POOL_SIZE	(ODEBUG_DEFAULT_POOL + CONFIG_NR_CPUS * \
> +(ODEBUG_TIMERS_PCPU_CNT + ODEBUG_WORK_PCPU_CNT))

Just a nit about indentation. There should be some indentation in the
continued line.

> +
>  #define ODEBUG_CHUNK_SHIFT	PAGE_SHIFT
>  #define ODEBUG_CHUNK_SIZE	(1 << ODEBUG_CHUNK_SHIFT)
>  #define ODEBUG_CHUNK_MASK	(~(ODEBUG_CHUNK_SIZE - 1))
> @@ -58,8 +131,13 @@ static int			debug_objects_fixups __read_mostly;
>  static int			debug_objects_warnings __read_mostly;
>  static int			debug_objects_enabled __read_mostly
>  				= CONFIG_DEBUG_OBJECTS_ENABLE_DEFAULT;
> +/*
> + * This is only used after replaced static objects, so no need to scale it
> + * to use the early boot static pool size and it has already been scaled
> + * according to actual No. CPUs in the box within debug_objects_mem_init().
> + */
>  static int			debug_objects_pool_size __read_mostly
> -				= ODEBUG_POOL_SIZE;
> +				= ODEBUG_DEFAULT_POOL;
>  static int			debug_objects_pool_min_level __read_mostly
>  				= ODEBUG_POOL_MIN_LEVEL;
>  static struct debug_obj_descr	*descr_test  __read_mostly;

Other than that, the patch looks OK to me.

Cheers,
Longman