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

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

[Sumit.Saxena]

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

Signed-off-by: Sumit Saxena <sumit.saxena@avagotech.com>
Signed-off-by: Kashyap Desai <kashyap.desai@avagotech.com>
---
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index 57b47fe..c69c1ac 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -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
 }
 
 /**
-- 
1.8.3.1

Re: [PATCH 02/11] megaraid_sas : Use writeq for 64bit pci write to avoid spinlock overhead

[Tomas Henzl]

On 09/06/2014 03:25 PM, Sumit.Saxena@avagotech.com wrote:
> Use writeq() for 64bit PCI write instead of writel() to avoid additional lock overhead.
>
> Signed-off-by: Sumit Saxena <sumit.saxena@avagotech.com>
> Signed-off-by: Kashyap Desai <kashyap.desai@avagotech.com>
> ---
>  drivers/scsi/megaraid/megaraid_sas_fusion.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index 57b47fe..c69c1ac 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -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?

>  {
> +#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

> +	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

> +	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
>  }
>  
>  /**

RE: [PATCH 02/11] megaraid_sas : Use writeq for 64bit pci write to avoid spinlock overhead

[Kashyap Desai]

> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com]
> Sent: Tuesday, September 09, 2014 7:01 PM
> To: Sumit.Saxena@avagotech.com; linux-scsi@vger.kernel.org
> Cc: martin.petersen@oracle.com; hch@infradead.org;
> jbottomley@parallels.com; kashyap.desai@avagotech.com;
> aradford@gmail.com
> Subject: Re: [PATCH 02/11] megaraid_sas : Use writeq for 64bit pci write
to
> avoid spinlock overhead
>
> On 09/06/2014 03:25 PM, Sumit.Saxena@avagotech.com wrote:
> > Use writeq() for 64bit PCI write instead of writel() to avoid
additional lock
> overhead.
> >
> > Signed-off-by: Sumit Saxena <sumit.saxena@avagotech.com>
> > Signed-off-by: Kashyap Desai <kashyap.desai@avagotech.com>
> > ---
> >  drivers/scsi/megaraid/megaraid_sas_fusion.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > index 57b47fe..c69c1ac 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > @@ -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?

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.

>
> >  {
> > +#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.

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

>
> Cheers,
> Tomas
>
> > +	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
> >  }
> >
> >  /**

Re: [PATCH 02/11] megaraid_sas : Use writeq for 64bit pci write to avoid spinlock overhead

[Tomas Henzl]

On 09/10/2014 12:15 PM, Kashyap Desai wrote:
>> -----Original Message-----
>> From: Tomas Henzl [mailto:thenzl@redhat.com]
>> Sent: Tuesday, September 09, 2014 7:01 PM
>> To: Sumit.Saxena@avagotech.com; linux-scsi@vger.kernel.org
>> Cc: martin.petersen@oracle.com; hch@infradead.org;
>> jbottomley@parallels.com; kashyap.desai@avagotech.com;
>> aradford@gmail.com
>> Subject: Re: [PATCH 02/11] megaraid_sas : Use writeq for 64bit pci write
> to
>> avoid spinlock overhead
>>
>> On 09/06/2014 03:25 PM, Sumit.Saxena@avagotech.com wrote:
>>> Use writeq() for 64bit PCI write instead of writel() to avoid
> additional lock
>> overhead.
>>> Signed-off-by: Sumit Saxena <sumit.saxena@avagotech.com>
>>> Signed-off-by: Kashyap Desai <kashyap.desai@avagotech.com>
>>> ---
>>>  drivers/scsi/megaraid/megaraid_sas_fusion.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> index 57b47fe..c69c1ac 100644
>>> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> @@ -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?
> 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.

ok

>
>>>  {
>>> +#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.

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

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

Thanks.

>
>> Cheers,
>> Tomas
>>
>>> +	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
>>>  }
>>>
>>>  /**
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html