Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 14 Jun 2022 17:54:32 +0100
From:   Catalin Marinas <catalin.marinas@arm.com>
To:     Waiman Long <longman@redhat.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>, 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: <Yqi9SP2HZ4dlQOWG@arm.com>
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>
Precedence: bulk

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 <catalin.marinas@arm.com>