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;
Message-ID: <c20294e1-b926-efa3-95fd-d30601d44a5d@redhat.com>
Date:   Tue, 14 Jun 2022 14:28:53 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH 3/3] mm/kmemleak: Prevent soft lockup in first object
 iteration loop of kmemleak_scan()
Content-Language: en-US
From:   Waiman Long <longman@redhat.com>
To:     Catalin Marinas <catalin.marinas@arm.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
References: <20220612183301.981616-1-longman@redhat.com>
 <20220612183301.981616-4-longman@redhat.com> <YqjCGWmM2cGG1OOF@arm.com>
 <YqjFHOMT+rDgz3EK@arm.com> <325768cd-19bd-71ae-83d6-1ca5e84f7462@redhat.com>
In-Reply-To: <325768cd-19bd-71ae-83d6-1ca5e84f7462@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 6/14/22 14:22, Waiman Long wrote:
> On 6/14/22 13:27, Catalin Marinas wrote:
>
>>>> raw_spin_unlock_irq(&object->lock);
>>>> +
>>>> +        /*
>>>> +         * With object pinned by a positive reference count, it
>>>> +         * won't go away and we can safely release the RCU read
>>>> +         * lock and do a cond_resched() to avoid soft lockup every
>>>> +         * 64k objects.
>>>> +         */
>>>> +        if (object_pinned && !(gray_list_cnt & 0xffff)) {
>>>> +            rcu_read_unlock();
>>>> +            cond_resched();
>>>> +            rcu_read_lock();
>>>> +        }
>>> I'm not sure this gains much. There should be very few gray objects
>>> initially (those passed to kmemleak_not_leak() for example). The
>>> majority should be white objects.
>>>
>>> If we drop the fine-grained object->lock, we could instead take
>>> kmemleak_lock outside the loop with a cond_resched_lock(&kmemleak_lock)
>>> within the loop. I think we can get away with not having an
>>> rcu_read_lock() at all for list traversal with the big lock outside the
>>> loop.
>> Actually this doesn't work is the current object in the iteration is
>> freed. Does list_for_each_rcu_safe() help?
>
> list_for_each_rcu_safe() helps if we are worrying about object being 
> freed. However, it won't help if object->next is freed instead.
>
> How about something like:
>
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 7dd64139a7c7..fd836e43cb16 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -1417,12 +1417,16 @@ static void kmemleak_scan(void)
>         struct zone *zone;
>         int __maybe_unused i;
>         int new_leaks = 0;
> +       int loop1_cnt = 0;
>
>         jiffies_last_scan = jiffies;
>
>         /* prepare the kmemleak_object's */
>         rcu_read_lock();
>         list_for_each_entry_rcu(object, &object_list, object_list) {
> +               bool obj_pinned = false;
> +
> +               loop1_cnt++;
>                 raw_spin_lock_irq(&object->lock);
>  #ifdef DEBUG
>                 /*
> @@ -1437,10 +1441,32 @@ static void kmemleak_scan(void)
>  #endif
>                 /* reset the reference count (whiten the object) */
>                 object->count = 0;
> -               if (color_gray(object) && get_object(object))
> +               if (color_gray(object) && get_object(object)) {
>                         list_add_tail(&object->gray_list, &gray_list);
> +                       obj_pinned = true;
> +               }
>
>                 raw_spin_unlock_irq(&object->lock);
> +
> +               /*
> +                * Do a cond_resched() to avoid soft lockup every 64k 
> objects.
> +                * Make sure a reference has been taken so that the 
> object
> +                * won't go away without RCU read lock.
> +                */
> +               if (loop1_cnt & 0xffff) {


Sorry, should be "(!(loop1_cnt & 0xffff))".

> + if (!obj_pinned && !get_object(object)) {
> +                               /* Try the next object instead */
> +                               loop1_cnt--;
> +                               continue;
> +                       }
> +
> +                       rcu_read_unlock();
> +                       cond_resched();
> +                       rcu_read_lock();
> +
> +                       if (!obj_pinned)
> +                               put_object(object);
> +               }
>         }
>         rcu_read_unlock();
>
Cheers,
Longman