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

2014-09-10 Thread Kashyap Desai 
 -Original Message-
 From: Tomas Henzl [mailto:the...@redhat.com]
 Sent: Tuesday, September 09, 2014 7:01 PM
 To: sumit.sax...@avagotech.com; linux-scsi@vger.kernel.org
 Cc: martin.peter...@oracle.com; h...@infradead.org;
 jbottom...@parallels.com; kashyap.de...@avagotech.com;
 aradf...@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.sax...@avagotech.com wrote:
  Use writeq() for 64bit PCI write instead of writel() to avoid
additional lock
 overhead.
 
  Signed-off-by: Sumit Saxena sumit.sax...@avagotech.com
  Signed-off-by: Kashyap Desai kashyap.de...@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
   }
 
   /**
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-09-10 Thread Tomas Henzl 
On 09/10/2014 12:15 PM, Kashyap Desai wrote:
 -Original Message-
 From: Tomas Henzl [mailto:the...@redhat.com]
 Sent: Tuesday, September 09, 2014 7:01 PM
 To: sumit.sax...@avagotech.com; linux-scsi@vger.kernel.org
 Cc: martin.peter...@oracle.com; h...@infradead.org;
 jbottom...@parallels.com; kashyap.de...@avagotech.com;
 aradf...@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.sax...@avagotech.com wrote:
 Use writeq() for 64bit PCI write instead of writel() to avoid
 additional lock
 overhead.
 Signed-off-by: Sumit Saxena sumit.sax...@avagotech.com
 Signed-off-by: Kashyap Desai kashyap.de...@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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-09-09 Thread Tomas Henzl 
On 09/06/2014 03:25 PM, sumit.sax...@avagotech.com wrote:
 Use writeq() for 64bit PCI write instead of writel() to avoid additional lock 
 overhead.

 Signed-off-by: Sumit Saxena sumit.sax...@avagotech.com
 Signed-off-by: Kashyap Desai kashyap.de...@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
  }
  
  /**

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html