| Message ID | 20240903-work-kmem_cache_args-v2-0-76f97e9a4560@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | slab: add struct kmem_cache_args | expand |
On Tue, Sep 03, 2024 at 04:20:41PM +0200, Christian Brauner wrote: > However, I came up with an alternative using _Generic() to create a > compatibility layer that will call the correct variant of > kmem_cache_create() depending on whether struct kmem_cache_args is > passed or not. That compatibility layer can stay in place until we > updated all calls to be based on struct kmem_cache_args. Very nice! Yeah, this looks good to me. I'd been long pondering something like this for kmalloc itself (though codetags have taken that in a different direction). All the patches look like the right way forward to me: Reviewed-by: Kees Cook <kees@kernel.org>
Hi,
This is a really nice improvement! With fear of offending someone, the
kmem_cache API has really just grown warts over the years, with
additions slapped on top without anyone taking the time to refactor it a
bit and clean it up. It's about time.
Reviewed-by: Jens Axboe <axboe@kernel.dk>
On 9/3/24 16:20, Christian Brauner wrote: > Hey, > > As discussed last week the various kmem_cache_*() functions should be > replaced by a unified function that is based around a struct, with only > the basic parameters passed separately. > > Vlastimil already said that he would like to keep core parameters out > of the struct: name, object size, and flags. I personally don't care > much and would not object to moving everything into the struct but > that's a matter of taste and I yield that decision power to the > maintainer. > > In the first version I pointed out that the choice of name is somewhat > forced as kmem_cache_create() is taken and the only way to reuse it > would be to replace all users in one go. Or to do a global > sed/kmem_cache_create()/kmem_cache_create2()/g. And then introduce > kmem_cache_setup(). That doesn't strike me as a viable option. > > If we really cared about the *_create() suffix then an alternative might > be to do a sed/kmem_cache_setup()/kmem_cache_create()/g after every user > in the kernel is ported. I honestly don't think that's worth it but I > wanted to at least mention it to highlight the fact that this might lead > to a naming compromise. > > However, I came up with an alternative using _Generic() to create a > compatibility layer that will call the correct variant of > kmem_cache_create() depending on whether struct kmem_cache_args is > passed or not. That compatibility layer can stay in place until we > updated all calls to be based on struct kmem_cache_args. > > From a cursory grep (and not excluding Documentation mentions) we will > have to replace 44 kmem_cache_create_usercopy() calls and about 463 > kmem_cache_create() calls which makes for a bit above 500 calls to port > to kmem_cache_setup(). That'll probably be good work for people getting > into kernel development. > > Signed-off-by: Christian Brauner <brauner@kernel.org> Besides the nits I replied to individual patches, LGTM and thanks for doing the work. You could add to all: Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Too bad it's quite late for 6.12, right? It would have to be vfs tree anyway for that due to the kmem_cache_create_rcu() prerequisities. Otherwise I can handle it in the slab tree after the merge window, for 6.13. Also for any more postings please Cc the SLAB ALLOCATOR section, only a small subset of that is completely MIA :) Thanks, Vlastimil > --- > Changes in v2: > - Remove kmem_cache_setup() and add a compatibility layer built around > _Generic() so that we can keep the kmem_cache_create() name and type > switch on the third argument to either call __kmem_cache_create() or > __kmem_cache_create_args(). > - Link to v1: https://lore.kernel.org/r/20240902-work-kmem_cache_args-v1-0-27d05bc05128@kernel.org > > --- > Christian Brauner (15): > sl*b: s/__kmem_cache_create/do_kmem_cache_create/g > slab: add struct kmem_cache_args > slab: port kmem_cache_create() to struct kmem_cache_args > slab: port kmem_cache_create_rcu() to struct kmem_cache_args > slab: port kmem_cache_create_usercopy() to struct kmem_cache_args > slab: pass struct kmem_cache_args to create_cache() > slub: pull kmem_cache_open() into do_kmem_cache_create() > slab: pass struct kmem_cache_args to do_kmem_cache_create() > sl*b: remove rcu_freeptr_offset from struct kmem_cache > slab: port KMEM_CACHE() to struct kmem_cache_args > slab: port KMEM_CACHE_USERCOPY() to struct kmem_cache_args > slab: create kmem_cache_create() compatibility layer > file: port to struct kmem_cache_args > slab: remove kmem_cache_create_rcu() > io_uring: port to struct kmem_cache_args > > fs/file_table.c | 11 +++- > include/linux/slab.h | 60 ++++++++++++++----- > io_uring/io_uring.c | 14 +++-- > mm/slab.h | 6 +- > mm/slab_common.c | 150 +++++++++++++++++++---------------------------- > mm/slub.c | 162 +++++++++++++++++++++++++-------------------------- > 6 files changed, 203 insertions(+), 200 deletions(-) > --- > base-commit: 6e016babce7c845ed015da25c7a097fa3482d95a > change-id: 20240902-work-kmem_cache_args-e9760972c7d4 >
On Wed, Sep 04, 2024 at 10:25:32AM GMT, Vlastimil Babka wrote: > On 9/3/24 16:20, Christian Brauner wrote: > > Hey, > > > > As discussed last week the various kmem_cache_*() functions should be > > replaced by a unified function that is based around a struct, with only > > the basic parameters passed separately. > > > > Vlastimil already said that he would like to keep core parameters out > > of the struct: name, object size, and flags. I personally don't care > > much and would not object to moving everything into the struct but > > that's a matter of taste and I yield that decision power to the > > maintainer. > > > > In the first version I pointed out that the choice of name is somewhat > > forced as kmem_cache_create() is taken and the only way to reuse it > > would be to replace all users in one go. Or to do a global > > sed/kmem_cache_create()/kmem_cache_create2()/g. And then introduce > > kmem_cache_setup(). That doesn't strike me as a viable option. > > > > If we really cared about the *_create() suffix then an alternative might > > be to do a sed/kmem_cache_setup()/kmem_cache_create()/g after every user > > in the kernel is ported. I honestly don't think that's worth it but I > > wanted to at least mention it to highlight the fact that this might lead > > to a naming compromise. > > > > However, I came up with an alternative using _Generic() to create a > > compatibility layer that will call the correct variant of > > kmem_cache_create() depending on whether struct kmem_cache_args is > > passed or not. That compatibility layer can stay in place until we > > updated all calls to be based on struct kmem_cache_args. > > > > From a cursory grep (and not excluding Documentation mentions) we will > > have to replace 44 kmem_cache_create_usercopy() calls and about 463 > > kmem_cache_create() calls which makes for a bit above 500 calls to port > > to kmem_cache_setup(). That'll probably be good work for people getting > > into kernel development. > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > Besides the nits I replied to individual patches, LGTM and thanks for doing > the work. You could add to all: > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > > Too bad it's quite late for 6.12, right? It would have to be vfs tree anyway > for that due to the kmem_cache_create_rcu() prerequisities. Otherwise I can Imho, we can do it and Linus can always just tell us to go away and wait for v6.13. But if you prefer to wait that's fine for me too. And I don't even need to take it all through the vfs tree. I mean, I'm happy to do it but the vfs.file branch in it's current form is stable. So you could just git pull -S --no-ff git@gitolite.kernel.org:pub/scm/linux/kernel/git/vfs/vfs vfs.file and note in the merge message that you're bringing in the branch as prerequisites for the rework and then pull this series in. My pull requests will go out latest on Friday before the final release. If Linus merges it you can just send yours after. > handle it in the slab tree after the merge window, for 6.13. > > Also for any more postings please Cc the SLAB ALLOCATOR section, only a > small subset of that is completely MIA :) Sure.
On 9/4/24 10:42, Christian Brauner wrote: > On Wed, Sep 04, 2024 at 10:25:32AM GMT, Vlastimil Babka wrote: >> On 9/3/24 16:20, Christian Brauner wrote: >> > Hey, >> > >> > As discussed last week the various kmem_cache_*() functions should be >> > replaced by a unified function that is based around a struct, with only >> > the basic parameters passed separately. >> > >> > Vlastimil already said that he would like to keep core parameters out >> > of the struct: name, object size, and flags. I personally don't care >> > much and would not object to moving everything into the struct but >> > that's a matter of taste and I yield that decision power to the >> > maintainer. >> > >> > In the first version I pointed out that the choice of name is somewhat >> > forced as kmem_cache_create() is taken and the only way to reuse it >> > would be to replace all users in one go. Or to do a global >> > sed/kmem_cache_create()/kmem_cache_create2()/g. And then introduce >> > kmem_cache_setup(). That doesn't strike me as a viable option. >> > >> > If we really cared about the *_create() suffix then an alternative might >> > be to do a sed/kmem_cache_setup()/kmem_cache_create()/g after every user >> > in the kernel is ported. I honestly don't think that's worth it but I >> > wanted to at least mention it to highlight the fact that this might lead >> > to a naming compromise. >> > >> > However, I came up with an alternative using _Generic() to create a >> > compatibility layer that will call the correct variant of >> > kmem_cache_create() depending on whether struct kmem_cache_args is >> > passed or not. That compatibility layer can stay in place until we >> > updated all calls to be based on struct kmem_cache_args. >> > >> > From a cursory grep (and not excluding Documentation mentions) we will >> > have to replace 44 kmem_cache_create_usercopy() calls and about 463 >> > kmem_cache_create() calls which makes for a bit above 500 calls to port >> > to kmem_cache_setup(). That'll probably be good work for people getting >> > into kernel development. >> > >> > Signed-off-by: Christian Brauner <brauner@kernel.org> >> >> Besides the nits I replied to individual patches, LGTM and thanks for doing >> the work. You could add to all: >> >> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> >> >> Too bad it's quite late for 6.12, right? It would have to be vfs tree anyway >> for that due to the kmem_cache_create_rcu() prerequisities. Otherwise I can > > Imho, we can do it and Linus can always just tell us to go away and wait > for v6.13. But if you prefer to wait that's fine for me too. Right, we can try. Maybe there's rc8 anyway due to conferences overlapping with merge window otherwise, so there's more soak time in -next possible. > And I don't even need to take it all through the vfs tree. I mean, I'm > happy to do it but the vfs.file branch in it's current form is stable. > So you could just > > git pull -S --no-ff git@gitolite.kernel.org:pub/scm/linux/kernel/git/vfs/vfs vfs.file > > and note in the merge message that you're bringing in the branch as > prerequisites for the rework and then pull this series in. OK I'll do that with a v3! Thanks. > My pull requests will go out latest on Friday before the final release. > If Linus merges it you can just send yours after. > >> handle it in the slab tree after the merge window, for 6.13. >> >> Also for any more postings please Cc the SLAB ALLOCATOR section, only a >> small subset of that is completely MIA :) > > Sure.
On Tue, Sep 03, 2024 at 01:25:22PM GMT, Jens Axboe wrote: > Hi, > > This is a really nice improvement! With fear of offending someone, the > kmem_cache API has really just grown warts over the years, with > additions slapped on top without anyone taking the time to refactor it a > bit and clean it up. It's about time. Thank you! I'm sure there's more to improve here in the future. :)
Hey, As discussed last week the various kmem_cache_*() functions should be replaced by a unified function that is based around a struct, with only the basic parameters passed separately. Vlastimil already said that he would like to keep core parameters out of the struct: name, object size, and flags. I personally don't care much and would not object to moving everything into the struct but that's a matter of taste and I yield that decision power to the maintainer. In the first version I pointed out that the choice of name is somewhat forced as kmem_cache_create() is taken and the only way to reuse it would be to replace all users in one go. Or to do a global sed/kmem_cache_create()/kmem_cache_create2()/g. And then introduce kmem_cache_setup(). That doesn't strike me as a viable option. If we really cared about the *_create() suffix then an alternative might be to do a sed/kmem_cache_setup()/kmem_cache_create()/g after every user in the kernel is ported. I honestly don't think that's worth it but I wanted to at least mention it to highlight the fact that this might lead to a naming compromise. However, I came up with an alternative using _Generic() to create a compatibility layer that will call the correct variant of kmem_cache_create() depending on whether struct kmem_cache_args is passed or not. That compatibility layer can stay in place until we updated all calls to be based on struct kmem_cache_args. From a cursory grep (and not excluding Documentation mentions) we will have to replace 44 kmem_cache_create_usercopy() calls and about 463 kmem_cache_create() calls which makes for a bit above 500 calls to port to kmem_cache_setup(). That'll probably be good work for people getting into kernel development. Signed-off-by: Christian Brauner <brauner@kernel.org> --- Changes in v2: - Remove kmem_cache_setup() and add a compatibility layer built around _Generic() so that we can keep the kmem_cache_create() name and type switch on the third argument to either call __kmem_cache_create() or __kmem_cache_create_args(). - Link to v1: https://lore.kernel.org/r/20240902-work-kmem_cache_args-v1-0-27d05bc05128@kernel.org --- Christian Brauner (15): sl*b: s/__kmem_cache_create/do_kmem_cache_create/g slab: add struct kmem_cache_args slab: port kmem_cache_create() to struct kmem_cache_args slab: port kmem_cache_create_rcu() to struct kmem_cache_args slab: port kmem_cache_create_usercopy() to struct kmem_cache_args slab: pass struct kmem_cache_args to create_cache() slub: pull kmem_cache_open() into do_kmem_cache_create() slab: pass struct kmem_cache_args to do_kmem_cache_create() sl*b: remove rcu_freeptr_offset from struct kmem_cache slab: port KMEM_CACHE() to struct kmem_cache_args slab: port KMEM_CACHE_USERCOPY() to struct kmem_cache_args slab: create kmem_cache_create() compatibility layer file: port to struct kmem_cache_args slab: remove kmem_cache_create_rcu() io_uring: port to struct kmem_cache_args fs/file_table.c | 11 +++- include/linux/slab.h | 60 ++++++++++++++----- io_uring/io_uring.c | 14 +++-- mm/slab.h | 6 +- mm/slab_common.c | 150 +++++++++++++++++++---------------------------- mm/slub.c | 162 +++++++++++++++++++++++++-------------------------- 6 files changed, 203 insertions(+), 200 deletions(-) --- base-commit: 6e016babce7c845ed015da25c7a097fa3482d95a change-id: 20240902-work-kmem_cache_args-e9760972c7d4