| Message ID | 5beae8ff.1c69fb81.bd59a.b7fa@mx.google.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | drivers/scsi/fnic/fnic_trace.c: Use vzalloc | expand |
On 13/11/2018 15:08, Sabyasachi Gupta wrote: > Replaced vmalloc + memset with vzalloc > > Signed-off-by: Sabyasachi Gupta <sabyasachi.linux@gmail.com> > --- > drivers/scsi/fnic/fnic_trace.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/fnic/fnic_trace.c b/drivers/scsi/fnic/fnic_trace.c > index 8271785..129ab27 100644 > --- a/drivers/scsi/fnic/fnic_trace.c > +++ b/drivers/scsi/fnic/fnic_trace.c > @@ -468,14 +468,14 @@ int fnic_trace_buf_init(void) > fnic_max_trace_entries = (trace_max_pages * PAGE_SIZE)/ > FNIC_ENTRY_SIZE_BYTES; > > - fnic_trace_buf_p = (unsigned long)vmalloc((trace_max_pages * PAGE_SIZE)); > + fnic_trace_buf_p = (unsigned long)vzalloc((trace_max_pages * > + PAGE_SIZE)); If you remove the extra brackets in vzalloc() argument then you may not spill onto the next line. > if (!fnic_trace_buf_p) { > printk(KERN_ERR PFX "Failed to allocate memory " > "for fnic_trace_buf_p\n"); > err = -ENOMEM; > goto err_fnic_trace_buf_init; > } > - memset((void *)fnic_trace_buf_p, 0, (trace_max_pages * PAGE_SIZE)); > > fnic_trace_entries.page_offset = > vmalloc(array_size(fnic_max_trace_entries, >
On 13/11/2018 16:44, John Garry wrote: > On 13/11/2018 15:08, Sabyasachi Gupta wrote: >> Replaced vmalloc + memset with vzalloc >> >> Signed-off-by: Sabyasachi Gupta <sabyasachi.linux@gmail.com> >> --- >> drivers/scsi/fnic/fnic_trace.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/scsi/fnic/fnic_trace.c >> b/drivers/scsi/fnic/fnic_trace.c >> index 8271785..129ab27 100644 >> --- a/drivers/scsi/fnic/fnic_trace.c >> +++ b/drivers/scsi/fnic/fnic_trace.c >> @@ -468,14 +468,14 @@ int fnic_trace_buf_init(void) >> fnic_max_trace_entries = (trace_max_pages * PAGE_SIZE)/ >> FNIC_ENTRY_SIZE_BYTES; >> >> - fnic_trace_buf_p = (unsigned long)vmalloc((trace_max_pages * >> PAGE_SIZE)); >> + fnic_trace_buf_p = (unsigned long)vzalloc((trace_max_pages * >> + PAGE_SIZE)); > > If you remove the extra brackets in vzalloc() argument then you may not > spill onto the next line. And remove the unnecessary cast. vzalloc() (just like vmalloc()) returns a void*, so no reason to cast it.
On Tue, Nov 13, 2018 at 9:23 PM Johannes Thumshirn <jthumshirn@suse.de> wrote: > > On 13/11/2018 16:44, John Garry wrote: > > On 13/11/2018 15:08, Sabyasachi Gupta wrote: > >> Replaced vmalloc + memset with vzalloc > >> > >> Signed-off-by: Sabyasachi Gupta <sabyasachi.linux@gmail.com> > >> --- > >> drivers/scsi/fnic/fnic_trace.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/scsi/fnic/fnic_trace.c > >> b/drivers/scsi/fnic/fnic_trace.c > >> index 8271785..129ab27 100644 > >> --- a/drivers/scsi/fnic/fnic_trace.c > >> +++ b/drivers/scsi/fnic/fnic_trace.c > >> @@ -468,14 +468,14 @@ int fnic_trace_buf_init(void) > >> fnic_max_trace_entries = (trace_max_pages * PAGE_SIZE)/ > >> FNIC_ENTRY_SIZE_BYTES; > >> > >> - fnic_trace_buf_p = (unsigned long)vmalloc((trace_max_pages * > >> PAGE_SIZE)); > >> + fnic_trace_buf_p = (unsigned long)vzalloc((trace_max_pages * > >> + PAGE_SIZE)); > > > > If you remove the extra brackets in vzalloc() argument then you may not > > spill onto the next line. > > And remove the unnecessary cast. vzalloc() (just like vmalloc()) returns > a void*, so no reason to cast it. > > -- > Johannes Thumshirn SUSE Labs > jthumshirn@suse.de +49 911 74053 689 > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: Felix Imendörffer, Jane Smithard, Graham Norton > HRB 21284 (AG Nürnberg) > Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 I will remove it and send v2
On Tue, 2018-11-13 at 16:53 +0100, Johannes Thumshirn wrote: > On 13/11/2018 16:44, John Garry wrote: > > On 13/11/2018 15:08, Sabyasachi Gupta wrote: [...] > > > - fnic_trace_buf_p = (unsigned long)vmalloc((trace_max_pages * > > > PAGE_SIZE)); > > > + fnic_trace_buf_p = (unsigned long)vzalloc((trace_max_pages * > > > + PAGE_SIZE)); > > > > If you remove the extra brackets in vzalloc() argument then you may > > not spill onto the next line. > > And remove the unnecessary cast. vzalloc() (just like vmalloc()) > returns a void*, so no reason to cast it. This is incorrect advice: there's no need to cast it to other *pointer* types, but if you cast it to a non-pointer type (which this is doing) the compiler will complain if there is no explicit cast. James
On 13/11/2018 17:11, James Bottomley wrote: > This is incorrect advice: there's no need to cast it to other *pointer* > types, but if you cast it to a non-pointer type (which this is doing) > the compiler will complain if there is no explicit cast. Right, sorry for that and thanks for spotting it James. Byte, Johannes
On Tue, Nov 13, 2018 at 9:46 PM Johannes Thumshirn <jthumshirn@suse.de> wrote: > > On 13/11/2018 17:11, James Bottomley wrote: > > This is incorrect advice: there's no need to cast it to other *pointer* > > types, but if you cast it to a non-pointer type (which this is doing) > > the compiler will complain if there is no explicit cast. shall I leave the typecast as it is? > Right, sorry for that and thanks for spotting it James. > > Byte, > Johannes > -- > Johannes Thumshirn SUSE Labs > jthumshirn@suse.de +49 911 74053 689 > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: Felix Imendörffer, Jane Smithard, Graham Norton > HRB 21284 (AG Nürnberg) > Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
On 13/11/2018 17:22, Sabyasachi Gupta wrote: > On Tue, Nov 13, 2018 at 9:46 PM Johannes Thumshirn <jthumshirn@suse.de> wrote: >> >> On 13/11/2018 17:11, James Bottomley wrote: >>> This is incorrect advice: there's no need to cast it to other *pointer* >>> types, but if you cast it to a non-pointer type (which this is doing) >>> the compiler will complain if there is no explicit cast. > shall I leave the typecast as it is? Yes, sorry for not looking at it deeply enough.
From: James Bottomley > Sent: 13 November 2018 16:11 > > On Tue, 2018-11-13 at 16:53 +0100, Johannes Thumshirn wrote: > > On 13/11/2018 16:44, John Garry wrote: > > > On 13/11/2018 15:08, Sabyasachi Gupta wrote: > [...] > > > > - fnic_trace_buf_p = (unsigned long)vmalloc((trace_max_pages * > > > > PAGE_SIZE)); > > > > + fnic_trace_buf_p = (unsigned long)vzalloc((trace_max_pages * > > > > + PAGE_SIZE)); > > > > > > If you remove the extra brackets in vzalloc() argument then you may > > > not spill onto the next line. > > > > And remove the unnecessary cast. vzalloc() (just like vmalloc()) > > returns a void*, so no reason to cast it. > > This is incorrect advice: there's no need to cast it to other *pointer* > types, but if you cast it to a non-pointer type (which this is doing) > the compiler will complain if there is no explicit cast. The real question is why is this code using 'unsigned long' to hold pointers? Never mind why it is allocating a large memory block then setting up another array with pointers to every 64 bytes down it. Also why default to 16 * PAGE_SIZE - that is silly if pages are big. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/scsi/fnic/fnic_trace.c b/drivers/scsi/fnic/fnic_trace.c index 8271785..129ab27 100644 --- a/drivers/scsi/fnic/fnic_trace.c +++ b/drivers/scsi/fnic/fnic_trace.c @@ -468,14 +468,14 @@ int fnic_trace_buf_init(void) fnic_max_trace_entries = (trace_max_pages * PAGE_SIZE)/ FNIC_ENTRY_SIZE_BYTES; - fnic_trace_buf_p = (unsigned long)vmalloc((trace_max_pages * PAGE_SIZE)); + fnic_trace_buf_p = (unsigned long)vzalloc((trace_max_pages * + PAGE_SIZE)); if (!fnic_trace_buf_p) { printk(KERN_ERR PFX "Failed to allocate memory " "for fnic_trace_buf_p\n"); err = -ENOMEM; goto err_fnic_trace_buf_init; } - memset((void *)fnic_trace_buf_p, 0, (trace_max_pages * PAGE_SIZE)); fnic_trace_entries.page_offset = vmalloc(array_size(fnic_max_trace_entries,
Replaced vmalloc + memset with vzalloc Signed-off-by: Sabyasachi Gupta <sabyasachi.linux@gmail.com> --- drivers/scsi/fnic/fnic_trace.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)