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: <672e0958-e438-fc78-c036-988cae596399@intel.com>
Date:   Mon, 25 Jul 2022 21:23:43 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v2 1/2] mm/slub: enable debugging memory wasting of
 kmalloc
Content-Language: en-US
To:     Kefeng Wang <wangkefeng.wang@huawei.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        "Christoph Lameter" <cl@linux.com>,
        Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        Hyeonggon Yoo <42.hyeyoo@gmail.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     "Hansen, Dave" <dave.hansen@intel.com>,
        Robin Murphy <robin.murphy@arm.com>,
        John Garry <john.garry@huawei.com>
References: <20220725112025.22625-1-feng.tang@intel.com>
 <f1ee3bd6-d068-b401-04ef-e14ca55529ac@huawei.com>
From:   Feng Tang <feng.tang@intel.com>
In-Reply-To: <f1ee3bd6-d068-b401-04ef-e14ca55529ac@huawei.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VHFkUU1SMjF3d1hxMllYcVdEcXkzR3pqKy9SOXdSelRVcDV1eGxzYXlWOFRz?=
 =?utf-8?B?VElhVG5ITG05ZENLTVZjdE5CYTY3UzRPZXJhMVpIcVdpQjFhRk84ZUxBRGVR?=
 =?utf-8?B?RGxOUTRPaW1BbzdPOVRvc2puZjFCWWF0cFVDWFRuei8xSURWYnAzbWtXeFVk?=
 =?utf-8?B?bVhKeU41YjFhbnE0akkwZ2Q5VnRGdWRvYWtsRkE0WGU1SWE1YzExZXIycnRJ?=
 =?utf-8?B?UXBvOTRKOW1vSC9ZeHFmRUoveVZQTUdaK1pZbnVHTVNYU1kyTTFWc3l2Y1d0?=
 =?utf-8?B?RWNjckZVeCsyV2Z1djVoV24wTHYvbUl5VG00dEV4eU0wOTIwRk0zZW9reFJI?=
 =?utf-8?B?MkhLOEVZWHFnalNFcllQQmlLS1NOVE9DbFRqQkZtc29uVDlmVUNMZU9TS2FU?=
 =?utf-8?B?NXNoQW5WU2ZtbDh1T1J3WlJhSHkySDh4VUdPb0NwNUIrcUtwRFk5NVFEKzVi?=
 =?utf-8?B?K2pvRzNoMkFSS3pZeXdheVFJYy9GSktmN3JzekNRUE9aMFJ3cWJOWmxidjY1?=
 =?utf-8?B?RFA3eURNM20rTXhXTUpDU3R0RkFHNFRXbGZjMUFDek9QcUZ0Q0gvMDlaODFa?=
 =?utf-8?B?R3BVYWsvYVlFMUV1NERzVC94VkxYYmtWVkxCaW9IckFldnovNXZST1Z4MEZD?=
 =?utf-8?B?R3dNQWQvTzBuaTJqV2I1cm9rdDU3VEd1ZkxNUDFkUXJuamlqSU1iNEFrZ1pS?=
 =?utf-8?B?allYdGo2bE52VXYzb205aEZ6SzA1T1hnSFdHRlY3K3dHRnI0bHpRZDBvRXYz?=
 =?utf-8?B?dmhqZXl3bjNyd21xTFp1azh2NzExQkhjY3RUYmgyeU1RczROeWVVbC8xbUx5?=
 =?utf-8?B?Y1NHNnBqYjJSVXFUTE9UdmVjcXBvS3VySCtCZkdkZmZJSHB2bjZ0T3E0aDlE?=
 =?utf-8?B?RlFhcWJudWlFTkVmVi9mY1dzL2xhZkxYMWc1VDU0R1ZyNmZUa0FoN1YzbWxp?=
 =?utf-8?B?a2FsOHpoL0NxWUhHbWtkMURHNEdFcm03bmpFdFBPc2YwM0wxK0dKK1Z3YjhN?=
 =?utf-8?B?dU40V012OVRqQXM4VkVkY3JjRmNxeG9XdC9WMHVDNkpDanFjRzRVNzhzaXVY?=
 =?utf-8?B?b1l2VnJmQ2JTTnh1UC9JZXlCQVVGQU9FS0NBd0NqQVd2dEdNMitiY0dwS1M4?=
 =?utf-8?B?amVWanJsS1QwQ0NRK0NVeVowQnJuYlpqY0tPRkFzTjA2Y1Y0VHQyQzlEaGds?=
 =?utf-8?B?MWhraGcxMGo1Zm1XblNhK0dvZDFuTzE1amxWUk9MZE9ablhaZE5xSUZlSDJw?=
 =?utf-8?B?QU96Tk9QMFBYdnBxY1BweTZaTWc4TjZpY3BhN0k1NjZCUDAveXBUdEVXRkwy?=
 =?utf-8?B?amZHaU9qSTM2ZHppc2xScnVFWTRQRmxKaDcwbzVkVTBwVDZWbEh4UXVqMDNX?=
 =?utf-8?B?MU5mR1lFL1ZjdWlmZ0xhcDkwSHptb0FpNDF6RUNCSStFRUVOUVJQN3ZpTXNI?=
 =?utf-8?B?UHJjMkJGa2E4cERTMlpraExKMDUvOWVZUWcwSUwvN1B1bWszakpMNUtrZys4?=
 =?utf-8?B?N1FLa3AvT2tobDZSaHFha0VINXk0YU5OcGo1eVVVWkhuK21ISmNLV1cxUHhR?=
 =?utf-8?B?NkY3bDlzeGpmWnhweDd4c3dGM3BOK1l0QXpNUnZ5QklLQkZKMEY1RjJaNGli?=
 =?utf-8?B?VU5uTWdLbmNxMENneUtMbGM2RElzWTloMlNoYTVQYjhVdjFrQkEvK05ya2ZM?=
 =?utf-8?B?WWJTeDdUb05OSVpiWVZSWjVDVGw3OEJQRXdpa0YrVWo2RkpLM1JrdGpmWENR?=
 =?utf-8?B?dm0wWGRDN3N4QUgrNnBOTkFYVlhHTW9IUmpqTks0aWhLNnFodHpZV3l3TGJ4?=
 =?utf-8?B?QVpJN3V6bCtwWUVmU2ZZZE1tdWQvNjZwdVIwQmIrZDdvWFJhZ0Zhb2t5M2xZ?=
 =?utf-8?B?MWkwNnRnYjFtZE9VdlNxMzdid0FLTVk1N3pvMnVFdWlkS0hwWEZ1Y2R4c3gr?=
 =?utf-8?B?RVhPM0NNM3hZOEs2TmRibkxwVXRMaXJvcHYvTk5mOW5Fc2Z3L21NdGpMc0RK?=
 =?utf-8?B?bmFONDRJTDJkZlo5ZHVsNnlXRmlzNTFKdmp1N0NZNEZSMHdPd3RBVDJUVDdK?=
 =?utf-8?B?ZisrZ2NZUDdveHJkeUUxMkI2YmdvZUM2bHRnT2hKN040VGFlUlE0RlV2Rits?=
 =?utf-8?Q?HiwC82dO/fF+0aQeom6biOs4O?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5637f274-3e88-4b50-7045-08da6e40eca5
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Jul 2022 13:23:55.7468
 (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: rxH6zzr0piAhkRcFSOXUwUbyWcaHDNy3eh2jGbO/SRW4bE0FaGnE0j9teI+3CC+lhawB2v3AmOfyS6mAwuWHXg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6067
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 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_MED,SPF_HELO_NONE,SPF_NONE 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

Hi Kefeng,

Thanks for the review.

On 2022/7/25 20:19, Kefeng Wang wrote:
> 
> On 2022/7/25 19:20, Feng Tang wrote:
>> kmalloc's API family is critical for mm, with one shortcoming that
>> its object size is fixed to be power of 2. When user requests memory
>> for '2^n + 1' bytes, actually 2^(n+1) bytes will be allocated, so
>> in worst case, there is around 50% memory space waste.
>>
>> We've met a kernel boot OOM panic (v5.10), and from the dumped slab info:
>>
>>       [   26.062145] kmalloc-2k            814056KB     814056KB
>>
>> >From debug we found there are huge number of 'struct iova_magazine',
>> whose size is 1032 bytes (1024 + 8), so each allocation will waste
>> 1016 bytes. Though the issue was solved by giving the right (bigger)
>> size of RAM, it is still nice to optimize the size (either use a
>> kmalloc friendly size or create a dedicated slab for it).
>>
>> And from lkml archive, there was another crash kernel OOM case [1]
>> back in 2019, which seems to be related with the similar slab waste
>> situation, as the log is similar:
>>
>>       [    4.332648] iommu: Adding device 0000:20:02.0 to group 16
>>       [    4.338946] swapper/0 invoked oom-killer: gfp_mask=0x6040c0(GFP_KERNEL|__GFP_COMP), nodemask=(null), order=0, oom_score_adj=0
>>       ...
>>       [    4.857565] kmalloc-2048           59164KB      59164KB
>>
>> The crash kernel only has 256M memory, and 59M is pretty big here.
>> (Note: the related code has been changed and optimised in recent
>> kernel [2], these logs are picked just to demo the problem)
>>
>> So add an way to track each kmalloc's memory waste info, and leverage
>> the existing SLUB debug framework to show its call stack info, so
>> that user can evaluate the waste situation, identify some hot spots
>> and optimize accordingly, for a better utilization of memory.
>>
>> The waste info is integrated into existing interface:
>> /sys/kernel/debug/slab/kmalloc-xx/alloc_traces, one example of
>> 'kmalloc-4k' after boot is:
>>
>> 126 ixgbe_alloc_q_vector+0xa5/0x4a0 [ixgbe] waste=233856/1856 age=1493302/1493830/1494358 pid=1284 cpus=32 nodes=1
>>           __slab_alloc.isra.86+0x52/0x80
>>           __kmalloc_node+0x143/0x350
>>           ixgbe_alloc_q_vector+0xa5/0x4a0 [ixgbe]
>>           ixgbe_init_interrupt_scheme+0x1a6/0x730 [ixgbe]
>>           ixgbe_probe+0xc8e/0x10d0 [ixgbe]
>>           local_pci_probe+0x42/0x80
>>           work_for_cpu_fn+0x13/0x20
>>           process_one_work+0x1c5/0x390
>>
>> which means in 'kmalloc-4k' slab, there are 126 requests of
>> 2240 bytes which got a 4KB space (wasting 1856 bytes each
>> and 233856 bytes in total). And when system starts some real
>> workload like multiple docker instances, there are more
>> severe waste.
>>
>> [1]. https://lkml.org/lkml/2019/8/12/266
>> [2]. https://lore.kernel.org/lkml/2920df89-9975-5785-f79b-257d3052dfaf@huawei.com/
>>
>> [Thanks Hyeonggon for pointing out several bugs about sorting/format]
>> [Thanks Vlastimil for suggesting way to reduce memory usage of
>>    orig_size and keep it only for kmalloc objects]
>>
>> Signed-off-by: Feng Tang <feng.tang@intel.com>
>> ---
>>     since v1:
>>       * limit the 'orig_size' to kmalloc objects only, and save
>>         it after track in metadata (Vlastimil Babka)
>>       * fix a offset calculation problem in print_trailer
>>
>>     since RFC:
>>       * fix problems in kmem_cache_alloc_bulk() and records sorting,
>>         improve the print format (Hyeonggon Yoo)
>>       * fix a compiling issue found by 0Day bot
>>       * update the commit log based info from iova developers
>>
>>
>>
>>    include/linux/slab.h |  2 +
>>    mm/slub.c            | 96 ++++++++++++++++++++++++++++++++++++--------
>>    2 files changed, 82 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/slab.h b/include/linux/slab.h
>> index 0fefdf528e0d..a713b0e5bbcd 100644
>> --- a/include/linux/slab.h
>> +++ b/include/linux/slab.h
>> @@ -29,6 +29,8 @@
>>    #define SLAB_RED_ZONE		((slab_flags_t __force)0x00000400U)
>>    /* DEBUG: Poison objects */
>>    #define SLAB_POISON		((slab_flags_t __force)0x00000800U)
>> +/* Indicate a kmalloc slab */
>> +#define SLAB_KMALLOC		((slab_flags_t __force)0x00001000U)
>>    /* Align objs on cache lines */
>>    #define SLAB_HWCACHE_ALIGN	((slab_flags_t __force)0x00002000U)
>>    /* Use GFP_DMA memory */
>> diff --git a/mm/slub.c b/mm/slub.c
>> index b1281b8654bd..9763a38bc4f0 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -191,6 +191,12 @@ static inline bool kmem_cache_debug(struct kmem_cache *s)
>>    	return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS);
>>    }
>>    
>> +static inline bool slub_debug_orig_size(struct kmem_cache *s)
>> +{
>> +	return (s->flags & SLAB_KMALLOC &&
>> +			kmem_cache_debug_flags(s, SLAB_STORE_USER));
> Swap two judgments to reduce the SLAB_KMALLOC check if no SLAB_STORE_USER.
> 

Ok, will change.

>> +}
>> +
>>    void *fixup_red_left(struct kmem_cache *s, void *p)
>>    {
>>    	if (kmem_cache_debug_flags(s, SLAB_RED_ZONE))
>> @@ -814,6 +820,36 @@ static void print_slab_info(const struct slab *slab)
>>    	pr_err("Slab 0x%p objects=%u used=%u fp=0x%p flags=%pGp\n",
>>    	       slab, slab->objects, slab->inuse, slab->freelist,
>>    	       folio_flags(folio, 0));
>> +
>> +}
>> +static inline void set_orig_size(struct kmem_cache *s,
>> +					void *object, unsigned int orig_size)
>> +{
>> +	void *p = kasan_reset_tag(object);
>> +
>> +	if (!slub_debug_orig_size(s))
>> +		return;
>> +
>> +	p = object + get_info_end(s);
> Look like p += get_info_end(s);  ?
>> +
>> +	if (s->flags & SLAB_STORE_USER)
>> +		p += sizeof(struct track) * 2;
>> +
>> +	*(unsigned int *)p = orig_size;
>> +}
>> +
>> +static unsigned int get_orig_size(struct kmem_cache *s, void *object)
>> +{
>> +	void *p = kasan_reset_tag(object);
>> +
>> +	if (!slub_debug_orig_size(s))
>> +		return s->object_size;
>> +
>> +	p = object + get_info_end(s);
> ditto...

Good catch! will change both of them, thanks!

Thanks,
Feng

>> +	if (s->flags & SLAB_STORE_USER)
>> +		p += sizeof(struct track) * 2;
>> +
>> +	return *(unsigned int *)p;
>>    }

[...]