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: <199aebfb-f364-cd9b-5d2b-dbe42b779a41@intel.com>
Date:   Fri, 18 Feb 2022 11:54:54 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.6.1
Subject: Re: [PATCH 2/9] lib/ref_tracker: compact stacktraces before printing
Content-Language: en-US
To:     Eric Dumazet <edumazet@google.com>
CC:     LKML <linux-kernel@vger.kernel.org>,
        <intel-gfx@lists.freedesktop.org>,
        <dri-devel@lists.freedesktop.org>, netdev <netdev@vger.kernel.org>,
        "Jani Nikula" <jani.nikula@linux.intel.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        "Lucas De Marchi" <lucas.demarchi@intel.com>,
        Chris Wilson <chris.p.wilson@intel.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        "Jakub Kicinski" <kuba@kernel.org>
References: <20220217140441.1218045-1-andrzej.hajda@intel.com>
 <20220217140441.1218045-3-andrzej.hajda@intel.com>
 <CANn89iKgzztLA3y6V+vw3RiyoScC3K=1Z1_gajj8H56wGWDw6A@mail.gmail.com>
From:   Andrzej Hajda <andrzej.hajda@intel.com>
Organization: Intel Technology Poland sp. z o.o. - ul. Slowackiego 173, 80-298
 Gdansk - KRS 101882 - NIP 957-07-52-316
In-Reply-To: <CANn89iKgzztLA3y6V+vw3RiyoScC3K=1Z1_gajj8H56wGWDw6A@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SEFUY1FzWTlBdzUvbEVMaWJoRWUzSE1vaUkrNUEyaDZ5eCtHZnNHM00yd2hQ?=
 =?utf-8?B?QjB1TW84RFpjVzc4TFBOMW51dS9teGpXRmJzTzdFL3I1QklPeTdiaFAyaitW?=
 =?utf-8?B?RGJsanVXaC9QSWgrTThuL29UV3J4aGxsYVR5cUJucTh0MWdZcVdjYTNib0p3?=
 =?utf-8?B?ZjFNYXRMTkhUOVlvZENJYWJsditpS1RzN0oxM04vVXhkeGs3OFlsa2FvMU5S?=
 =?utf-8?B?NEsrV0dCK3BFNW01U3FuOC9vb0F5ekdQcUowak9PL3RGZVhJVmZiTTRNTzBN?=
 =?utf-8?B?UHFERnJEQWM4NTFHdFFYWnhNRGt3V3NYbDhzVTN2alArR3NJcmpuMTNjZkVr?=
 =?utf-8?B?dEFaU2FKcTdsY0w5eFFLVkFlU0FwckdSRjI4UkRCK2xpTENBcENoT0JZWU9i?=
 =?utf-8?B?aXc1TzBuUHkvNnpYbXVmVzhNWXU4aTAvdDh5d0dsMWtva2NwZGc4cXhUekpm?=
 =?utf-8?B?VUUrcHpULytZdGNjTjJZSUkxTDF2RlBUSjZhZGFJZkRnY1pUanl2RlVUNmE4?=
 =?utf-8?B?Mno3ZFdxWlBjd0xzVy9CWmdndEg0SUVTS2NqZjVwcy9LK3dxMGZ2Um1UYU11?=
 =?utf-8?B?UFdESWk0MTVOeTVVNHBuRjFKdlVRNVI4a3ZkdjczMTRlWmN4akViMDBxbE5D?=
 =?utf-8?B?WFhGeGVTS0dueEZBb21VanBJK2hoc2FjOXJKS1JEclpxc1FXMEtydVl6Njhx?=
 =?utf-8?B?NW8vd2UvSjZwUDNlU2llSGo0V0diMHAyTno0OUp1aUVtOXVDYVpwUUk0eCtW?=
 =?utf-8?B?MWRad3VFTTk1ZGhDRzFOVFpZMnlKcy9RYnhwd3d4TzVFRGczQlUwVmdJakYr?=
 =?utf-8?B?aDhqVW4wM1BEV044STRiU2tGaUZuakhaUm0zU0xQMkpwZjJUajIxQXZnZHg1?=
 =?utf-8?B?U0dWUjVtNFhNclFhWVhvcUQ3YXhQUnRJMzJsdkh3dlNQK3R3VTJoNVV2TkFs?=
 =?utf-8?B?SEUyQThrN2ZzZnVUSjNHaXQ5VU9sd2dsZDRUU1FCbkdjQTY3RG1pc2lSeVIy?=
 =?utf-8?B?K0RzOENmWjJSVlBsUVYvNHYxQnAraTRJTXk1NFplbWJGc1I3YlR1V1JLQlZ2?=
 =?utf-8?B?aXpKVVRTekNrdjZwMEIzanc5aG50Mk5QcWtCM2xyWTYwMUNrVkZLMXk3OFdu?=
 =?utf-8?B?UkVPVDVrZHFpTDYwVEtQa2RhK2tuWVFzQnJWK1ZyNU5MTDdOdDhabnNYU2JP?=
 =?utf-8?B?QlNQRVBsSUhyRUhJMXdFVU5VVGIzN3ZFSE5uTU80UUIwTHZVWXI2bTYzMjNG?=
 =?utf-8?B?RmZ4NStUditEOXlVN3BLY1QxTS93UzRTaVRidTI3WHQ4WjBlVmVia0lOcjcv?=
 =?utf-8?B?KzhMVFFobGF0Y3BUS01tdk5xWFRFdGYvdTZ5c3Y4a3Bib0k3ZC96dHJqSW12?=
 =?utf-8?B?Um1HaCtnOUxxTE0ra2VjMExiZ1hEdWNrNDBhYlV3Vy9sT0gxNkRaN2RMbWFh?=
 =?utf-8?B?WFJrZ00xc0xOZVloUEttQW1hMSsvbWxIcm1YYUdyUkwzcXpWQ2s3Z3BvRkFm?=
 =?utf-8?B?ejhRNHdZZmlXMVdMT0JsSmlxU1h6LzhXcUJYYTF1WWRTYk9lRCtHd2dGRDQ4?=
 =?utf-8?B?WVBVaGdNNGxTOG8reFJyZHdVeXY1RGRQYWMzcEUrWTlCVndmbVExR1dVRC9z?=
 =?utf-8?B?OXFWSWxwNUpFOVBWdGhaQnBXNFcvT2hldzRqOHFNOW1JdVVXUkRnanViZXpM?=
 =?utf-8?B?VFdnVzRzeFpwalNXdWllMmppOGRzSmFvVnVpY3hQYjRGY1Y2VDc4a0p0SEM2?=
 =?utf-8?B?MGI5NVI1TmwvRHJ6N2pCbVljQ09KcElYSk95bnF0NlkwT2wrVVpZZ0hBcEdK?=
 =?utf-8?B?TEpXRDU4U1lRbTVNTFpJcTMrMjZZZXRNK2pSSE5tUEJBYTR5UnB3TklkSTA4?=
 =?utf-8?B?YkZ0aWlWR2ZqMENkL1MrV1pjNW5BVDdMOVE3bk5CclFZVFNWd3BKczRwbHQv?=
 =?utf-8?B?NkU2TWZmS3B3S05EbitmZWYwV2ZYL0VLRWRlWmtoeVYzUDU0dkI1eGtyNWtM?=
 =?utf-8?B?OXdqQWZieEQ0Y0tXMDdVUUFwUjBtUDlKZ21wVXR4VS85TVYwV0JLbGN2aDlp?=
 =?utf-8?B?RVVxc1JOT0tEUGV4bkoxY2cvUVlvbCtsc0FWY0M1bW5WZm9SVUhKcjJkSElN?=
 =?utf-8?B?RlBRcGhCaFJBTnZWZVVub2FMSWUyTXdrVjlIZXpKS3dhZUxoYXp5VURXSmw2?=
 =?utf-8?Q?aEl5z8cP4peaa1IAN5wD8TI=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3c17da7c-10e2-4e00-9f3a-08d9f2cd1c4e
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3180.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Feb 2022 10:55:00.9267
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: jyQyktlloNc653YS1yi9jOBh7KOnnQJNj6SMDzI8HkBqekWegnJDS9O+OWO5myKhMkjfAmDGiZHGnfWbSsypDA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR11MB3521
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_HI,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 17.02.2022 16:23, Eric Dumazet wrote:
> On Thu, Feb 17, 2022 at 6:05 AM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
>> In cases references are taken alternately on multiple exec paths leak
>> report can grow substantially, sorting and grouping leaks by stack_handle
>> allows to compact it.
>>
>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
>> Reviewed-by: Chris Wilson <chris.p.wilson@intel.com>
>> ---
>>   lib/ref_tracker.c | 35 +++++++++++++++++++++++++++--------
>>   1 file changed, 27 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
>> index 1b0c6d645d64a..0e9c7d2828ccb 100644
>> --- a/lib/ref_tracker.c
>> +++ b/lib/ref_tracker.c
>> @@ -1,5 +1,6 @@
>>   // SPDX-License-Identifier: GPL-2.0-or-later
>>   #include <linux/export.h>
>> +#include <linux/list_sort.h>
>>   #include <linux/ref_tracker.h>
>>   #include <linux/slab.h>
>>   #include <linux/stacktrace.h>
>> @@ -14,23 +15,41 @@ struct ref_tracker {
>>          depot_stack_handle_t    free_stack_handle;
>>   };
>>
>> +static int ref_tracker_cmp(void *priv, const struct list_head *a, const struct list_head *b)
>> +{
>> +       const struct ref_tracker *ta = list_entry(a, const struct ref_tracker, head);
>> +       const struct ref_tracker *tb = list_entry(b, const struct ref_tracker, head);
>> +
>> +       return ta->alloc_stack_handle - tb->alloc_stack_handle;
>> +}
>> +
>>   void __ref_tracker_dir_print(struct ref_tracker_dir *dir,
>>                             unsigned int display_limit)
>>   {
>> +       unsigned int i = 0, count = 0;
>>          struct ref_tracker *tracker;
>> -       unsigned int i = 0;
>> +       depot_stack_handle_t stack;
>>
>>          lockdep_assert_held(&dir->lock);
>>
>> +       if (list_empty(&dir->list))
>> +               return;
>> +
>> +       list_sort(NULL, &dir->list, ref_tracker_cmp);
> What is going to be the cost of sorting a list with 1,000,000 items in it ?

Do we really have such cases?


>
> I just want to make sure we do not trade printing at most ~10 references
> (from netdev_wait_allrefs()) to a soft lockup :/ with no useful info
> if something went terribly wrong.
>
> I suggest that you do not sort a potential big list, and instead
> attempt to allocate an array of @display_limits 'struct stack_counts'
>
> I suspect @display_limits will always be kept to a reasonable value
> (less than 100 ?)

I though rather about 16 :)
In theory everything is possible, but do we have real case examples 
which could lead to 100 stack traces?
Maybe some frameworks used by multiple consumers (drivers) ???

>
> struct stack_counts {
>      depot_stack_handle_t stack_handle;
>      unsigned int count;
> }
>
> Then, iterating the list and update the array (that you can keep
> sorted by ->stack_handle)
>
> Then after iterating, print the (at_most) @display_limits handles
> found in the temp array.

OK, could be faster and less invasive.
Other solution would be keeping the array in dir and update in every 
tracker alloc/free, this way we avoid iteration over potentially big 
list, but it would cost memory and since printing is rather rare I am 
not sure if it is worth.

I will try your proposition.

Regards
Andrzej

>
>> +
>>          list_for_each_entry(tracker, &dir->list, head) {
>> -               if (i < display_limit) {
>> -                       pr_err("leaked reference.\n");
>> -                       if (tracker->alloc_stack_handle)
>> -                               stack_depot_print(tracker->alloc_stack_handle);
>> -                       i++;
>> -               } else {
>> +               if (i++ >= display_limit)
>>                          break;
>> -               }
>> +               if (!count++)
>> +                       stack = tracker->alloc_stack_handle;
>> +               if (stack == tracker->alloc_stack_handle &&
>> +                   !list_is_last(&tracker->head, &dir->list))
>> +                       continue;
>> +
>> +               pr_err("leaked %d references.\n", count);
>> +               if (stack)
>> +                       stack_depot_print(stack);
>> +               count = 0;
>>          }
>>   }
>>   EXPORT_SYMBOL(__ref_tracker_dir_print);
>> --
>> 2.25.1
>>