| Message ID | 20231210102747.13545-1-dinghao.liu@zju.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | nvdimm-btt: simplify code with the scope based resource management | expand |
On 12/10/23 03:27, Dinghao Liu wrote: > Use the scope based resource management (defined in > linux/cleanup.h) to automate resource lifetime > control on struct btt_sb *super in discover_arenas(). > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > --- > drivers/nvdimm/btt.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > index d5593b0dc700..ff42778b51de 100644 > --- a/drivers/nvdimm/btt.c > +++ b/drivers/nvdimm/btt.c > @@ -16,6 +16,7 @@ > #include <linux/fs.h> > #include <linux/nd.h> > #include <linux/backing-dev.h> > +#include <linux/cleanup.h> > #include "btt.h" > #include "nd.h" > > @@ -847,7 +848,7 @@ static int discover_arenas(struct btt *btt) > { > int ret = 0; > struct arena_info *arena; > - struct btt_sb *super; > + struct btt_sb *super __free(kfree) = NULL; > size_t remaining = btt->rawsize; > u64 cur_nlba = 0; > size_t cur_off = 0; > @@ -860,10 +861,8 @@ static int discover_arenas(struct btt *btt) > while (remaining) { > /* Alloc memory for arena */ > arena = alloc_arena(btt, 0, 0, 0); > - if (!arena) { > - ret = -ENOMEM; > - goto out_super; > - } > + if (!arena) > + return -ENOMEM; > > arena->infooff = cur_off; > ret = btt_info_read(arena, super); > @@ -919,14 +918,11 @@ static int discover_arenas(struct btt *btt) > btt->nlba = cur_nlba; > btt->init_state = INIT_READY; > > - kfree(super); > return ret; > > out: > kfree(arena); > free_arenas(btt); > - out_super: > - kfree(super); > return ret; > } > I would do the allocation like something below for the first chunk. Otherwise the rest LGTM. diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index d5593b0dc700..143921e7f26c 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -16,6 +16,7 @@ #include <linux/fs.h> #include <linux/nd.h> #include <linux/backing-dev.h> +#include <linux/cleanup.h> #include "btt.h" #include "nd.h" @@ -845,25 +846,23 @@ static void parse_arena_meta(struct arena_info *arena, struct btt_sb *super, static int discover_arenas(struct btt *btt) { + struct btt_sb *super __free(kfree) = + kzalloc(sizeof(*super), GFP_KERNEL); int ret = 0; struct arena_info *arena; - struct btt_sb *super; size_t remaining = btt->rawsize; u64 cur_nlba = 0; size_t cur_off = 0; int num_arenas = 0; - super = kzalloc(sizeof(*super), GFP_KERNEL); if (!super) return -ENOMEM; while (remaining) { /* Alloc memory for arena */ arena = alloc_arena(btt, 0, 0, 0); - if (!arena) { - ret = -ENOMEM; - goto out_super; - } + if (!arena) + return -ENOMEM; arena->infooff = cur_off; ret = btt_info_read(arena, super);
> > On 12/10/23 03:27, Dinghao Liu wrote: > > Use the scope based resource management (defined in > > linux/cleanup.h) to automate resource lifetime > > control on struct btt_sb *super in discover_arenas(). > > > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > > --- > > drivers/nvdimm/btt.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > > index d5593b0dc700..ff42778b51de 100644 > > --- a/drivers/nvdimm/btt.c > > +++ b/drivers/nvdimm/btt.c > > @@ -16,6 +16,7 @@ > > #include <linux/fs.h> > > #include <linux/nd.h> > > #include <linux/backing-dev.h> > > +#include <linux/cleanup.h> > > #include "btt.h" > > #include "nd.h" > > > > @@ -847,7 +848,7 @@ static int discover_arenas(struct btt *btt) > > { > > int ret = 0; > > struct arena_info *arena; > > - struct btt_sb *super; > > + struct btt_sb *super __free(kfree) = NULL; > > size_t remaining = btt->rawsize; > > u64 cur_nlba = 0; > > size_t cur_off = 0; > > @@ -860,10 +861,8 @@ static int discover_arenas(struct btt *btt) > > while (remaining) { > > /* Alloc memory for arena */ > > arena = alloc_arena(btt, 0, 0, 0); > > - if (!arena) { > > - ret = -ENOMEM; > > - goto out_super; > > - } > > + if (!arena) > > + return -ENOMEM; > > > > arena->infooff = cur_off; > > ret = btt_info_read(arena, super); > > @@ -919,14 +918,11 @@ static int discover_arenas(struct btt *btt) > > btt->nlba = cur_nlba; > > btt->init_state = INIT_READY; > > > > - kfree(super); > > return ret; > > > > out: > > kfree(arena); > > free_arenas(btt); > > - out_super: > > - kfree(super); > > return ret; > > } > > > > I would do the allocation like something below for the first chunk. Otherwise the rest LGTM. > > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > index d5593b0dc700..143921e7f26c 100644 > --- a/drivers/nvdimm/btt.c > +++ b/drivers/nvdimm/btt.c > @@ -16,6 +16,7 @@ > #include <linux/fs.h> > #include <linux/nd.h> > #include <linux/backing-dev.h> > +#include <linux/cleanup.h> > #include "btt.h" > #include "nd.h" > > @@ -845,25 +846,23 @@ static void parse_arena_meta(struct arena_info *arena, struct btt_sb *super, > > static int discover_arenas(struct btt *btt) > { > + struct btt_sb *super __free(kfree) = > + kzalloc(sizeof(*super), GFP_KERNEL); > int ret = 0; > struct arena_info *arena; > - struct btt_sb *super; > size_t remaining = btt->rawsize; > u64 cur_nlba = 0; > size_t cur_off = 0; > int num_arenas = 0; > > - super = kzalloc(sizeof(*super), GFP_KERNEL); > if (!super) > return -ENOMEM; > > while (remaining) { > /* Alloc memory for arena */ It's a little strange that we do not check super immediately after allocation. How about this: static int discover_arenas(struct btt *btt) { int ret = 0; struct arena_info *arena; - struct btt_sb *super; size_t remaining = btt->rawsize; u64 cur_nlba = 0; size_t cur_off = 0; int num_arenas = 0; - super = kzalloc(sizeof(*super), GFP_KERNEL); + struct btt_sb *super __free(kfree) = + kzalloc(sizeof(*super), GFP_KERNEL); if (!super) return -ENOMEM; while (remaining) {
On 12/12/23 20:12, dinghao.liu@zju.edu.cn wrote: >> >> On 12/10/23 03:27, Dinghao Liu wrote: >>> Use the scope based resource management (defined in >>> linux/cleanup.h) to automate resource lifetime >>> control on struct btt_sb *super in discover_arenas(). >>> >>> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> >>> --- >>> drivers/nvdimm/btt.c | 12 ++++-------- >>> 1 file changed, 4 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c >>> index d5593b0dc700..ff42778b51de 100644 >>> --- a/drivers/nvdimm/btt.c >>> +++ b/drivers/nvdimm/btt.c >>> @@ -16,6 +16,7 @@ >>> #include <linux/fs.h> >>> #include <linux/nd.h> >>> #include <linux/backing-dev.h> >>> +#include <linux/cleanup.h> >>> #include "btt.h" >>> #include "nd.h" >>> >>> @@ -847,7 +848,7 @@ static int discover_arenas(struct btt *btt) >>> { >>> int ret = 0; >>> struct arena_info *arena; >>> - struct btt_sb *super; >>> + struct btt_sb *super __free(kfree) = NULL; >>> size_t remaining = btt->rawsize; >>> u64 cur_nlba = 0; >>> size_t cur_off = 0; >>> @@ -860,10 +861,8 @@ static int discover_arenas(struct btt *btt) >>> while (remaining) { >>> /* Alloc memory for arena */ >>> arena = alloc_arena(btt, 0, 0, 0); >>> - if (!arena) { >>> - ret = -ENOMEM; >>> - goto out_super; >>> - } >>> + if (!arena) >>> + return -ENOMEM; >>> >>> arena->infooff = cur_off; >>> ret = btt_info_read(arena, super); >>> @@ -919,14 +918,11 @@ static int discover_arenas(struct btt *btt) >>> btt->nlba = cur_nlba; >>> btt->init_state = INIT_READY; >>> >>> - kfree(super); >>> return ret; >>> >>> out: >>> kfree(arena); >>> free_arenas(btt); >>> - out_super: >>> - kfree(super); >>> return ret; >>> } >>> >> >> I would do the allocation like something below for the first chunk. Otherwise the rest LGTM. >> >> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c >> index d5593b0dc700..143921e7f26c 100644 >> --- a/drivers/nvdimm/btt.c >> +++ b/drivers/nvdimm/btt.c >> @@ -16,6 +16,7 @@ >> #include <linux/fs.h> >> #include <linux/nd.h> >> #include <linux/backing-dev.h> >> +#include <linux/cleanup.h> >> #include "btt.h" >> #include "nd.h" >> >> @@ -845,25 +846,23 @@ static void parse_arena_meta(struct arena_info *arena, struct btt_sb *super, >> >> static int discover_arenas(struct btt *btt) >> { >> + struct btt_sb *super __free(kfree) = >> + kzalloc(sizeof(*super), GFP_KERNEL); >> int ret = 0; >> struct arena_info *arena; >> - struct btt_sb *super; >> size_t remaining = btt->rawsize; >> u64 cur_nlba = 0; >> size_t cur_off = 0; >> int num_arenas = 0; >> >> - super = kzalloc(sizeof(*super), GFP_KERNEL); >> if (!super) >> return -ENOMEM; >> >> while (remaining) { >> /* Alloc memory for arena */ > > It's a little strange that we do not check super immediately after allocation. > How about this: > > static int discover_arenas(struct btt *btt) > { > int ret = 0; > struct arena_info *arena; > - struct btt_sb *super; > size_t remaining = btt->rawsize; > u64 cur_nlba = 0; > size_t cur_off = 0; > int num_arenas = 0; > > - super = kzalloc(sizeof(*super), GFP_KERNEL); > + struct btt_sb *super __free(kfree) = > + kzalloc(sizeof(*super), GFP_KERNEL); > if (!super) > return -ENOMEM; > > while (remaining) { > That's fine by me
> > It's a little strange that we do not check super immediately after allocation. > > How about this: > > > > static int discover_arenas(struct btt *btt) > > { > > int ret = 0; > > struct arena_info *arena; > > - struct btt_sb *super; > > size_t remaining = btt->rawsize; > > u64 cur_nlba = 0; > > size_t cur_off = 0; > > int num_arenas = 0; > > > > - super = kzalloc(sizeof(*super), GFP_KERNEL); > > + struct btt_sb *super __free(kfree) = > > + kzalloc(sizeof(*super), GFP_KERNEL); > > if (!super) > > return -ENOMEM; > > > > while (remaining) { > > > > That's fine by me I will resend a new patch soon, thanks! Regards, Dinghao
diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index d5593b0dc700..ff42778b51de 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -16,6 +16,7 @@ #include <linux/fs.h> #include <linux/nd.h> #include <linux/backing-dev.h> +#include <linux/cleanup.h> #include "btt.h" #include "nd.h" @@ -847,7 +848,7 @@ static int discover_arenas(struct btt *btt) { int ret = 0; struct arena_info *arena; - struct btt_sb *super; + struct btt_sb *super __free(kfree) = NULL; size_t remaining = btt->rawsize; u64 cur_nlba = 0; size_t cur_off = 0; @@ -860,10 +861,8 @@ static int discover_arenas(struct btt *btt) while (remaining) { /* Alloc memory for arena */ arena = alloc_arena(btt, 0, 0, 0); - if (!arena) { - ret = -ENOMEM; - goto out_super; - } + if (!arena) + return -ENOMEM; arena->infooff = cur_off; ret = btt_info_read(arena, super); @@ -919,14 +918,11 @@ static int discover_arenas(struct btt *btt) btt->nlba = cur_nlba; btt->init_state = INIT_READY; - kfree(super); return ret; out: kfree(arena); free_arenas(btt); - out_super: - kfree(super); return ret; }
Use the scope based resource management (defined in linux/cleanup.h) to automate resource lifetime control on struct btt_sb *super in discover_arenas(). Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> --- drivers/nvdimm/btt.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)