From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9B007C433FE for ; Thu, 7 Oct 2021 11:02:01 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 3E5B461058 for ; Thu, 7 Oct 2021 11:02:01 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 3E5B461058 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 99F546B0071; Thu, 7 Oct 2021 07:02:00 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8B2B86B0073; Thu, 7 Oct 2021 07:02:00 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6B71C6B0074; Thu, 7 Oct 2021 07:02:00 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0022.hostedemail.com [216.40.44.22]) by kanga.kvack.org (Postfix) with ESMTP id 591936B0071 for ; Thu, 7 Oct 2021 07:02:00 -0400 (EDT) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 17D38183EE7A0 for ; Thu, 7 Oct 2021 11:02:00 +0000 (UTC) X-FDA: 78669351600.09.03BACB0 Received: from mail-wr1-f48.google.com (mail-wr1-f48.google.com [209.85.221.48]) by imf06.hostedemail.com (Postfix) with ESMTP id 810B9801A89B for ; Thu, 7 Oct 2021 11:01:59 +0000 (UTC) Received: by mail-wr1-f48.google.com with SMTP id r7so17766090wrc.10 for ; Thu, 07 Oct 2021 04:01:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=NJgOSqoCCgXqOknLz7AdQYpr89OwA0jzXUySVyR80Xs=; b=M7iUMKBa9OdQuivKHhVb7u/+Q7iX2KCFCBHWKvWjQ40ubCMP3Fqw/TEwd+sRZ8pZw5 C44lLGOhyw6JXYdO0nULsRQFxS7qPtDTX1C8Q0G8Q9HcH5Kli5EMS1wScn0mA899ja5h QewOHemIzJmWooZX9DaYRBVCNW5wxn0Ds1FM9CsvfyOPbyKF9xtjjN9QqqYketJ3WeHj P7LAJuiw0Y72L7p3Gl0K+elu1PHzC11Dz45zjpkh9XO9k1i3Wzzg7JbayVbvMCbGDQiq bnEoYoRy5+b30rOA+1/3aAjuYLH6mIy23leaFq2ckQCUoY+hc3Ke9GuU/Jfh14THLIk3 N0Sg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=NJgOSqoCCgXqOknLz7AdQYpr89OwA0jzXUySVyR80Xs=; b=3SwCBbril4DBF22UtJH0TyLX1eX6UgZC14x0CoRkokpT4qkWVWFxjIsIVSCEnaZuP+ B8G37MtkPIl3CciatdDA9y1HzTAroxrp7zWFx7eNqnsczTDBOsrTE9NIWLDLU8x+EMn1 l4GthSDN25qxEGQu8Pf3kPX6boIicvcpoihIY+dXUs7jhi4xTPi4Zu6zf3hokoI99Hgb y92F9bmQmiqq0W5tWbTtFAMGNPbjWfb40gmLVar1QZKzThPsztS+dbP6EY+i/6Cusw0B FtQZiS0RsXE2r+spCIuzd2x6+6A6D5ycP7YP8E/DuJ4/IDDxuDHn4C7q6iCp4yXztHaH 2UdA== X-Gm-Message-State: AOAM532AvqyHyKKRvFK54huomTL12FvgTDWvlgW4WuLZkohxcX9iucOM Ljq5y3dJenFZZ5DOJNUfEZTNpQ== X-Google-Smtp-Source: ABdhPJxNZ3GZnwKRx3cEfL0sobv2ozaQ3OzXMKIBQI9jkD1YsD2fOajFBkhLkC09EyiLBtdLx1KUIg== X-Received: by 2002:a7b:c5d8:: with SMTP id n24mr3990455wmk.51.1633604517859; Thu, 07 Oct 2021 04:01:57 -0700 (PDT) Received: from elver.google.com ([2a00:79e0:15:13:aedf:c006:6996:1a79]) by smtp.gmail.com with ESMTPSA id c185sm8441412wma.8.2021.10.07.04.01.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Oct 2021 04:01:56 -0700 (PDT) Date: Thu, 7 Oct 2021 13:01:51 +0200 From: Marco Elver To: Vlastimil Babka Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, kasan-dev@googlegroups.com, Vijayanand Jitta , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter , Andrey Ryabinin , Alexander Potapenko , Andrey Konovalov , Dmitry Vyukov , Geert Uytterhoeven , Oliver Glitta , Imran Khan Subject: Re: [PATCH] lib/stackdepot: allow optional init and stack_table allocation by kvmalloc() Message-ID: References: <20211007095815.3563-1-vbabka@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211007095815.3563-1-vbabka@suse.cz> User-Agent: Mutt/2.0.5 (2021-01-21) X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 810B9801A89B X-Stat-Signature: 9qkidrmqqk7jed8hgt75po9c87rpwi5y Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=M7iUMKBa; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf06.hostedemail.com: domain of elver@google.com designates 209.85.221.48 as permitted sender) smtp.mailfrom=elver@google.com X-HE-Tag: 1633604519-474631 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Oct 07, 2021 at 11:58AM +0200, Vlastimil Babka wrote: [...] > - Add a CONFIG_STACKDEPOT_ALWAYS_INIT flag to keep using the current > well-defined point of allocation as part of mem_init(). Make CONFIG_KASAN > select this flag. > - Other users have to call stack_depot_init() as part of their own init when > it's determined that stack depot will actually be used. This may depend on > both config and runtime conditions. Convert current users which are > page_owner and several in the DRM subsystem. Same will be done for SLUB > later. > - Because the init might now be called after the boot-time memblock allocation > has given all memory to the buddy allocator, change stack_depot_init() to > allocate stack_table with kvmalloc() when memblock is no longer available. > Also handle allocation failure by disabling stackdepot (could have > theoretically happened even with memblock allocation previously), and don't > unnecessarily align the memblock allocation to its own size anymore. ... > Hi, I'd appreciate review of the DRM parts - namely that I've got correctly > that stack_depot_init() is called from the proper init functions and iff > stack_depot_save() is going to be used later. Thanks! For ease of review between stackdepot and DRM changes, I thought it'd be nice to split into 2 patches, but not sure it'll work, because you're changing the semantics of the normal STACKDEPOT. One option would be to flip it around, and instead have STACKDEPOT_LAZY_INIT, but that seems counter-intuitive if the majority of STACKDEPOT users are LAZY_INIT users. On the other hand, the lazy initialization mode you're introducing requires an explicit stack_depot_init() call somewhere and isn't as straightforward as before. Not sure what is best. My intuition tells me STACKDEPOT_LAZY_INIT would be safer as it's a deliberate opt-in to the lazy initialization behaviour. Preferences? [...] > --- a/drivers/gpu/drm/drm_mm.c > +++ b/drivers/gpu/drm/drm_mm.c > @@ -980,6 +980,10 @@ void drm_mm_init(struct drm_mm *mm, u64 start, u64 size) > add_hole(&mm->head_node); > > mm->scan_active = 0; > + > +#ifdef CONFIG_DRM_DEBUG_MM > + stack_depot_init(); > +#endif DRM_DEBUG_MM implies STACKDEPOT. Not sure what is more readable to drm maintainers, but perhaps it'd be nicer to avoid the #ifdef here, and instead just keep the no-op version of stack_depot_init() in . I don't have a strong preference. > } > EXPORT_SYMBOL(drm_mm_init); > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 0d85f3c5c526..806c32ab410b 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -68,6 +68,9 @@ static noinline depot_stack_handle_t __save_depot_stack(void) > static void init_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm) > { > spin_lock_init(&rpm->debug.lock); > + > + if (rpm->available) > + stack_depot_init(); > } > > static noinline depot_stack_handle_t > diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h > index c34b55a6e554..60ba99a43745 100644 > --- a/include/linux/stackdepot.h > +++ b/include/linux/stackdepot.h > @@ -15,6 +15,16 @@ > > typedef u32 depot_stack_handle_t; > > +/* > + * Every user of stack depot has to call this during its own init when it's > + * decided that it will be calling stack_depot_save() later. > + * > + * The alternative is to select STACKDEPOT_ALWAYS_INIT to have stack depot > + * enabled as part of mm_init(), for subsystems where it's known at compile time > + * that stack depot will be used. > + */ > +int stack_depot_init(void); > + > depot_stack_handle_t __stack_depot_save(unsigned long *entries, > unsigned int nr_entries, > gfp_t gfp_flags, bool can_alloc); > @@ -30,13 +40,4 @@ int stack_depot_snprint(depot_stack_handle_t handle, char *buf, size_t size, > > void stack_depot_print(depot_stack_handle_t stack); > > -#ifdef CONFIG_STACKDEPOT > -int stack_depot_init(void); > -#else > -static inline int stack_depot_init(void) > -{ > - return 0; > -} > -#endif /* CONFIG_STACKDEPOT */ > - Could we avoid the IS_ENABLED() in init/main.c by adding a wrapper here: +#ifdef CONFIG_STACKDEPOT_ALWAYS_INIT +static inline int stack_depot_early_init(void) { return stack_depot_init(); } +#else +static inline int stack_depot_early_init(void) { return 0; } +#endif /* CONFIG_STACKDEPOT_ALWAYS_INIT */ > #endif > diff --git a/init/main.c b/init/main.c > index ee4d3e1b3eb9..b6a5833d98f5 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -844,7 +844,8 @@ static void __init mm_init(void) > init_mem_debugging_and_hardening(); > kfence_alloc_pool(); > report_meminit(); > - stack_depot_init(); > + if (IS_ENABLED(CONFIG_STACKDEPOT_ALWAYS_INIT)) > + stack_depot_init(); I'd push the decision of when to call this into via wrapper stack_depot_early_init(). > mem_init(); > mem_init_print_info(); > /* page_owner must be initialized after buddy is ready */ > diff --git a/lib/Kconfig b/lib/Kconfig > index 5e7165e6a346..df6bcf0a4cc3 100644 > --- a/lib/Kconfig > +++ b/lib/Kconfig > @@ -671,6 +671,9 @@ config STACKDEPOT > bool > select STACKTRACE > > +config STACKDEPOT_ALWAYS_INIT > + bool It looks like every users of STACKDEPOT_ALWAYS_INIT will also select STACKDEPOT, so we could just make this: +config STACKDEPOT_ALWAYS_INIT + bool + select STACKDEPOT And remove the redundant 'select STACKDEPOT' in Kconfig.kasan. > config STACK_HASH_ORDER > int "stack depot hash size (12 => 4KB, 20 => 1024KB)" > range 12 20 > diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan > index cdc842d090db..695deb603c66 100644 > --- a/lib/Kconfig.kasan > +++ b/lib/Kconfig.kasan > @@ -39,6 +39,7 @@ menuconfig KASAN > HAVE_ARCH_KASAN_HW_TAGS > depends on (SLUB && SYSFS) || (SLAB && !DEBUG_SLAB) > select STACKDEPOT > + select STACKDEPOT_ALWAYS_INIT [...] > > -int __init stack_depot_init(void) > +/* > + * __ref because of memblock_alloc(), which will not be actually called after > + * the __init code is gone The reason is that after __init code is gone, slab_is_available() will be true (might be worth adding to the comment). > + */ > +__ref int stack_depot_init(void) > { > - if (!stack_depot_disable) { > + mutex_lock(&stack_depot_init_mutex); > + if (!stack_depot_disable && stack_table == NULL) { > size_t size = (STACK_HASH_SIZE * sizeof(struct stack_record *)); Thanks, -- Marco