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 X-Spam-Level: X-Spam-Status: No, score=-18.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E64F4C432BE for ; Tue, 31 Aug 2021 12:07:03 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 5013D60FED for ; Tue, 31 Aug 2021 12:07:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 5013D60FED Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id C738B8D0001; Tue, 31 Aug 2021 08:07:02 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C232A6B0074; Tue, 31 Aug 2021 08:07:02 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AEA8C8D0001; Tue, 31 Aug 2021 08:07:02 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0051.hostedemail.com [216.40.44.51]) by kanga.kvack.org (Postfix) with ESMTP id 9EF396B0073 for ; Tue, 31 Aug 2021 08:07:02 -0400 (EDT) Received: from smtpin16.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 2C4DC181AEF1F for ; Tue, 31 Aug 2021 12:07:02 +0000 (UTC) X-FDA: 78535249884.16.C4EB3B9 Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by imf16.hostedemail.com (Postfix) with ESMTP id AB65CF00008C for ; Tue, 31 Aug 2021 12:07:01 +0000 (UTC) Received: from imap1.suse-dmz.suse.de (imap1.suse-dmz.suse.de [192.168.254.73]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 17AD61FE79; Tue, 31 Aug 2021 12:07:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1630411620; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=B76cia+77ekkr69gJ9o/UvwsYHQ+OHYWjhSshJwhB6U=; b=LtYofda91clqaXsXXRUGIyqzZOUEDKTz8p/nLPl/raPBLPOWZfMM8QsNOuEjuMtssi5FzH E4ymFttScah10uTzXQ4LlvjbfahqgAp/9+dKTHLJuQJ4BFARMJFRmsZah36ZxN7FQwknxK 8or9k+pKocwi98T9FjALk9sJJ6s6nyc= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1630411620; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=B76cia+77ekkr69gJ9o/UvwsYHQ+OHYWjhSshJwhB6U=; b=ELHxz13DHttSe8KYEx6ZZnxwQTIZIQI9qBHcT9CpECFUsO0/fvSbWFI0FQEOBTcLC/FIG4 rMrEobnNCqnO5xBw== Received: from imap1.suse-dmz.suse.de (imap1.suse-dmz.suse.de [192.168.254.73]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap1.suse-dmz.suse.de (Postfix) with ESMTPS id E579413A92; Tue, 31 Aug 2021 12:06:59 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap1.suse-dmz.suse.de with ESMTPSA id 2KYlN2MbLmE+FgAAGKfGzw (envelope-from ); Tue, 31 Aug 2021 12:06:59 +0000 Message-ID: <2772cf56-4183-857f-d070-c54bceb5c8d9@suse.cz> Date: Tue, 31 Aug 2021 14:06:59 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.0.3 Content-Language: en-US To: Imran Khan , cl@linux.com, akpm@linux-foundation.org Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, David Rientjes , Pekka Enberg , Joonsoo Kim , Oliver Glitta References: <20210831062539.898293-1-imran.f.khan@oracle.com> <20210831062539.898293-3-imran.f.khan@oracle.com> From: Vlastimil Babka Subject: Re: [RFC PATCH 2/2] mm, slub: Use stackdepot to store user information for slub object. In-Reply-To: <20210831062539.898293-3-imran.f.khan@oracle.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=LtYofda9; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=ELHxz13D; dmarc=none; spf=pass (imf16.hostedemail.com: domain of vbabka@suse.cz designates 195.135.220.29 as permitted sender) smtp.mailfrom=vbabka@suse.cz X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: AB65CF00008C X-Stat-Signature: itkiwktkycowwdudw6gp8tki4on3zhb4 X-HE-Tag: 1630411621-729604 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 8/31/21 08:25, Imran Khan wrote: > SLAB_STORE_USER causes information about allocating and freeing context > of a slub object, to be stored in metadata area in a couple of struct > track objects. These objects store allocation and/or freeing stack trace > in an array. This may result in same stack trace getting stored in metadata > area of multiple objects. > STACKDEPOT can be used to store unique stack traces without any > duplication,so use STACKDEPOT to store allocation and/or freeing stack > traces as well. > This results in low memory footprint, as we are not storing multiple > copies of the same stack trace for an allocation or free. > > Signed-off-by: Imran Khan > --- > mm/slub.c | 87 ++++++++++++++++++++++++++++++------------------------- > 1 file changed, 48 insertions(+), 39 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index df1ac8aff86f..8e2a2b837106 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -264,8 +264,8 @@ static inline bool kmem_cache_has_cpu_partial(struct kmem_cache *s) > #define TRACK_ADDRS_COUNT 16 > struct track { > unsigned long addr; /* Called from address */ > -#ifdef CONFIG_STACKTRACE > - unsigned long addrs[TRACK_ADDRS_COUNT]; /* Called from address */ > +#ifdef CONFIG_STACKDEPOT > + depot_stack_handle_t stack; > #endif > int cpu; /* Was running on cpu */ > int pid; /* Pid context */ > @@ -668,6 +668,27 @@ static inline unsigned int get_info_end(struct kmem_cache *s) > return s->inuse; > } > > +#ifdef CONFIG_STACKDEPOT > +static depot_stack_handle_t slub_debug_save_stack(gfp_t flags) > +{ > + unsigned long entries[TRACK_ADDRS_COUNT]; > + unsigned int nr_entries; > + > + nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 4); > + nr_entries = filter_irq_stacks(entries, nr_entries); > + return stack_depot_save(entries, nr_entries, flags); > +} > + > +static void print_stack(depot_stack_handle_t stack) > +{ > + unsigned long *entries; > + unsigned int nr_entries; > + > + nr_entries = stack_depot_fetch(stack, &entries); > + stack_trace_print(entries, nr_entries, 0); > +} This function could become part of stackdepot itself? > +#endif > + > static struct track *get_track(struct kmem_cache *s, void *object, > enum track_item alloc) > { > @@ -679,21 +700,13 @@ static struct track *get_track(struct kmem_cache *s, void *object, > } > > static void set_track(struct kmem_cache *s, void *object, > - enum track_item alloc, unsigned long addr) > + enum track_item alloc, unsigned long addr, gfp_t flags) > { > struct track *p = get_track(s, object, alloc); > > if (addr) { > -#ifdef CONFIG_STACKTRACE > - unsigned int nr_entries; > - > - metadata_access_enable(); > - nr_entries = stack_trace_save(kasan_reset_tag(p->addrs), > - TRACK_ADDRS_COUNT, 3); > - metadata_access_disable(); > - > - if (nr_entries < TRACK_ADDRS_COUNT) > - p->addrs[nr_entries] = 0; > +#ifdef CONFIG_STACKDEPOT > + p->stack = slub_debug_save_stack(flags); > #endif > p->addr = addr; > p->cpu = smp_processor_id(); > @@ -709,10 +722,11 @@ static void init_tracking(struct kmem_cache *s, void *object) > if (!(s->flags & SLAB_STORE_USER)) > return; > > - set_track(s, object, TRACK_FREE, 0UL); > - set_track(s, object, TRACK_ALLOC, 0UL); > + set_track(s, object, TRACK_FREE, 0UL, 0U); > + set_track(s, object, TRACK_ALLOC, 0UL, 0U); > } > > + > static void print_track(const char *s, struct track *t, unsigned long pr_time) > { > if (!t->addr) > @@ -720,15 +734,11 @@ static void print_track(const char *s, struct track *t, unsigned long pr_time) > > pr_err("%s in %pS age=%lu cpu=%u pid=%d\n", > s, (void *)t->addr, pr_time - t->when, t->cpu, t->pid); > -#ifdef CONFIG_STACKTRACE > - { > - int i; > - for (i = 0; i < TRACK_ADDRS_COUNT; i++) > - if (t->addrs[i]) > - pr_err("\t%pS\n", (void *)t->addrs[i]); > - else > - break; > - } > +#ifdef CONFIG_STACKDEPOT > + if (t->stack) > + print_stack(t->stack); > + else > + pr_err("(stack is not available)\n"); > #endif > } > > @@ -1261,7 +1271,8 @@ static inline int alloc_consistency_checks(struct kmem_cache *s, > > static noinline int alloc_debug_processing(struct kmem_cache *s, > struct page *page, > - void *object, unsigned long addr) > + void *object, unsigned long addr, > + gfp_t flags) > { > if (s->flags & SLAB_CONSISTENCY_CHECKS) { > if (!alloc_consistency_checks(s, page, object)) > @@ -1270,7 +1281,7 @@ static noinline int alloc_debug_processing(struct kmem_cache *s, > > /* Success perform special debug activities for allocs */ > if (s->flags & SLAB_STORE_USER) > - set_track(s, object, TRACK_ALLOC, addr); > + set_track(s, object, TRACK_ALLOC, addr, flags); > trace(s, page, object, 1); > init_object(s, object, SLUB_RED_ACTIVE); > return 1; > @@ -1350,7 +1361,7 @@ static noinline int free_debug_processing( > } > > if (s->flags & SLAB_STORE_USER) > - set_track(s, object, TRACK_FREE, addr); > + set_track(s, object, TRACK_FREE, addr, GFP_NOWAIT); > trace(s, page, object, 0); > /* Freepointer not overwritten by init_object(), SLAB_POISON moved it */ > init_object(s, object, SLUB_RED_INACTIVE); > @@ -2987,7 +2998,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node, > check_new_page: > > if (kmem_cache_debug(s)) { > - if (!alloc_debug_processing(s, page, freelist, addr)) { > + if (!alloc_debug_processing(s, page, freelist, addr, gfpflags)) { > /* Slab failed checks. Next slab needed */ > goto new_slab; > } else { > @@ -4275,6 +4286,8 @@ void kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct page *page) > void *objp0; > struct kmem_cache *s = page->slab_cache; > struct track __maybe_unused *trackp; > + unsigned long __maybe_unused *entries; > + unsigned int __maybe_unused nr_entries; > > kpp->kp_ptr = object; > kpp->kp_page = page; > @@ -4297,19 +4310,15 @@ void kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct page *page) > objp = fixup_red_left(s, objp); > trackp = get_track(s, objp, TRACK_ALLOC); > kpp->kp_ret = (void *)trackp->addr; > -#ifdef CONFIG_STACKTRACE > - for (i = 0; i < KS_ADDRS_COUNT && i < TRACK_ADDRS_COUNT; i++) { > - kpp->kp_stack[i] = (void *)trackp->addrs[i]; > - if (!kpp->kp_stack[i]) > - break; > - } > +#ifdef CONFIG_STACKDEPOT > + nr_entries = stack_depot_fetch(trackp->stack, &entries); > + for (i = 0; i < nr_entries; i++) > + kpp->kp_stack[i] = (void *)entries[i]; Hmm, in case stack_depot_save() fails and returns a zero handle (e.g. due to enomem) this seems to rely on stack_depot_fetch() returning gracefully with zero nr_entries for a zero handle. But I don't see such guarantee? stack_depot_init() isn't creating such entry and stack_depot_save() doesn't have such check. So it will work accidentally, or return garbage? But it would be IMHO useful to add such guarantee to stackdepot one way or another. > trackp = get_track(s, objp, TRACK_FREE); > - for (i = 0; i < KS_ADDRS_COUNT && i < TRACK_ADDRS_COUNT; i++) { > - kpp->kp_free_stack[i] = (void *)trackp->addrs[i]; > - if (!kpp->kp_free_stack[i]) > - break; > - } > + nr_entries = stack_depot_fetch(trackp->stack, &entries); > + for (i = 0; i < nr_entries; i++) > + kpp->kp_free_stack[i] = (void *)entries[i]; > #endif > #endif > } >