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: <be54417d-3fe2-9af6-a1e9-85d516b7fd87@intel.com>
Date:   Fri, 18 Feb 2022 11:11:24 +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 3/9] lib/ref_tracker: __ref_tracker_dir_print improve
 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-4-andrzej.hajda@intel.com>
 <CANn89iJ3W3ioVUaBJikCpFdCa9o_APpqyb0FmK9AmYPtgOeC7w@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: <CANn89iJ3W3ioVUaBJikCpFdCa9o_APpqyb0FmK9AmYPtgOeC7w@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?bFNNWmxhZHZBZUx1djYvV2xTS0NUVHBXRmh3U3JWUGUwOHNrdW1uajFFem1s?=
 =?utf-8?B?WDYvcjlkZ1pNVS9vOVZrWjlCaCtyeW1BVXVEU1diNFpHeXc4TWlOY0xURlpH?=
 =?utf-8?B?cDUrdG94ZW1XMFZvSXF4dGVTb01BOUUzV1YvTXRhUitVWk1jZ1ZnR1M2Y2lr?=
 =?utf-8?B?WGc3NFo5NWd2RTN3Z2duV05yZ1lEdXY5TUJEY0FsMmV2aEpFUEUxbHFTMERP?=
 =?utf-8?B?NXlFcjMrVExyTTNSTDRSR2F0dHQybThwN0lxSkhySW1teXVLU2oyV09sa3Na?=
 =?utf-8?B?RjFHMUt4Z0ZacldPL3VnRU9JOUt6Q2hYK0Z5Y1dVUHBmbnUzVVhSc2JyTHJU?=
 =?utf-8?B?NnFwaTVZY0srVGUxUzlzb3RJZ0xDNFhoS2UyUDhlTWdwNndUdUJnclYxY25s?=
 =?utf-8?B?NHkzN0dVanNoblZXTU9vdjJjaFIwem41Mk9zVEU2M0o3ODR1dUNYRFNzNHp1?=
 =?utf-8?B?MkNReGw5VFl1bThteHVCaUdFS0dtbFNWa2V0cjVleWlUQ2FCVGN5K3k3eGRz?=
 =?utf-8?B?UXllUTZkaVV2NGJEQmVsZVhlcXViRVhiZDM5SDBLNVdGSDlOOEE3dXRnRnFs?=
 =?utf-8?B?SWl5Z21vMVlxRmZDbmJVczNmekRBRGNUZHdMUnAvYUJXTllQL2FScXFWU1VM?=
 =?utf-8?B?SkRrSGx4WmdDbHhwVVpIUE1ZRUNmalNRMWs2UnFRemRzZzNuSUpCOFcyZlJ4?=
 =?utf-8?B?VFR5U0JzOVZtcHEwamNBcndBeWJJZTVhWnpaaXpUSHJyRFZCSnhDTWF4NVJW?=
 =?utf-8?B?NTlFbU9TODU0V2liYlQ1Rk9SelArMk5qNFZMZmdQblgzdW1DTUlLcDV5Y1hJ?=
 =?utf-8?B?WGtuWVRjTXJiSzFoVERDcjA4cXlJTGF1ZnErVnB3ekdoam1FTE9sMHNjNGYr?=
 =?utf-8?B?Q0pVdDhUS20wSHcvTCtMeTVaS3JWd3NZZGdnczJ0OG9FKzQwQnpRNDFZdHV0?=
 =?utf-8?B?Y1Rlb1VyM3M3QmlJbG41K2ZBcUVzbUo3bjdKZGoxd3BjZS9NWWxwRTIvdnFE?=
 =?utf-8?B?aXp2SjVuQ1h1RjJOUCtxWnVzUndySkZsUnhtZ1E4RWRRSTgzWlhWM2kwNGEz?=
 =?utf-8?B?a2JyVlY2SzJ5VU05QXEvL3BtL3hKOFArbXhTQS82QStqUUxGdk45cTdLemh0?=
 =?utf-8?B?OHlQM0tLV3U4b04vdWZwdzd4RUxGdFh6OTV0MC95eEVQQWhVR0NPMVYvNjZ5?=
 =?utf-8?B?Z0hHZHNHV05kMlVmRFhtbjNJYTRDejlKUHNVQ2NOczNkRDQ2UVlhQlJTMWpL?=
 =?utf-8?B?OGdVMTdaWk1LeHk5MjJUa3BoTVZ0bzVLVmdOaTlFeWh5TzFNTHI3aThRblVX?=
 =?utf-8?B?N0swOUNpdGRWNjdoY3YxK01VaEdoZ1RCSHM4WEdNeXQ5YVJkTUI3YVVqV3Uz?=
 =?utf-8?B?Vkd1dGR0VFZhbGgwcG81eGdzS0FydFJUOGZRSytsdjh5Nm43K2MwL09OcCtq?=
 =?utf-8?B?anZyMzFPQVBEV1NndldzTnR6R2dSVjg1SHczR1gySjRYZFRzTlZCanRpdldH?=
 =?utf-8?B?aDU3dUdPRVY2UnBFQjczU1ZWR3NBNW96dnY2ZFVhTFcwc1ViMUxpWGsxMXlP?=
 =?utf-8?B?cW9MQlIwMWxrMmN1L2NKeUNxOGs5T1ZhaXJVS0J4dU5FT0dOdHVWTEUxbm43?=
 =?utf-8?B?U25YNGxudTdIaGc0ZDBTVUJoZ0cvV1FqWFlQdkRReFlTdWJQQzgrOFowN1M5?=
 =?utf-8?B?TWJiWFRDTWJxV1FQVmQwOWVZMitCMjFpbzlpNW5pZUErM0xTOFA2T3ZNc1Fw?=
 =?utf-8?B?VHhoY3A4bUF2emlDYWFRSzFYVEgwZEJGZVltUGNXRHkyaERrNWpEQTMyOC91?=
 =?utf-8?B?aytlWmJvdEV2RThpN2h0a1ZNSHI3UFdNWlg2M1Ewc01TK1RHVTY4aGNER28w?=
 =?utf-8?B?SG5ZRUpiOVQwM0tubUU0Ty9tZE5Td3ZCVk5sblNCdHBVMGp2ZkZ3cFhNRGJH?=
 =?utf-8?B?VkRPbER3eUlLVFdSS1k1QjduaEtweFI1dUh1R0F2Vll4SmZOTzlkU0VSOU1j?=
 =?utf-8?B?OW1aRmpOK0ZjZ29UYldJYXlLZUFlZnBOS0pKVm5NTFNsNTlHemp3aHZVdmFW?=
 =?utf-8?B?Vm9xS0xJVUgxT3p1OFRsaTJaZDBXSUJjcGVIVVRYSTdmZStpdHpobjdQcU1Y?=
 =?utf-8?B?dVRldTBRT1FqdE1KZlZmSTh2S0hHaVpCZTFzVDRHUThIWFZjbGdvbFFSWjg1?=
 =?utf-8?Q?EEb9grL2PZKWrA5KPwyLK4E=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 13475f63-08ef-463b-21d3-08d9f2c70875
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:11:30.6912
 (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: jKj7pHMEcIimwYVj2nZb4WRW3wpATTmcAgyyvSPaJAiDV7CEQ4/cHUqCMtiZKb27KBvZ/9zHxJ25GaWM813sQQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5346
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,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:38, Eric Dumazet wrote:
> On Thu, Feb 17, 2022 at 6:05 AM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
>> To improve readibility of ref_tracker printing following changes
>> have been performed:
>> - added display name for ref_tracker_dir,
>> - stack trace is printed indented, in the same printk call,
>> - total number of references is printed every time,
>> - print info about dropped references.
>>
>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
>> Reviewed-by: Chris Wilson <chris.p.wilson@intel.com>
>> ---
>>   include/linux/ref_tracker.h | 15 ++++++++++++---
>>   lib/ref_tracker.c           | 28 ++++++++++++++++++++++------
>>   2 files changed, 34 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h
>> index b9c968a716483..090230e5b485d 100644
>> --- a/include/linux/ref_tracker.h
>> +++ b/include/linux/ref_tracker.h
>> @@ -15,18 +15,26 @@ struct ref_tracker_dir {
>>          refcount_t              untracked;
>>          struct list_head        list; /* List of active trackers */
>>          struct list_head        quarantine; /* List of dead trackers */
>> +       char                    name[32];
>>   #endif
>>   };
>>
>>   #ifdef CONFIG_REF_TRACKER
>> -static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir,
>> -                                       unsigned int quarantine_count)
>> +
>> +// Temporary allow two and three arguments, until consumers are converted
>> +#define ref_tracker_dir_init(_d, _q, args...) _ref_tracker_dir_init(_d, _q, ##args, #_d)
>> +#define _ref_tracker_dir_init(_d, _q, _n, ...) __ref_tracker_dir_init(_d, _q, _n)
>> +
>> +static inline void __ref_tracker_dir_init(struct ref_tracker_dir *dir,
>> +                                       unsigned int quarantine_count,
>> +                                       const char *name)
>>   {
>>          INIT_LIST_HEAD(&dir->list);
>>          INIT_LIST_HEAD(&dir->quarantine);
>>          spin_lock_init(&dir->lock);
>>          dir->quarantine_avail = quarantine_count;
>>          refcount_set(&dir->untracked, 1);
>> +       strlcpy(dir->name, name, sizeof(dir->name));
>>          stack_depot_init();
>>   }
>>
>> @@ -47,7 +55,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir,
>>   #else /* CONFIG_REF_TRACKER */
>>
>>   static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir,
>> -                                       unsigned int quarantine_count)
>> +                                       unsigned int quarantine_count,
>> +                                       ...)
>>   {
>>   }
>>
>> diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
>> index 0e9c7d2828ccb..943cff08110e3 100644
>> --- a/lib/ref_tracker.c
>> +++ b/lib/ref_tracker.c
>> @@ -1,4 +1,7 @@
>>   // SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +#define pr_fmt(fmt) "ref_tracker: " fmt
>> +
>>   #include <linux/export.h>
>>   #include <linux/list_sort.h>
>>   #include <linux/ref_tracker.h>
>> @@ -7,6 +10,7 @@
>>   #include <linux/stackdepot.h>
>>
>>   #define REF_TRACKER_STACK_ENTRIES 16
>> +#define STACK_BUF_SIZE 1024
>
>>   struct ref_tracker {
>>          struct list_head        head;   /* anchor into dir->list or dir->quarantine */
>> @@ -26,31 +30,43 @@ static int ref_tracker_cmp(void *priv, const struct list_head *a, const struct l
>>   void __ref_tracker_dir_print(struct ref_tracker_dir *dir,
>>                             unsigned int display_limit)
>>   {
>> -       unsigned int i = 0, count = 0;
>> +       unsigned int i = 0, count = 0, total = 0;
>>          struct ref_tracker *tracker;
>>          depot_stack_handle_t stack;
>> +       char *sbuf;
>>
>>          lockdep_assert_held(&dir->lock);
>>
>>          if (list_empty(&dir->list))
>>                  return;
>>
>> +       sbuf = kmalloc(STACK_BUF_SIZE, GFP_NOWAIT);
>> +
>> +       list_for_each_entry(tracker, &dir->list, head)
>> +               ++total;
> Another iteration over a potential long list.
>
> You can count the @skipped number in the following iteration just fine.

Skipped is already in 'count', but you are right with double looping> I 
can count total in the same loop,
just forgot to merge loops during code evolution.
Will be fixed.

Regards
Andrzej

> int skipped = 0;
>
>> +
>>          list_sort(NULL, &dir->list, ref_tracker_cmp);
>>
>>          list_for_each_entry(tracker, &dir->list, head) {
>> -               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;
>> +               if (i++ >= display_limit)
>                              skipped++;
>> +                       continue;
>>
>> -               pr_err("leaked %d references.\n", count);
>> -               if (stack)
>> -                       stack_depot_print(stack);
>> +               if (sbuf && !stack_depot_snprint(stack, sbuf, STACK_BUF_SIZE, 4))
>> +                       sbuf[0] = 0;
>> +               pr_err("%s@%pK has %d/%d users at\n%s\n",
>> +                      dir->name, dir, count, total, sbuf);
>>                  count = 0;
>>          }
>> +       if (i > display_limit)
>> +               pr_err("%s@%pK skipped %d/%d reports with %d unique stacks.\n",
>> +                      dir->name, dir, count, total, i - display_limit);
>> +
>> +       kfree(sbuf);
>>   }
>>   EXPORT_SYMBOL(__ref_tracker_dir_print);
>>
>> --
>> 2.25.1
>>