megaraid_sas : Use writeq for 64bit pci write to avoid spinlock overhead

PATCH Superseded [email protected] 3 participants 4 messages Sep 6, 2014 – Sep 10, 2014

Subsystems (2)

SCSI Megaraid SCSI/SAS drivers

lore

Share & tools

lore

[email protected]

author

2014-09-06 13:41 UTC #0 lore raw

PATCH 2/11 megaraid_sas : Use writeq for 64bit pci write to avoid spinlock overhead

Use writeq() for 64bit PCI write instead of writel() to avoid additional lock overhead.

Signed-off-by Sumit Saxena <[email protected]>

Signed-off-by Kashyap Desai <[email protected]>

---
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/scsi/megaraid/megaraid_sas_fusion.c +8 -0

index 57b47fe..c69c1ac 100644
@@ -1065,6 +1065,13 @@ megasas_fire_cmd_fusion(struct megasas_instance *instance,
 			u32 req_desc_hi,
 			struct megasas_register_set __iomem *regs)
 {
+#if defined(writeq) && defined(CONFIG_64BIT)
+	u64 req_data = 0;
+
+	req_data = req_desc_hi;
+	req_data = ((req_data << 32) | (u32)req_desc_lo);
+	writeq(le64_to_cpu(req_data), &(regs)->inbound_low_queue_port);
+#else
 	unsigned long flags;
 
 	spin_lock_irqsave(&instance->hba_lock, flags);

@@ -1072,6 +1079,7 @@ megasas_fire_cmd_fusion(struct megasas_instance *instance,
 	writel(le32_to_cpu(req_desc_lo), &(regs)->inbound_low_queue_port);
 	writel(le32_to_cpu(req_desc_hi), &(regs)->inbound_high_queue_port);
 	spin_unlock_irqrestore(&instance->hba_lock, flags);
+#endif
 }
 
 /**

Signature · 1 line

1.8.3.1

Tomas Henzl

2014-09-09 13:31 UTC #1 lore raw

Quoted 13 lines · On 09/06/2014 03:25 PM, [email protected] wrote:

Use writeq() for 64bit PCI write instead of writel() to avoid additional lock overhead.
Signed-off-by Sumit Saxena <[email protected]>
Signed-off-by Kashyap Desai <[email protected]>
---
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/scsi/megaraid/megaraid_sas_fusion.c +0 -0
index 57b47fe..c69c1ac 100644

drivers/scsi/megaraid/megaraid_sas_fusion.c megasas_fire_cmd_fusion(struct megasas_instance *instance, · line 1066

Quoted 2 lines

drivers/scsi/megaraid/megaraid_sas_fusion.c +0 -0

@@ -1065,6 +1065,13 @@ megasas_fire_cmd_fusion(struct megasas_instance *instance,
 			u32 req_desc_hi,
 			struct megasas_register_set __iomem *regs)

Hi Sumit,
the fn params are a bit confusing req_desc_lo is of type dma_addr_t and req_desc_hi is u32, is it possible to unite it in the future?

drivers/scsi/megaraid/megaraid_sas_fusion.c megasas_fire_cmd_fusion(struct megasas_instance *instance, · line 1068

Quoted 2 lines

drivers/scsi/megaraid/megaraid_sas_fusion.c +1 -0
 {
+#if defined(writeq) && defined(CONFIG_64BIT)

On a similar place mpt2sas(_base_writeq) uses only "#ifndef writeq" if it's incorrect fix it there too or remove the CONFIG_64 here

drivers/scsi/megaraid/megaraid_sas_fusion.c megasas_fire_cmd_fusion(struct megasas_instance *instance, · line 1072

Quoted 4 lines

drivers/scsi/megaraid/megaraid_sas_fusion.c +4 -0

+	u64 req_data = 0;
+
+	req_data = req_desc_hi;
+	req_data = ((req_data << 32) | (u32)req_desc_lo);

This seems to be critical path (you are removing an spinlock to avoid overhead), so why do you have three consecutive assignments to the same variable? (~(u64 req_data = r_hi << 32 | r_lo))

Cheers,
Tomas

Quoted 13 lines

drivers/scsi/megaraid/megaraid_sas_fusion.c +3 -0

+	writeq(le64_to_cpu(req_data), &(regs)->inbound_low_queue_port);
+#else
 	unsigned long flags;
 
 	spin_lock_irqsave(&instance->hba_lock, flags);

@@ -1072,6 +1079,7 @@ megasas_fire_cmd_fusion(struct megasas_instance *instance,
 	writel(le32_to_cpu(req_desc_lo), &(regs)->inbound_low_queue_port);
 	writel(le32_to_cpu(req_desc_hi), &(regs)->inbound_high_queue_port);
 	spin_unlock_irqrestore(&instance->hba_lock, flags);
+#endif
 }
 
 /**

Kashyap Desai

2014-09-10 10:15 UTC #2 lore raw

Quoted 8 lines

-----Original Message-----
From: Tomas Henzl [mailto:[email protected]]
Sent: Tuesday, September 09, 2014 7:01 PM
To: [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 02/11] megaraid_sas : Use writeq for 64bit pci write

Quoted 4 lines

avoid spinlock overhead
Quoted 1 line · On 09/06/2014 03:25 PM, [email protected] wrote:
Use writeq() for 64bit PCI write instead of writel() to avoid

additional lock

Quoted 15 lines

overhead.
drivers/scsi/megaraid/megaraid_sas_fusion.c megasas_fire_cmd_fusion(struct
Quoted 13 lines
Signed-off-by Sumit Saxena <[email protected]>
Signed-off-by Kashyap Desai <[email protected]>
---
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/scsi/megaraid/megaraid_sas_fusion.c +0 -0
index 57b47fe..c69c1ac 100644
@@ -1065,6 +1065,13 @@ megasas_fire_cmd_fusion(struct
megasas_instance *instance,

Quoted 6 lines

drivers/scsi/megaraid/megaraid_sas_fusion.c megasas_fire_cmd_fusion(struct megasas_instance *instance, · line 1066
Quoted 2 lines
drivers/scsi/megaraid/megaraid_sas_fusion.c +0 -0
 			u32 req_desc_hi,
 			struct megasas_register_set __iomem *regs)
Hi Sumit,
the fn params are a bit confusing req_desc_lo is of type dma_addr_t and req_desc_hi is u32, is it possible to unite it in the future?

Agree. We should make changes here. We will do it in separate patch. Originally fire_cmd() was written for MFI controller and carry forward for all generation.
In MFI it use second argument as 32 bit address and third argument as frame count, but later in Fusion adapter it started using differently.

Quoted 6 lines

drivers/scsi/megaraid/megaraid_sas_fusion.c megasas_fire_cmd_fusion(struct megasas_instance *instance, · line 1068
Quoted 3 lines
drivers/scsi/megaraid/megaraid_sas_fusion.c +1 -0
 {
+#if defined(writeq) && defined(CONFIG_64BIT)
On a similar place mpt2sas(_base_writeq) uses only "#ifndef writeq" if it's incorrect fix it there too or remove the CONFIG_64 here

We would like to change at mpt2sas as we have all the code with below check for writeq()
Original discuss was started when we submitted this change in mpt2sas, but we have delta from day-1.
LSI/Avago internal source has "#if defined(writeq) && defined(CONFIG_64BIT)" check in mpt2sas.

I think now writeq() is implemented in all arch, so we can safely remove check for #if writeq().
But we can keep this check as it is to continue for older Distribution to take direct advantage without maintaining any separate patch.

Quoted 10 lines

drivers/scsi/megaraid/megaraid_sas_fusion.c megasas_fire_cmd_fusion(struct megasas_instance *instance, · line 1072
Quoted 5 lines
drivers/scsi/megaraid/megaraid_sas_fusion.c +4 -0
+	u64 req_data = 0;
+
+	req_data = req_desc_hi;
+	req_data = ((req_data << 32) | (u32)req_desc_lo);
This seems to be critical path (you are removing an spinlock to avoid overhead), so why do you have three consecutive assignments to the same variable?
(~(u64 req_data = r_hi << 32 | r_lo))

Agree. We will be doing this change and re-submit the patch to address this.

Quoted 21 lines

Cheers,
Tomas
Quoted 6 lines
drivers/scsi/megaraid/megaraid_sas_fusion.c +2 -0
+	writeq(le64_to_cpu(req_data), &(regs)-
 inbound_low_queue_port);
+#else
 	unsigned long flags;
 
spin_lock_irqsave(&instance->hba_lock, flags); @@ -1072,6 +1079,7
@@
Quoted 10 lines
megasas_fire_cmd_fusion(struct megasas_instance *instance,
writel(le32_to_cpu(req_desc_lo), &(regs)-
inbound_low_queue_port);
writel(le32_to_cpu(req_desc_hi), &(regs)-
inbound_high_queue_port);
drivers/scsi/megaraid/megaraid_sas_fusion.c +1 -0
 	spin_unlock_irqrestore(&instance->hba_lock, flags);
+#endif
 }
 
 /**

Tomas Henzl

2014-09-10 11:18 UTC #3 lore raw

Quoted 30 lines · Kashyap Desai

Quoted 8 lines
-----Original Message-----
From: Tomas Henzl [mailto:[email protected]]
Sent: Tuesday, September 09, 2014 7:01 PM
To: [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 02/11] megaraid_sas : Use writeq for 64bit pci write
to
Quoted 4 lines
avoid spinlock overhead
Quoted 1 line · On 09/06/2014 03:25 PM, [email protected] wrote:
Use writeq() for 64bit PCI write instead of writel() to avoid
additional lock
Quoted 16 lines
overhead.
drivers/scsi/megaraid/megaraid_sas_fusion.c megasas_fire_cmd_fusion(struct
Quoted 12 lines
Signed-off-by Sumit Saxena <[email protected]>
Signed-off-by Kashyap Desai <[email protected]>
---
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/scsi/megaraid/megaraid_sas_fusion.c +0 -0
index 57b47fe..c69c1ac 100644
@@ -1065,6 +1065,13 @@ megasas_fire_cmd_fusion(struct
megasas_instance *instance,
Quoted 2 lines
drivers/scsi/megaraid/megaraid_sas_fusion.c +0 -0
 			u32 req_desc_hi,
 			struct megasas_register_set __iomem *regs)

Quoted 8 lines

Quoted 3 lines
Hi Sumit,
the fn params are a bit confusing req_desc_lo is of type dma_addr_t and req_desc_hi is u32, is it possible to unite it in the future?
Agree. We should make changes here. We will do it in separate patch. Originally fire_cmd() was written for MFI controller and carry forward for all generation.
In MFI it use second argument as 32 bit address and third argument as frame count, but later in Fusion adapter it started using differently.

Quoted 12 lines

Quoted 5 lines
drivers/scsi/megaraid/megaraid_sas_fusion.c megasas_fire_cmd_fusion(struct megasas_instance *instance, · line 1068
Quoted 3 lines
drivers/scsi/megaraid/megaraid_sas_fusion.c +1 -0
 {
+#if defined(writeq) && defined(CONFIG_64BIT)
On a similar place mpt2sas(_base_writeq) uses only "#ifndef writeq" if it's incorrect fix it there too or remove the CONFIG_64 here
We would like to change at mpt2sas as we have all the code with below check for writeq()
Original discuss was started when we submitted this change in mpt2sas, but we have delta from day-1.
LSI/Avago internal source has "#if defined(writeq) && defined(CONFIG_64BIT)" check in mpt2sas.

Quoted 4 lines

I think now writeq() is implemented in all arch, so we can safely remove check for #if writeq().
But we can keep this check as it is to continue for older Distribution to take direct advantage without maintaining any separate patch.

I don't know which combination of writeq and config_64bit is the right way, I was hoping that someone who knows will help with. (I'll accept almost any combination you'll post...)

Quoted 10 lines

Quoted 9 lines
drivers/scsi/megaraid/megaraid_sas_fusion.c megasas_fire_cmd_fusion(struct megasas_instance *instance, · line 1072
Quoted 5 lines
drivers/scsi/megaraid/megaraid_sas_fusion.c +4 -0
+	u64 req_data = 0;
+
+	req_data = req_desc_hi;
+	req_data = ((req_data << 32) | (u32)req_desc_lo);
This seems to be critical path (you are removing an spinlock to avoid overhead), so why do you have three consecutive assignments to the same variable?
(~(u64 req_data = r_hi << 32 | r_lo))

Quoted 2 lines

Agree. We will be doing this change and re-submit the patch to address this.

Thanks.

Quoted 25 lines

Quoted 4 lines
Cheers,
Tomas
Quoted 17 lines
Quoted 6 lines
drivers/scsi/megaraid/megaraid_sas_fusion.c +2 -0
+	writeq(le64_to_cpu(req_data), &(regs)-
 inbound_low_queue_port);
+#else
 	unsigned long flags;
 
spin_lock_irqsave(&instance->hba_lock, flags); @@ -1072,6 +1079,7
@@
Quoted 10 lines
megasas_fire_cmd_fusion(struct megasas_instance *instance,
writel(le32_to_cpu(req_desc_lo), &(regs)-
inbound_low_queue_port);
writel(le32_to_cpu(req_desc_hi), &(regs)-
inbound_high_queue_port);
drivers/scsi/megaraid/megaraid_sas_fusion.c +1 -0
 	spin_unlock_irqrestore(&instance->hba_lock, flags);
+#endif
 }
 
 /**
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html