| Message ID | 20200423204620.26395-1-christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: fnic: Use kmalloc instead of vmalloc for a small memory allocation | expand |
On Thu, Apr 23, 2020 at 10:46:20PM +0200, Christophe JAILLET wrote: > 'struct fc_trace_flag_type' is just a few bytes long. There is no need > to allocate such a structure with vmalloc. Using kmalloc instead. > > While at it, axe a useless test when freeing the memory. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/scsi/fnic/fnic_debugfs.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/scsi/fnic/fnic_debugfs.c b/drivers/scsi/fnic/fnic_debugfs.c > index 13f7d88d6e57..8d6ce3470594 100644 > --- a/drivers/scsi/fnic/fnic_debugfs.c > +++ b/drivers/scsi/fnic/fnic_debugfs.c > @@ -58,8 +58,7 @@ int fnic_debugfs_init(void) > fnic_trace_debugfs_root); > > /* Allocate memory to structure */ > - fc_trc_flag = (struct fc_trace_flag_type *) > - vmalloc(sizeof(struct fc_trace_flag_type)); > + fc_trc_flag = kmalloc(sizeof(*fc_trc_flag), GFP_KERNEL); > > if (fc_trc_flag) { I hate success handling... This test should be reversed so that we do error handling. It should return -ENOMEM instead of 0 on allocation failure, otherwise it leads to a NULL dereference down the road. Although, of course in current kernel small size allocations like this never fail in real life. The other thing I wonder is if we should just replace the vmalloc() implementation with kvmalloc(). IOW rename vmalloc() to __vmalloc() and "#define vmalloc kvmalloc" (not literally). That was allocations of less than a page would always be done with kmalloc(). regards, dan carpenter
On 4/23/20 10:46 PM, Christophe JAILLET wrote: > 'struct fc_trace_flag_type' is just a few bytes long. There is no need > to allocate such a structure with vmalloc. Using kmalloc instead. > > While at it, axe a useless test when freeing the memory. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/scsi/fnic/fnic_debugfs.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/scsi/fnic/fnic_debugfs.c b/drivers/scsi/fnic/fnic_debugfs.c > index 13f7d88d6e57..8d6ce3470594 100644 > --- a/drivers/scsi/fnic/fnic_debugfs.c > +++ b/drivers/scsi/fnic/fnic_debugfs.c > @@ -58,8 +58,7 @@ int fnic_debugfs_init(void) > fnic_trace_debugfs_root); > > /* Allocate memory to structure */ > - fc_trc_flag = (struct fc_trace_flag_type *) > - vmalloc(sizeof(struct fc_trace_flag_type)); > + fc_trc_flag = kmalloc(sizeof(*fc_trc_flag), GFP_KERNEL); > > if (fc_trc_flag) { > fc_trc_flag->fc_row_file = 0; > @@ -87,8 +86,7 @@ void fnic_debugfs_terminate(void) > debugfs_remove(fnic_trace_debugfs_root); > fnic_trace_debugfs_root = NULL; > > - if (fc_trc_flag) > - vfree(fc_trc_flag); > + kfree(fc_trc_flag); > } > > /* > Reviewed-by: Hannes Reinecke <har@suse.de> Cheers, Hannes
diff --git a/drivers/scsi/fnic/fnic_debugfs.c b/drivers/scsi/fnic/fnic_debugfs.c index 13f7d88d6e57..8d6ce3470594 100644 --- a/drivers/scsi/fnic/fnic_debugfs.c +++ b/drivers/scsi/fnic/fnic_debugfs.c @@ -58,8 +58,7 @@ int fnic_debugfs_init(void) fnic_trace_debugfs_root); /* Allocate memory to structure */ - fc_trc_flag = (struct fc_trace_flag_type *) - vmalloc(sizeof(struct fc_trace_flag_type)); + fc_trc_flag = kmalloc(sizeof(*fc_trc_flag), GFP_KERNEL); if (fc_trc_flag) { fc_trc_flag->fc_row_file = 0; @@ -87,8 +86,7 @@ void fnic_debugfs_terminate(void) debugfs_remove(fnic_trace_debugfs_root); fnic_trace_debugfs_root = NULL; - if (fc_trc_flag) - vfree(fc_trc_flag); + kfree(fc_trc_flag); } /*
'struct fc_trace_flag_type' is just a few bytes long. There is no need to allocate such a structure with vmalloc. Using kmalloc instead. While at it, axe a useless test when freeing the memory. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/scsi/fnic/fnic_debugfs.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)