drivers/scsi/fnic/fnic_trace.c: Use vzalloc

PATCH Superseded Sabyasachi Gupta 5 participants 9 messages Nov 13, 2018 – Nov 14, 2018

Subsystems (2)

SCSI Cisco FCoE HBA driver

lore

Share & tools

lore

Sabyasachi Gupta

author

2018-11-13 15:08 UTC #0 lore raw

PATCH 1/1 drivers/scsi/fnic/fnic_trace.c: Use vzalloc

Replaced vmalloc + memset with vzalloc

Signed-off-by Sabyasachi Gupta <[email protected]>

---
 drivers/scsi/fnic/fnic_trace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/scsi/fnic/fnic_trace.c +2 -2

index 8271785..129ab27 100644
@@ -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,

Signature · 1 line

2.7.4

John Garry

2018-11-13 15:44 UTC #1 lore raw

Quoted 12 lines · Sabyasachi Gupta

Replaced vmalloc + memset with vzalloc
Signed-off-by Sabyasachi Gupta <[email protected]>
---
 drivers/scsi/fnic/fnic_trace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/fnic/fnic_trace.c +0 -0
index 8271785..129ab27 100644

drivers/scsi/fnic/fnic_trace.c int fnic_trace_buf_init(void) · line 472

Quoted 6 lines

drivers/scsi/fnic/fnic_trace.c +2 -1

@@ -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.

Quoted 11 lines

drivers/scsi/fnic/fnic_trace.c +0 -1

 	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,

Johannes Thumshirn

2018-11-13 15:53 UTC #2 lore raw

Quoted 17 lines · John Garry

Quoted 16 lines · Sabyasachi Gupta
Replaced vmalloc + memset with vzalloc
Signed-off-by Sabyasachi Gupta <[email protected]>
---
drivers/scsi/fnic/fnic_trace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/fnic/fnic_trace.c +0 -0
index 8271785..129ab27 100644
@@ -468,14 +468,14 @@ int fnic_trace_buf_init(void)
fnic_max_trace_entries = (trace_max_pages * PAGE_SIZE)/ FNIC_ENTRY_SIZE_BYTES;

Quoted 7 lines

Quoted 4 lines
- 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.

Signature · 6 lines

Johannes Thumshirn                                        SUSE Labs
[email protected]                                +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

Sabyasachi Gupta

author

2018-11-13 16:04 UTC #3 lore raw

Quoted 30 lines · Johannes Thumshirn

Quoted 17 lines · John Garry
Quoted 16 lines · Sabyasachi Gupta
Replaced vmalloc + memset with vzalloc
Signed-off-by Sabyasachi Gupta <[email protected]>
---
 drivers/scsi/fnic/fnic_trace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/fnic/fnic_trace.c +0 -0
index 8271785..129ab27 100644
@@ -468,14 +468,14 @@ int fnic_trace_buf_init(void)
     fnic_max_trace_entries = (trace_max_pages * PAGE_SIZE)/
                       FNIC_ENTRY_SIZE_BYTES;
 
Quoted 7 lines
drivers/scsi/fnic/fnic_trace.c int fnic_trace_buf_init(void) · line 472
Quoted 4 lines
drivers/scsi/fnic/fnic_trace.c +2 -1
-    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.

Quoted 7 lines

--
Johannes Thumshirn SUSE Labs
[email protected] +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

James Bottomley

2018-11-13 16:11 UTC #4 lore raw

In reply to Johannes Thumshirn

Quoted 2 lines · Johannes Thumshirn

Quoted 1 line · John Garry
On 13/11/2018 15:08, Sabyasachi Gupta wrote:

[...]

Quoted 10 lines

Quoted 7 lines
drivers/scsi/fnic/fnic_trace.c int fnic_trace_buf_init(void) · line 472
Quoted 4 lines
- fnic_trace_buf_p = (unsigned long)vmalloc((trace_max_pages *
PAGE_SIZE));
drivers/scsi/fnic/fnic_trace.c +2 -0
+    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

Johannes Thumshirn

2018-11-13 16:16 UTC #5 lore raw

Quoted 3 lines · James Bottomley

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

Signature · 6 lines

Johannes Thumshirn                                        SUSE Labs
[email protected]                                +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

Sabyasachi Gupta

author

2018-11-13 16:22 UTC #6 lore raw

Quoted 5 lines · Johannes Thumshirn

Quoted 3 lines · James Bottomley
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?

Quoted 11 lines

Right, sorry for that and thanks for spotting it James.
Byte,
Johannes
--
Johannes Thumshirn SUSE Labs
[email protected] +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

Johannes Thumshirn

2018-11-13 16:25 UTC #7 lore raw

Quoted 7 lines · Sabyasachi Gupta

Quoted 5 lines · Johannes Thumshirn
Quoted 3 lines · James Bottomley
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.

Signature · 6 lines

Johannes Thumshirn                                        SUSE Labs
[email protected]                                +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

David Laight

2018-11-14 10:44 UTC #8 lore raw

In reply to James Bottomley

From: James Bottomley

Quoted 14 lines

Sent: 13 November 2018 16:11
Quoted 2 lines · Johannes Thumshirn
Quoted 1 line · John Garry
On 13/11/2018 15:08, Sabyasachi Gupta wrote:
[...]
Quoted 8 lines
Quoted 7 lines
drivers/scsi/fnic/fnic_trace.c int fnic_trace_buf_init(void) · line 472
Quoted 4 lines
- fnic_trace_buf_p = (unsigned long)vmalloc((trace_max_pages *
PAGE_SIZE));
drivers/scsi/fnic/fnic_trace.c +2 -0
+    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.

Quoted 6 lines

Quoted 2 lines
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)