Re: [PATCH 09/12] hpsa: separate monitor events from heartbeat worker

2017-04-28 Thread Martin Wilck 
On Thu, 2017-04-27 at 21:10 +, Don Brace wrote:
> > -
> > The new worker thread duplicates code from hpsa_rescan_ctlr_worker.
> > I
> > find this a bit irritating. Could you maybe use just a single
> > worker,
> > and just check using time stamps whether the "big" heartbeat needs
> > to
> > be performed?
> > 
> > Regards
> > Martin
> > 
> > --
> > Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
> > SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham
> > Norton
> > HRB 21284 (AG Nürnberg)
> 
> We thought about that, but we want to separate controller events
> from the rescan worker.
> 
> Both can cause a rescan to occur however for multipath we have
> found that we need to respond faster than the normal scheduled rescan
> interval for path fail-overs.
> 
> Getting controller events only involves reading a register, but
> the rescan worker can obtain an updated LUN list when there
> is a PTRAID device present.
> 
> However, I did refactor the patch to move common code to
> a separate function.
> 
> Would this be more acceptable?

Sounds good, yes. I'd also appreciate if you'd add these additional
comments to the commit message.

Regards
Martin

-- 
Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

RE: [PATCH 09/12] hpsa: separate monitor events from heartbeat worker

2017-04-27 Thread Don Brace 
> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of Martin Wilck
> Sent: Tuesday, April 11, 2017 7:19 AM
> To: Don Brace <don.br...@microsemi.com>; joseph.szczy...@hpe.com;
> Gerry Morong <gerry.mor...@microsemi.com>; John Hall
> <john.h...@microsemi.com>; j...@linux.vnet.ibm.com; Kevin Barnett
> <kevin.barn...@microsemi.com>; Mahesh Rajashekhara
> <mahesh.rajashekh...@microsemi.com>; Bader Ali - Saleh
> <bader.alisa...@microsemi.com>; h...@infradead.org; Scott Teel
> <scott.t...@microsemi.com>; Viswas G <viswa...@microsemi.com>; Justin
> Lindley <justin.lind...@microsemi.com>; Scott Benesh
> <scott.ben...@microsemi.com>; posw...@suse.com
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 09/12] hpsa: separate monitor events from heartbeat
> worker
> 
> > +/*
> > + * watch for controller events
> > + */
> > +static void hpsa_event_monitor_worker(struct work_struct *work)
> > +{
> > + struct ctlr_info *h = container_of(to_delayed_work(work),
> > + struct ctlr_info, event_monitor_work);
> > +
> > + if (h->remove_in_progress)
> > + return;
> > +
> > + if (hpsa_ctlr_needs_rescan(h)) {
> > + scsi_host_get(h->scsi_host);
> > + hpsa_ack_ctlr_events(h);
> > + hpsa_scan_start(h->scsi_host);
> > + scsi_host_put(h->scsi_host);
> > + }
> > +
> > + if (!h->remove_in_progress)
> > + schedule_delayed_work(>event_monitor_work,
> > + HPSA_EVENT_MONITOR_INTERVAL)
> > ;
> > +}
> > +
> 
> The new worker thread duplicates code from hpsa_rescan_ctlr_worker. I
> find this a bit irritating. Could you maybe use just a single worker,
> and just check using time stamps whether the "big" heartbeat needs to
> be performed?
> 
> Regards
> Martin
> 
> --
> Dr. Martin Wilck <mwi...@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)

We thought about that, but we want to separate controller events
from the rescan worker.

Both can cause a rescan to occur however for multipath we have
found that we need to respond faster than the normal scheduled rescan
interval for path fail-overs.

Getting controller events only involves reading a register, but
the rescan worker can obtain an updated LUN list when there
is a PTRAID device present.

However, I did refactor the patch to move common code to
a separate function.

Would this be more acceptable?

Thanks,
Don Brace
ESC - Smart Storage
Microsemi Corporation

Re: [PATCH 09/12] hpsa: separate monitor events from heartbeat worker

2017-04-11 Thread Martin Wilck 
On Fri, 2017-04-07 at 15:06 -0500, Don Brace wrote:
> From: Scott Teel 
> 
> create new worker thread to monitor controller events
>  - detect controller events more frequently.
>  - leave heartbeat check at 30 seconds.
> 
> Reviewed-by: Scott Benesh 
> Reviewed-by: Scott Teel 
> Reviewed-by: Kevin Barnett 
> Signed-off-by: Don Brace 
> ---
>  drivers/scsi/hpsa.c |   32 ++--
>  drivers/scsi/hpsa.h |1 +
>  2 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 40a87f9..50f7c09 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -1110,6 +1110,7 @@ static int is_firmware_flash_cmd(u8 *cdb)
>   */
>  #define HEARTBEAT_SAMPLE_INTERVAL_DURING_FLASH (240 * HZ)
>  #define HEARTBEAT_SAMPLE_INTERVAL (30 * HZ)
> +#define HPSA_EVENT_MONITOR_INTERVAL (15 * HZ)
>  static void dial_down_lockup_detection_during_fw_flash(struct
> ctlr_info *h,
>   struct CommandList *c)
>  {
> @@ -8650,6 +8651,29 @@ static int hpsa_luns_changed(struct ctlr_info
> *h)
>   return rc;
>  }
>  
> +/*
> + * watch for controller events
> + */
> +static void hpsa_event_monitor_worker(struct work_struct *work)
> +{
> + struct ctlr_info *h = container_of(to_delayed_work(work),
> + struct ctlr_info, event_monitor_work);
> +
> + if (h->remove_in_progress)
> + return;
> +
> + if (hpsa_ctlr_needs_rescan(h)) {
> + scsi_host_get(h->scsi_host);
> + hpsa_ack_ctlr_events(h);
> + hpsa_scan_start(h->scsi_host);
> + scsi_host_put(h->scsi_host);
> + }
> +
> + if (!h->remove_in_progress)
> + schedule_delayed_work(>event_monitor_work,
> + HPSA_EVENT_MONITOR_INTERVAL)
> ;
> +}
> +
>  static void hpsa_rescan_ctlr_worker(struct work_struct *work)
>  {
>   unsigned long flags;
> @@ -8668,9 +8692,9 @@ static void hpsa_rescan_ctlr_worker(struct
> work_struct *work)
>   return;
>   }
>  
> - if (hpsa_ctlr_needs_rescan(h) ||
> hpsa_offline_devices_ready(h)) {
> + if (h->drv_req_rescan || hpsa_offline_devices_ready(h)) {
> + h->drv_req_rescan = 0;
>   scsi_host_get(h->scsi_host);
> - hpsa_ack_ctlr_events(h);
>   hpsa_scan_start(h->scsi_host);
>   scsi_host_put(h->scsi_host);
>   } else if (h->discovery_polling) {
> @@ -8949,6 +8973,9 @@ static int hpsa_init_one(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>   INIT_DELAYED_WORK(>rescan_ctlr_work,
> hpsa_rescan_ctlr_worker);
>   queue_delayed_work(h->rescan_ctlr_wq, >rescan_ctlr_work,
>   h->heartbeat_sample_interval);
> + INIT_DELAYED_WORK(>event_monitor_work,
> hpsa_event_monitor_worker);
> + schedule_delayed_work(>event_monitor_work,
> + HPSA_EVENT_MONITOR_INTERVAL);
>   return 0;
>  
>  clean7: /* perf, sg, cmd, irq, shost, pci, lu, aer/h */
> @@ -9117,6 +9144,7 @@ static void hpsa_remove_one(struct pci_dev
> *pdev)
>   spin_unlock_irqrestore(>lock, flags);
>   cancel_delayed_work_sync(>monitor_ctlr_work);
>   cancel_delayed_work_sync(>rescan_ctlr_work);
> + cancel_delayed_work_sync(>event_monitor_work);
>   destroy_workqueue(h->rescan_ctlr_wq);
>   destroy_workqueue(h->resubmit_wq);
>  
> diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
> index 99539c0..3c22ac1 100644
> --- a/drivers/scsi/hpsa.h
> +++ b/drivers/scsi/hpsa.h
> @@ -245,6 +245,7 @@ struct ctlr_info {
>   u32 __percpu *lockup_detected;
>   struct delayed_work monitor_ctlr_work;
>   struct delayed_work rescan_ctlr_work;
> + struct delayed_work event_monitor_work;
>   int remove_in_progress;
>   /* Address of h->q[x] is passed to intr handler to know
> which queue */
>   u8 q[MAX_REPLY_QUEUES];

The new worker thread duplicates code from hpsa_rescan_ctlr_worker. I
find this a bit irritating. Could you maybe use just a single worker,
and just check using time stamps whether the "big" heartbeat needs to
be performed?

Regards
Martin

-- 
Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[PATCH 09/12] hpsa: separate monitor events from heartbeat worker

2017-04-07 Thread Don Brace 
From: Scott Teel 

create new worker thread to monitor controller events
 - detect controller events more frequently.
 - leave heartbeat check at 30 seconds.

Reviewed-by: Scott Benesh 
Reviewed-by: Scott Teel 
Reviewed-by: Kevin Barnett 
Signed-off-by: Don Brace 
---
 drivers/scsi/hpsa.c |   32 ++--
 drivers/scsi/hpsa.h |1 +
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 40a87f9..50f7c09 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -1110,6 +1110,7 @@ static int is_firmware_flash_cmd(u8 *cdb)
  */
 #define HEARTBEAT_SAMPLE_INTERVAL_DURING_FLASH (240 * HZ)
 #define HEARTBEAT_SAMPLE_INTERVAL (30 * HZ)
+#define HPSA_EVENT_MONITOR_INTERVAL (15 * HZ)
 static void dial_down_lockup_detection_during_fw_flash(struct ctlr_info *h,
struct CommandList *c)
 {
@@ -8650,6 +8651,29 @@ static int hpsa_luns_changed(struct ctlr_info *h)
return rc;
 }
 
+/*
+ * watch for controller events
+ */
+static void hpsa_event_monitor_worker(struct work_struct *work)
+{
+   struct ctlr_info *h = container_of(to_delayed_work(work),
+   struct ctlr_info, event_monitor_work);
+
+   if (h->remove_in_progress)
+   return;
+
+   if (hpsa_ctlr_needs_rescan(h)) {
+   scsi_host_get(h->scsi_host);
+   hpsa_ack_ctlr_events(h);
+   hpsa_scan_start(h->scsi_host);
+   scsi_host_put(h->scsi_host);
+   }
+
+   if (!h->remove_in_progress)
+   schedule_delayed_work(>event_monitor_work,
+   HPSA_EVENT_MONITOR_INTERVAL);
+}
+
 static void hpsa_rescan_ctlr_worker(struct work_struct *work)
 {
unsigned long flags;
@@ -8668,9 +8692,9 @@ static void hpsa_rescan_ctlr_worker(struct work_struct 
*work)
return;
}
 
-   if (hpsa_ctlr_needs_rescan(h) || hpsa_offline_devices_ready(h)) {
+   if (h->drv_req_rescan || hpsa_offline_devices_ready(h)) {
+   h->drv_req_rescan = 0;
scsi_host_get(h->scsi_host);
-   hpsa_ack_ctlr_events(h);
hpsa_scan_start(h->scsi_host);
scsi_host_put(h->scsi_host);
} else if (h->discovery_polling) {
@@ -8949,6 +8973,9 @@ static int hpsa_init_one(struct pci_dev *pdev, const 
struct pci_device_id *ent)
INIT_DELAYED_WORK(>rescan_ctlr_work, hpsa_rescan_ctlr_worker);
queue_delayed_work(h->rescan_ctlr_wq, >rescan_ctlr_work,
h->heartbeat_sample_interval);
+   INIT_DELAYED_WORK(>event_monitor_work, hpsa_event_monitor_worker);
+   schedule_delayed_work(>event_monitor_work,
+   HPSA_EVENT_MONITOR_INTERVAL);
return 0;
 
 clean7: /* perf, sg, cmd, irq, shost, pci, lu, aer/h */
@@ -9117,6 +9144,7 @@ static void hpsa_remove_one(struct pci_dev *pdev)
spin_unlock_irqrestore(>lock, flags);
cancel_delayed_work_sync(>monitor_ctlr_work);
cancel_delayed_work_sync(>rescan_ctlr_work);
+   cancel_delayed_work_sync(>event_monitor_work);
destroy_workqueue(h->rescan_ctlr_wq);
destroy_workqueue(h->resubmit_wq);
 
diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index 99539c0..3c22ac1 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -245,6 +245,7 @@ struct ctlr_info {
u32 __percpu *lockup_detected;
struct delayed_work monitor_ctlr_work;
struct delayed_work rescan_ctlr_work;
+   struct delayed_work event_monitor_work;
int remove_in_progress;
/* Address of h->q[x] is passed to intr handler to know which queue */
u8 q[MAX_REPLY_QUEUES];