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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id F3667CCA47A for ; Tue, 14 Jun 2022 16:54:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6D2506B0071; Tue, 14 Jun 2022 12:54:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 665A56B0072; Tue, 14 Jun 2022 12:54:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4FADD6B0073; Tue, 14 Jun 2022 12:54:41 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 3B7426B0071 for ; Tue, 14 Jun 2022 12:54:41 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 085DF60AF2 for ; Tue, 14 Jun 2022 16:54:41 +0000 (UTC) X-FDA: 79577440362.28.DFF0E63 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by imf13.hostedemail.com (Postfix) with ESMTP id 43E2320004 for ; Tue, 14 Jun 2022 16:54:39 +0000 (UTC) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 6D286B81A0C; Tue, 14 Jun 2022 16:54:38 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 33368C3411B; Tue, 14 Jun 2022 16:54:36 +0000 (UTC) Date: Tue, 14 Jun 2022 17:54:32 +0100 From: Catalin Marinas To: Waiman Long Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/3] mm/kmemleak: Skip unlikely objects in kmemleak_scan() without taking lock Message-ID: References: <20220612183301.981616-1-longman@redhat.com> <20220612183301.981616-3-longman@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220612183301.981616-3-longman@redhat.com> ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=none; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=arm.com (policy=none); spf=pass (imf13.hostedemail.com: domain of cmarinas@kernel.org designates 145.40.68.75 as permitted sender) smtp.mailfrom=cmarinas@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1655225680; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=dq2aQMO3XlutR+4ZsOsbxhwKFaHDqvuWz8b+L8nHZ/I=; b=vkZj76nxAVKtW8uWyu5ukgizwkWV8Fso4XN9tJs96L03bLzkJXGh+qjMb6o0HHU07Eo2Rc 1Ods7QzUx/owK3er0QRNtsT9vzEpUW6p9SsKaPvspzc1wQQlv17LHL3I0KkSF04jCXrqZv 6XlbDjslq6T2FIOEv0IJCPUu417EKFE= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1655225680; a=rsa-sha256; cv=none; b=W/CyEOrJbDLgxId89C5CXx2JD6qO2Nn6KPc8I+xj0aCT9NoIzOeP1bTflN9lPNwKO0+Nfm ueoslwqdWL3aYTko749+n9kJk/2odadWQFK+nkxbN70beDZeCr8qWbc4lJcwbRANU/u+Ub 6PbZNmuUNmw37wK2TZ/3WqP91QHi0t8= X-Stat-Signature: 3m49gmf77pzqrjjnakaog9cj6r6i7kgs X-Rspamd-Queue-Id: 43E2320004 X-Rspam-User: Authentication-Results: imf13.hostedemail.com; dkim=none; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=arm.com (policy=none); spf=pass (imf13.hostedemail.com: domain of cmarinas@kernel.org designates 145.40.68.75 as permitted sender) smtp.mailfrom=cmarinas@kernel.org X-Rspamd-Server: rspam10 X-HE-Tag: 1655225679-967881 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 Sun, Jun 12, 2022 at 02:33:00PM -0400, Waiman Long wrote: > With a debug kernel running on a 2-socket 96-thread x86-64 system > (HZ=1000), the 2nd and 3rd iteration loops speedup with this patch on > the first kmemleak_scan() call after bootup is shown in the table below. > > Before patch After patch > Loop # # of objects Elapsed time # of objects Elapsed time > ------ ------------ ------------ ------------ ------------ > 2 2,599,850 2.392s 2,596,364 0.266s > 3 2,600,176 2.171s 2,597,061 0.260s > > This patch reduces loop iteration times by about 88%. This will greatly > reduce the chance of a soft lockup happening in the 2nd or 3rd iteration > loops. Nice numbers, thanks for digging into this. But I'm slightly surprised that the first loop doesn't cause any problems. > diff --git a/mm/kmemleak.c b/mm/kmemleak.c > index dad9219c972c..7dd64139a7c7 100644 > --- a/mm/kmemleak.c > +++ b/mm/kmemleak.c > @@ -1508,6 +1508,13 @@ static void kmemleak_scan(void) > */ > rcu_read_lock(); > list_for_each_entry_rcu(object, &object_list, object_list) { > + /* > + * This is racy but we can save the overhead of lock/unlock > + * calls. The missed objects, if any, should be caught in > + * the next scan. > + */ > + if (!color_white(object)) > + continue; > raw_spin_lock_irq(&object->lock); > if (color_white(object) && (object->flags & OBJECT_ALLOCATED) > && update_checksum(object) && get_object(object)) { It's not actually scanning (like tree look-ups) but only updating the checksum of the potentially orphan objects. If the problem is caused by object->lock, we should have seen it with the first loop as well. It is possible that some large list is occasionally missed if there are concurrent updates and a significant number of objects turn up "white", forcing the checksum update. Otherwise this shouldn't be much different from the first loop if there are no massive (false) leaks. I think the race on color_white() can only be with a kmemleak_ignore() or kmemleak_not_leak() call, otherwise the object colour shouldn't be changed. So such objects can only turn from white to gray or black, so the race I think is safe. > @@ -1535,6 +1542,13 @@ static void kmemleak_scan(void) > */ > rcu_read_lock(); > list_for_each_entry_rcu(object, &object_list, object_list) { > + /* > + * This is racy but we can save the overhead of lock/unlock > + * calls. The missed objects, if any, should be caught in > + * the next scan. > + */ > + if (!color_white(object)) > + continue; > raw_spin_lock_irq(&object->lock); > if (unreferenced_object(object) && > !(object->flags & OBJECT_REPORTED)) { Same here. I did wonder whether it's worth keeping object->lock around, I even have a stashed patch lying around from 2019. Instead we'd have the big kmemleak_lock held for longer, though released periodically during scanning. We can then move the lock outside the loop and traversal would be faster but with an increased latency on slab allocation/freeing on other CPUs. Right now we take the kmemleak_lock when scanning a single block (e.g. object) to protect the rb-tree and rely on object->lock to ensure the object isn't freed. Other concurrent allocs/frees would only be blocked during single object scanning. Anyway, I'm not entirely sure it's the lock causing the issue as we don't see it with the first loop. I'm more inclined to think it's the checksum and the skipping if !color_white() would do the trick. Unless there's a better idea: Reviewed-by: Catalin Marinas