| Message ID | 20240201081935.200031-1-chentao@kylinos.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfsd: Simplify the allocation of slab caches in nfsd_drc_slab_create | expand |
On 1 Feb 2024, at 3:19, Kunwu Chan wrote: > Use the new KMEM_CACHE() macro instead of direct kmem_cache_create > to simplify the creation of SLAB caches. > Make the code cleaner and more readable. > > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > --- > fs/nfsd/nfscache.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c > index 5c1a4a0aa605..64ce0cc22197 100644 > --- a/fs/nfsd/nfscache.c > +++ b/fs/nfsd/nfscache.c > @@ -166,8 +166,7 @@ nfsd_reply_cache_free(struct nfsd_drc_bucket *b, struct nfsd_cacherep *rp, > > int nfsd_drc_slab_create(void) > { > - drc_slab = kmem_cache_create("nfsd_drc", > - sizeof(struct nfsd_cacherep), 0, 0, NULL); > + drc_slab = KMEM_CACHE(nfsd_cacherep, 0); > return drc_slab ? 0: -ENOMEM; > } > > -- > 2.39.2 I don't agree that the code is cleaner or more readable like this. I really dislike having to parse through the extra "simplification" to see what's actually being called and sent. Just my .02 worth. Ben
On Fri, 2024-02-02 at 09:13 -0500, Benjamin Coddington wrote: > On 1 Feb 2024, at 3:19, Kunwu Chan wrote: > > > Use the new KMEM_CACHE() macro instead of direct kmem_cache_create > > to simplify the creation of SLAB caches. > > Make the code cleaner and more readable. > > > > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > > --- > > fs/nfsd/nfscache.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c > > index 5c1a4a0aa605..64ce0cc22197 100644 > > --- a/fs/nfsd/nfscache.c > > +++ b/fs/nfsd/nfscache.c > > @@ -166,8 +166,7 @@ nfsd_reply_cache_free(struct nfsd_drc_bucket *b, struct nfsd_cacherep *rp, > > > > int nfsd_drc_slab_create(void) > > { > > - drc_slab = kmem_cache_create("nfsd_drc", > > - sizeof(struct nfsd_cacherep), 0, 0, NULL); > > + drc_slab = KMEM_CACHE(nfsd_cacherep, 0); > > return drc_slab ? 0: -ENOMEM; > > } > > > > -- > > 2.39.2 > > I don't agree that the code is cleaner or more readable like this. I really > dislike having to parse through the extra "simplification" to see what's > actually being called and sent. > > Just my .02 worth. > > Ben > This will also result in a behavioral change. The "nfsd_drc" string is lost with the above macro and (I think) the new name will be "nfsd_cacherep". I'm not necessarily opposed to that, as I don't think anything depends on the old name, but it should at least be noted in the changelog.
On Sat, 03 Feb 2024, Benjamin Coddington wrote: > On 1 Feb 2024, at 3:19, Kunwu Chan wrote: > > > Use the new KMEM_CACHE() macro instead of direct kmem_cache_create > > to simplify the creation of SLAB caches. > > Make the code cleaner and more readable. > > > > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > > --- > > fs/nfsd/nfscache.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c > > index 5c1a4a0aa605..64ce0cc22197 100644 > > --- a/fs/nfsd/nfscache.c > > +++ b/fs/nfsd/nfscache.c > > @@ -166,8 +166,7 @@ nfsd_reply_cache_free(struct nfsd_drc_bucket *b, struct nfsd_cacherep *rp, > > > > int nfsd_drc_slab_create(void) > > { > > - drc_slab = kmem_cache_create("nfsd_drc", > > - sizeof(struct nfsd_cacherep), 0, 0, NULL); > > + drc_slab = KMEM_CACHE(nfsd_cacherep, 0); > > return drc_slab ? 0: -ENOMEM; > > } > > > > -- > > 2.39.2 > > I don't agree that the code is cleaner or more readable like this. I really > dislike having to parse through the extra "simplification" to see what's > actually being called and sent. > > Just my .02 worth. > In general I agree that wrappers like this can hinder as much as they help - if not more. In this particular case it doesn't seem to bother me. This is probably because it is only used in initialisation code and I don't look at that nearly as much as code that uses the initialised things. Initialisation/cleanup code often has a lot of boilerplate which can make it look messy. Reducing that, which I think this patch helps with, can be a good thing. So I agree that we should be cautious about using (or creating) new wrapper macros, but in this case I am mildly in favour. Thanks, NeilBrown
Thank you to all the guys who responded to my emails. On 2024/2/2 22:24, Jeff Layton wrote: > On Fri, 2024-02-02 at 09:13 -0500, Benjamin Coddington wrote: >> On 1 Feb 2024, at 3:19, Kunwu Chan wrote: >> >>> Use the new KMEM_CACHE() macro instead of direct kmem_cache_create >>> to simplify the creation of SLAB caches. >>> Make the code cleaner and more readable. >>> >>> Signed-off-by: Kunwu Chan <chentao@kylinos.cn> >>> --- >>> fs/nfsd/nfscache.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c >>> index 5c1a4a0aa605..64ce0cc22197 100644 >>> --- a/fs/nfsd/nfscache.c >>> +++ b/fs/nfsd/nfscache.c >>> @@ -166,8 +166,7 @@ nfsd_reply_cache_free(struct nfsd_drc_bucket *b, struct nfsd_cacherep *rp, >>> >>> int nfsd_drc_slab_create(void) >>> { >>> - drc_slab = kmem_cache_create("nfsd_drc", >>> - sizeof(struct nfsd_cacherep), 0, 0, NULL); >>> + drc_slab = KMEM_CACHE(nfsd_cacherep, 0); >>> return drc_slab ? 0: -ENOMEM; >>> } >>> >>> -- >>> 2.39.2 >> >> I don't agree that the code is cleaner or more readable like this. I really >> dislike having to parse through the extra "simplification" to see what's >> actually being called and sent. >> >> Just my .02 worth. >> >> Ben >> > Everyone has a different opinion. From newcomers like me, a simple code is more important than checking all the args of a call function to understand what it does. Too many default arguments can cost us a lot of time that could be spent understanding the main logic of the module code, rather than wasting it on a single line of calls. > This will also result in a behavioral change. The "nfsd_drc" string is > lost with the above macro and (I think) the new name will be > "nfsd_cacherep". I'm not necessarily opposed to that, as I don't think > anything depends on the old name, but it should at least be noted in the > changelog. Thanks i'll update my v2 patch with a new commit msg to show the name change. >
diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c index 5c1a4a0aa605..64ce0cc22197 100644 --- a/fs/nfsd/nfscache.c +++ b/fs/nfsd/nfscache.c @@ -166,8 +166,7 @@ nfsd_reply_cache_free(struct nfsd_drc_bucket *b, struct nfsd_cacherep *rp, int nfsd_drc_slab_create(void) { - drc_slab = kmem_cache_create("nfsd_drc", - sizeof(struct nfsd_cacherep), 0, 0, NULL); + drc_slab = KMEM_CACHE(nfsd_cacherep, 0); return drc_slab ? 0: -ENOMEM; }
Use the new KMEM_CACHE() macro instead of direct kmem_cache_create to simplify the creation of SLAB caches. Make the code cleaner and more readable. Signed-off-by: Kunwu Chan <chentao@kylinos.cn> --- fs/nfsd/nfscache.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)