| Message ID | 1510023934-17517-1-git-send-email-miles.chen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 7 Nov 2017, miles.chen@mediatek.com wrote: > When slub_debug=O is set. It is possible to clear debug flags > for an "unmergeable" slab cache in kmem_cache_open(). > It makes the "unmergeable" cache became "mergeable" in sysfs_slab_add(). Right but that is only if disable_higher_order_debug is set. > These caches will generate their "unique IDs" by create_unique_id(), > but it is possible to create identical unique IDs. In my experiment, > sgpool-128, names_cache, biovec-256 generate the same ID ":Ft-0004096" > and the kernel reports "sysfs: cannot create duplicate filename > '/kernel/slab/:Ft-0004096'". Ok then the aliasing failed for some reason. The creation of the unique id and the alias detection needs to be in sync otherwise duplicate filenames are created. What is the difference there? The clearing of the DEBUG_METADATA_FLAGS looks ok to me. kmem_cache_alias should do the same right?
Hi Miles,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on mmotm/master]
[also build test ERROR on v4.14-rc8 next-20171107]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/miles-chen-mediatek-com/slub-Fix-sysfs-duplicate-filename-creation-when-slub_debug-O/20171108-100701
base: git://git.cmpxchg.org/linux-mmotm.git master
config: i386-randconfig-x001-201745 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
mm/slub.c: In function 'sysfs_slab_add':
>> mm/slub.c:5713:18: error: 'SLAB_NEVER_MERGE' undeclared (first use in this function)
(slub_debug & SLAB_NEVER_MERGE))
^~~~~~~~~~~~~~~~
mm/slub.c:5713:18: note: each undeclared identifier is reported only once for each function it appears in
vim +/SLAB_NEVER_MERGE +5713 mm/slub.c
5697
5698 static int sysfs_slab_add(struct kmem_cache *s)
5699 {
5700 int err;
5701 const char *name;
5702 struct kset *kset = cache_kset(s);
5703 int unmergeable = slab_unmergeable(s);
5704
5705 INIT_WORK(&s->kobj_remove_work, sysfs_slab_remove_workfn);
5706
5707 if (!kset) {
5708 kobject_init(&s->kobj, &slab_ktype);
5709 return 0;
5710 }
5711
5712 if (!unmergeable && disable_higher_order_debug &&
> 5713 (slub_debug & SLAB_NEVER_MERGE))
5714 unmergeable = 1;
5715
5716 if (unmergeable) {
5717 /*
5718 * Slabcache can never be merged so we can use the name proper.
5719 * This is typically the case for debug situations. In that
5720 * case we can catch duplicate names easily.
5721 */
5722 sysfs_remove_link(&slab_kset->kobj, s->name);
5723 name = s->name;
5724 } else {
5725 /*
5726 * Create a unique name for the slab as a target
5727 * for the symlinks.
5728 */
5729 name = create_unique_id(s);
5730 }
5731
5732 s->kobj.kset = kset;
5733 err = kobject_init_and_add(&s->kobj, &slab_ktype, NULL, "%s", name);
5734 if (err)
5735 goto out;
5736
5737 err = sysfs_create_group(&s->kobj, &slab_attr_group);
5738 if (err)
5739 goto out_del_kobj;
5740
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, 2017-11-07 at 09:22 -0600, Christopher Lameter wrote: > On Tue, 7 Nov 2017, miles.chen@mediatek.com wrote: > > > When slub_debug=O is set. It is possible to clear debug flags > > for an "unmergeable" slab cache in kmem_cache_open(). > > It makes the "unmergeable" cache became "mergeable" in sysfs_slab_add(). > > Right but that is only if disable_higher_order_debug is set. yes > > > These caches will generate their "unique IDs" by create_unique_id(), > > but it is possible to create identical unique IDs. In my experiment, > > sgpool-128, names_cache, biovec-256 generate the same ID ":Ft-0004096" > > and the kernel reports "sysfs: cannot create duplicate filename > > '/kernel/slab/:Ft-0004096'". > > Ok then the aliasing failed for some reason. The creation of the unique id > and the alias detection needs to be in sync otherwise duplicate filenames > are created. What is the difference there? The aliasing failed because find_mergeable() returns if (flags & SLAB_NEVER_MERGE) is true. So we do not go to search for alias caches. __kmem_cache_alias() find_mergeable() kmem_cache_flags() --> setup flag by the slub_debug if (flags & SLAB_NEVER_MERGE) return NULL; ... search alias logic... The flags maybe changed if disable_higher_order_debug=1. So the unmergeable cache becomes mergeable later. > > The clearing of the DEBUG_METADATA_FLAGS looks ok to me. kmem_cache_alias > should do the same right? > Yes, I think clearing DEBUG_METADATA flags in kmem_cache_alias is another solution for this issue. We will need to do calculate_sizes() by using original flags and compare the order of s->size and s->object_size when disable_higher_order_debug=1.
On Wed, 8 Nov 2017, Miles Chen wrote: > > Ok then the aliasing failed for some reason. The creation of the unique id > > and the alias detection needs to be in sync otherwise duplicate filenames > > are created. What is the difference there? > > The aliasing failed because find_mergeable() returns if (flags & > SLAB_NEVER_MERGE) is true. So we do not go to search for alias caches. > > __kmem_cache_alias() > find_mergeable() > kmem_cache_flags() --> setup flag by the slub_debug > if (flags & SLAB_NEVER_MERGE) return NULL; > ... > search alias logic... > > > The flags maybe changed if disable_higher_order_debug=1. So the > unmergeable cache becomes mergeable later. Ok so make sure taht the aliasing logic also clears those flags before checking for SLAB_NEVER_MERGE. > > The clearing of the DEBUG_METADATA_FLAGS looks ok to me. kmem_cache_alias > > should do the same right? > > > Yes, I think clearing DEBUG_METADATA flags in kmem_cache_alias is > another solution for this issue. > > We will need to do calculate_sizes() by using original flags and compare > the order of s->size and s->object_size when > disable_higher_order_debug=1. Hmmm... Or move the aliasing check to a point where we know the size of the slab objects?
On Wed, 2017-11-08 at 09:05 -0600, Christopher Lameter wrote: > On Wed, 8 Nov 2017, Miles Chen wrote: > > > > Ok then the aliasing failed for some reason. The creation of the unique id > > > and the alias detection needs to be in sync otherwise duplicate filenames > > > are created. What is the difference there? > > > > The aliasing failed because find_mergeable() returns if (flags & > > SLAB_NEVER_MERGE) is true. So we do not go to search for alias caches. > > > > __kmem_cache_alias() > > find_mergeable() > > kmem_cache_flags() --> setup flag by the slub_debug > > if (flags & SLAB_NEVER_MERGE) return NULL; > > ... > > search alias logic... > > > > > > The flags maybe changed if disable_higher_order_debug=1. So the > > unmergeable cache becomes mergeable later. > > Ok so make sure taht the aliasing logic also clears those flags before > checking for SLAB_NEVER_MERGE. > > > > The clearing of the DEBUG_METADATA_FLAGS looks ok to me. kmem_cache_alias > > > should do the same right? > > > > > Yes, I think clearing DEBUG_METADATA flags in kmem_cache_alias is > > another solution for this issue. > > > > We will need to do calculate_sizes() by using original flags and compare > > the order of s->size and s->object_size when > > disable_higher_order_debug=1. > > Hmmm... Or move the aliasing check to a point where we know the size of > the slab objects? The biggest concern is that we may have some merged caches even if we enable CONFIG_SLUB_DEBUG_ON and slub_debug=O. So a developer cannot say "I set CONFIG_SLUB_DEBUG_ON=y to stop all slab merging". (https://www.spinics.net/lists/linux-mm/msg77919.html) In this fix patch, it disables slab merging if SLUB_DEBUG=O and CONFIG_SLUB_DEBUG_ON=y but the debug features are disabled by the disable_higher_order_debug logic and it holds the "slab merging is off if any debug features are enabled" behavior.
On Thu, 9 Nov 2017, Miles Chen wrote: > In this fix patch, it disables slab merging if SLUB_DEBUG=O and > CONFIG_SLUB_DEBUG_ON=y but the debug features are disabled by the > disable_higher_order_debug logic and it holds the "slab merging is off > if any debug features are enabled" behavior. Sounds good. Where is the patch?
diff --git a/mm/slub.c b/mm/slub.c index 1efbb812..8cbf9f7 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -5704,6 +5704,10 @@ static int sysfs_slab_add(struct kmem_cache *s) return 0; } + if (!unmergeable && disable_higher_order_debug && + (slub_debug & SLAB_NEVER_MERGE)) + unmergeable = 1; + if (unmergeable) { /* * Slabcache can never be merged so we can use the name proper.