From: Faiyaz Mohammed <faiyazm@codeaurora.org>
To: Vlastimil Babka <vbabka@suse.cz>, Greg KH <greg@kroah.com>
Cc: cl@linux.com, penberg@kernel.org, rientjes@google.com,
iamjoonsoo.kim@lge.com, akpm@linux-foundation.org,
linux-mm@kvack.org, linux-kernel@vger.kernel.org,
glittao@gmail.com, vinmenon@codeaurora.org
Subject: Re: [PATCH v7] mm: slub: move sysfs slab alloc/free interfaces to debugfs
Date: Mon, 31 May 2021 15:20:40 +0530 [thread overview]
Message-ID: <2fc580b5-13a5-1a54-fe0b-9843ec685e64@codeaurora.org> (raw)
In-Reply-To: <043cfec6-7881-46e8-ec9c-3b4d6611ac2c@suse.cz>
On 5/31/2021 1:49 PM, Vlastimil Babka wrote:
> On 5/31/21 9:11 AM, Faiyaz Mohammed wrote:
>>
>>
>> On 5/26/2021 5:43 PM, Vlastimil Babka wrote:
>>> On 5/26/21 1:48 PM, Greg KH wrote:
>>>> On Wed, May 26, 2021 at 01:38:55PM +0200, Vlastimil Babka wrote:
>>>>>
>>>>> alias_list a single list and both slab_sysfs_init() and slab_debugfs_init()
>>>>> flush it. So only the init call that happens to be called first, does actually
>>>>> find an unflushed list. I think you
>>>>> need to use a separate list for debugfs (simpler) or a shared list with both
>>>>> sysfs and debugfs processing (probably more complicated).
>>>>>
>>>>> And finally a question, perhaps also for Greg. With sysfs, we hand out the
>>>>> lifecycle of struct kmem_cache to sysfs, to ensure we are not reading sysfs
>>>>> files of a cache that has been removed.
>>>>>
>>>>> But with debugfs, what are the guarantees that things won't blow up when a
>>>>> debugfs file is being read while somebody calls kmem_cache_destroy() on the cache?
>>>>
>>>> It's much harder, but usually the default debugfs_file_create() will
>>>> handle this for you. See the debugfs_file_create_unsafe() for the
>>>> "other" variant where you know you can tear things down "safely".
>>>
>>> Right, so IIUC debugfs will guarantee that while somebody reads the files, the
>>> debugfs cleanup will block, as debugfs_file_get() comment explains.
>>>
>>> In that case I think we have the cleanup order wrong in this patch:
>>>
>>> shutdown_cache() should first do debugfs_slab_release() (which would block) and
>>> only then proceed with slab_kmem_cache_release() which destroys the fundamental
>>> structures such as kmem_cache_node, which are also accessed by the debugfs file
>>> handlers.
>>>
>> If user is trying to read the data during shutdown_cache(), then I think
>> it's possible user will get empty data, to avoid that we can call
>
> Empty data is fine, when the cache is going away anyway.
>
>> debugfs_slab_release() first and then do other stuff in shutdown_cache().
>
> Everything above list_del(&s->list) should be OK, it's equivalent to normal
> cache operations which the debugfs files must cope with anyway.
> list_del(&s->list) is OK as the debugfs handlers don't go through the list. It's
> slab_kmem_cache_release() that matters.
>
okay, I will move debugfs_slab_release() before the
slab_kmem_cache_release() in next patch version.
>>>> That being said, yes there are still issues in this area, be careful
>>>> about what tools you expect to be constantly hitting debugfs files.
>>>
>>> FWIW, the files are accessible only to root.
>>>
>>>> thanks,
>>>>
>>>> greg k-h
>>>>
>>>
>>
>
next prev parent reply other threads:[~2021-05-31 9:51 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-25 7:38 [PATCH v7] mm: slub: move sysfs slab alloc/free interfaces to debugfs Faiyaz Mohammed
2021-05-25 7:53 ` Greg KH
2021-05-25 8:57 ` Faiyaz Mohammed
2021-05-25 11:54 ` Greg KH
2021-05-26 11:03 ` Vlastimil Babka
2021-05-26 15:06 ` Faiyaz Mohammed
2021-05-26 15:07 ` Vlastimil Babka
2021-05-26 11:38 ` Vlastimil Babka
2021-05-26 11:48 ` Greg KH
2021-05-26 12:13 ` Vlastimil Babka
2021-05-31 7:11 ` Faiyaz Mohammed
2021-05-31 8:19 ` Vlastimil Babka
2021-05-31 9:50 ` Faiyaz Mohammed [this message]
2021-05-31 6:55 ` Faiyaz Mohammed
2021-05-31 9:55 ` Vlastimil Babka
2021-05-31 11:07 ` Faiyaz Mohammed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2fc580b5-13a5-1a54-fe0b-9843ec685e64@codeaurora.org \
--to=faiyazm@codeaurora.org \
--cc=akpm@linux-foundation.org \
--cc=cl@linux.com \
--cc=glittao@gmail.com \
--cc=greg@kroah.com \
--cc=iamjoonsoo.kim@lge.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=penberg@kernel.org \
--cc=rientjes@google.com \
--cc=vbabka@suse.cz \
--cc=vinmenon@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).