Hi Brian,
Thanks for your quick response!
>>Should we perhaps use add_timer_on so the timer fires on the same core as
the >>HRRQ is
>>bound to?
Actually the original pure_poll prototype has add_timer_on but it is
commented out. I will try it.
Thanks,
Wendy
From: Brian King <brk...@linux.vnet.ibm.com>
To: wenxi...@linux.vnet.ibm.com, iprdd-devel@lists.sourceforge.net
Date: 11/17/2015 05:31 PM
Subject: Re: [Iprdd-devel] [PATCH 3/3] Add interrupt pure poller support
A few quick comments below...
On 11/17/2015 11:20 AM, wenxi...@linux.vnet.ibm.com wrote:
> From: Wen Xiong <wenxi...@linux.vnet.ibm.com>
>
> ---
> drivers/scsi/ipr.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++
+++++-
> drivers/scsi/ipr.h | 3 +
> 2 files changed, 159 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
> index 1248d27..3ae9ead 100644
> --- a/drivers/scsi/ipr.c
> +++ b/drivers/scsi/ipr.c
> @@ -102,6 +102,8 @@ static unsigned int ipr_number_of_msix = 2;
> static unsigned int ipr_fast_reboot;
> static unsigned int ipr_cpu_map = 0;
> static unsigned int ipr_use_blk_mq = 0;
> +static unsigned int ipr_poll_mode = 0;
> +static unsigned int ipr_poll_timer = 10;
>
> static unsigned int *ipr_used_cpu;
> static unsigned int ipr_possible_cpu_cnt;
> @@ -234,7 +236,10 @@ module_param_named(cpu_map, ipr_cpu_map, int, 0);
> MODULE_PARM_DESC(cpu_map, "Defines how to map CPUs to IRQ vectors per
adapter");
> module_param_named(use_blk_mq, ipr_use_blk_mq, uint, S_IRUGO);
> MODULE_PARM_DESC(use_blk_mq, "ipr use block mq enable/disable. (default:
disable(0))");
> -
> +module_param_named(poll_mode, ipr_poll_mode, uint, S_IRUGO);
> +MODULE_PARM_DESC(poll_mode, "ipr polling mode enable/disable. (default:
disable(0))");
> +module_param_named(poll_timer, ipr_poll_timer, uint, S_IRUGO);
> +MODULE_PARM_DESC(poll_timer, "Milliseconds between polling. (default:
10)");
> MODULE_LICENSE("GPL");
> MODULE_VERSION(IPR_DRIVER_VERSION);
>
> @@ -3749,6 +3754,69 @@ static struct device_attribute
ipr_iopoll_weight_attr = {
> };
>
> /**
> + * ipr_show_poll_mode - Show ipr polling mode
> + * @dev: class device struct
> + * @buf: buffer
> + *
> + * Return value:
> + * number of bytes printed to buffer
> + **/
> +static ssize_t ipr_show_poll_mode(struct device *dev,
> + struct
> device_attribute
*attr, char *buf)
> +{
> + struct Scsi_Host *shost = class_to_shost(dev);
> + struct ipr_ioa_cfg *ioa_cfg = (struct ipr_ioa_cfg *)shost->
hostdata;
> + unsigned long lock_flags = 0;
> + int len;
> +
> + spin_lock_irqsave(ioa_cfg->host->host_lock, lock_flags);
> + len = snprintf(buf, PAGE_SIZE, "%d\n", ioa_cfg->poll_mode);
> + spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags);
> + return len;
> +}
> +
> +static void ipr_disable_hrrq_interrupt(struct ipr_ioa_cfg *ioa_cfg);
> +/**
> + * ipr_store_poll_mode - Change the adapter's polling mode
> + * @dev: class device struct
> + * @buf: buffer
> + *
> + * Return value:
> + * number of bytes printed to buffer
> + **/
> +static ssize_t ipr_store_poll_mode(struct device *dev,
> + struct
> device_attribute
*attr,
> + const char
> *buf, size_t
count)
> +{
> + struct Scsi_Host *shost = class_to_shost(dev);
> + struct ipr_ioa_cfg *ioa_cfg = (struct ipr_ioa_cfg *)shost->
hostdata;
> + unsigned long lock_flags;
> + int i;
> +
> + if (kstrtoul(buf, 10, &ioa_cfg->poll_mode))
> + return -EINVAL;
> +
> + if (ioa_cfg->poll_mode && ioa_cfg->sis64 && ioa_cfg->nvectors
> 1) {
> + spin_lock_irqsave(ioa_cfg->host->host_lock,
lock_flags);
> + ipr_disable_hrrq_interrupt(ioa_cfg);
> +
> spin_unlock_irqrestore(ioa_cfg->host->host_lock,
lock_flags);
> + for (i = 0; i < IPR_MAX_HRRQ_NUM; i++)
> + add_timer(&ioa_cfg->hrrq
[i].hrrq_poll_timer);
Should we perhaps use add_timer_on so the timer fires on the same core as
the HRRQ is
bound to?
> + }
> +
> + return strlen(buf);
> +}
> +
> +static struct device_attribute ipr_poll_mode_attr = {
> + .attr = {
> + .name = "poll_mode",
> + .mode = S_IRUGO | S_IWUSR,
> + },
> + .show = ipr_show_poll_mode,
> + .store = ipr_store_poll_mode
> +};
> +
> +/**
> * ipr_alloc_ucode_buffer - Allocates a microcode download buffer
> * @buf_len: buffer length
> *
> @@ -4190,6 +4258,7 @@ static struct device_attribute *ipr_ioa_attrs[] = {
> &ipr_ioa_fw_type_attr,
> &ipr_iopoll_weight_attr,
> &ipr_cpu_map_attr,
> + &ipr_poll_mode_attr,
> NULL,
> };
>
> @@ -5597,6 +5666,47 @@ static irqreturn_t ipr_handle_other_interrupt
(struct ipr_ioa_cfg *ioa_cfg,
> }
>
> /**
> + * ipr_disable_hrrq_interrupt - Disable HRRQ interrupt
> + * @ioa_cfg pointer to ioa config struct
> + *
> +**/
> +static void ipr_disable_hrrq_interrupt(struct ipr_ioa_cfg *ioa_cfg)
> +{
> + u32 int_reg;
> +
> + /* Mask HRRQ interrupt */
> + if (ioa_cfg->sis64)
> + writeq(IPR_PCII_HRRQ_UPDATED,
> +
> ioa_cfg->regs.set_interrupt_mask_reg);
Seems like a bug if we call this function for SIS-32. I'm find not
supporting polling
mode for SIS-32, but let's remove this if check and just comment that this
is SIS-64 only.
> +
> + /* MMIO read to flush the write */
> + int_reg = readl(ioa_cfg->regs.sense_interrupt_reg32);
> +}
> +
> +/**
> + * ipr_enable_hrrq_interrupt - Enable HRRQ interrupt
> + * @ioa_cfg pointer to ioa config struct
> + *
> + **/
> +static void ipr_enable_hrrq_interrupt(struct ipr_ioa_cfg *ioa_cfg)
> +{
> + u32 int_reg;
> + int i;
> +
> + for (i = 0; i < IPR_MAX_HRRQ_NUM; i++)
> + if (!ioa_cfg->hrrq[i].allow_interrupts)
> + return;
> +
> + /* Enable HRRQ interrupt */
> + if (ioa_cfg->sis64)
> + writeq(IPR_PCII_HRRQ_UPDATED,
> +
> ioa_cfg->regs.clr_interrupt_mask_reg);
Same comment here as above.
> +
> + /* MMIO read to flush the write */
> + int_reg = readl(ioa_cfg->regs.sense_interrupt_reg32);
> +}
> +
> +/**
> * ipr_isr_eh - Interrupt service routine error handler
> * @ioa_cfg: ioa config struct
> * @msg: message to log
> @@ -5692,6 +5802,29 @@ static int ipr_iopoll(struct blk_iopoll *iop, int
budget)
> return completed_ops;
> }
>
> +void ipr_hrrq_poll_timeout(unsigned long ptr)
Should be static
> +{
> + struct ipr_hrr_queue *hrrq = (struct ipr_hrr_queue *) ptr;
> + struct ipr_ioa_cfg *ioa_cfg = hrrq->ioa_cfg;
> + struct ipr_cmnd *ipr_cmd, *temp;
> + unsigned long hrrq_flags;
> + LIST_HEAD(doneq);
> +
> + if (ioa_cfg->poll_mode && ioa_cfg->sis64 && ioa_cfg->nvectors
> 1) {
> + spin_lock_irqsave(hrrq->lock, hrrq_flags);
> + ipr_process_hrrq(hrrq, -1, &doneq);
> + spin_unlock_irqrestore(hrrq->lock, hrrq_flags);
> +
> + list_for_each_entry_safe(ipr_cmd, temp, &doneq,
queue) {
> + list_del(&ipr_cmd->queue);
> + del_timer(&ipr_cmd->timer);
> + ipr_cmd->fast_done(ipr_cmd);
> + }
> + mod_timer(&hrrq->hrrq_poll_timer,
> + (jiffies + msecs_to_jiffies
(ipr_poll_timer)));
> + }
> +}
> +
> /**
> * ipr_isr - Interrupt service routine
> * @irq: irq number
> @@ -9880,6 +10013,11 @@ static void ipr_init_ioa_cfg(struct ipr_ioa_cfg
*ioa_cfg,
> ioa_cfg->hrrq[i].lock =
> ioa_cfg->
host->host_lock;
> else
> ioa_cfg->hrrq[i].lock =
> &ioa_cfg->hrrq
[i]._lock;
> +
> + init_timer(&ioa_cfg->hrrq[i].hrrq_poll_timer);
> + ioa_cfg->hrrq[i].hrrq_poll_timer.function =
ipr_hrrq_poll_timeout;
> + ioa_cfg->hrrq[i].hrrq_poll_timer.data =
> (unsigned
long) &ioa_cfg->hrrq[i];
> + ioa_cfg->hrrq[i].hrrq_poll_timer.expires =
> jiffies
+ msecs_to_jiffies(ipr_poll_timer);
> }
> }
>
> @@ -10547,6 +10685,12 @@ static void __ipr_remove(struct pci_dev *pdev)
>
> ipr_free_all_resources(ioa_cfg);
>
> + if (ioa_cfg->poll_mode && ioa_cfg->sis64 && ioa_cfg->nvectors
> 1) {
> + for (i = 0; i < IPR_MAX_HRRQ_NUM; i++)
> + del_timer(&ioa_cfg->hrrq
[i].hrrq_poll_timer);
> + ipr_enable_hrrq_interrupt(ioa_cfg);
> + }
> +
> LEAVE;
> }
>
> @@ -10585,6 +10729,7 @@ static void ipr_remove(struct pci_dev *pdev)
> static int ipr_probe(struct pci_dev *pdev, const struct pci_device_id
*dev_id)
> {
> struct ipr_ioa_cfg *ioa_cfg;
> + unsigned long lock_flags = 0;
> int rc, i;
>
> rc = ipr_probe_ioa(pdev, dev_id);
> @@ -10630,6 +10775,7 @@ static int ipr_probe(struct pci_dev *pdev, const
struct pci_device_id *dev_id)
> scsi_scan_host(ioa_cfg->host);
> ioa_cfg->iopoll_weight = ioa_cfg->chip_cfg->iopoll_weight;
> ioa_cfg->use_blk_mq = ipr_use_blk_mq;
> + ioa_cfg->poll_mode = ipr_poll_mode;
>
> if (ioa_cfg->iopoll_weight && ioa_cfg->sis64 && ioa_cfg->
nvectors > 1) {
> for (i = 1; i < ioa_cfg->hrrq_num; i++) {
> @@ -10639,6 +10785,15 @@ static int ipr_probe(struct pci_dev *pdev, const
struct pci_device_id *dev_id)
> }
> }
>
> + if (ioa_cfg->poll_mode && ioa_cfg->sis64 && ioa_cfg->nvectors
> 1) {
> + spin_lock_irqsave(ioa_cfg->host->host_lock,
lock_flags);
> + ipr_disable_hrrq_interrupt(ioa_cfg);
> + for (i = 0; i < ioa_cfg->hrrq_num; i++)
> + add_timer(&ioa_cfg->hrrq
[i].hrrq_poll_timer);
Same comment as above regarding using add_timer_on
> +
> +
> spin_unlock_irqrestore(ioa_cfg->host->host_lock,
lock_flags);
> + }
> +
> schedule_work(&ioa_cfg->work_q);
> return 0;
> }
> diff --git a/drivers/scsi/ipr.h b/drivers/scsi/ipr.h
> index c28a673..caa747c 100644
> --- a/drivers/scsi/ipr.h
> +++ b/drivers/scsi/ipr.h
> @@ -518,6 +518,7 @@ struct ipr_hrr_queue {
> u8 removing_ioa:1;
>
> struct blk_iopoll iopoll;
> + struct timer_list hrrq_poll_timer;
> };
>
> /* Command packet structure */
> @@ -1612,6 +1613,8 @@ struct ipr_ioa_cfg {
>
> u32 use_blk_mq;
>
> + u32 poll_mode;
> +
> }; /* struct ipr_ioa_cfg */
>
> struct ipr_cmnd {
>
--
Brian King
Power Linux I/O
IBM Linux Technology Center
------------------------------------------------------------------------------
Give your users amazing mobile app experiences with Intel XDK.
Use one codebase in this all-in-one HTML5 development environment.
Design, debug & build mobile apps & 2-D/3-D games for multiple OSs.
Then get your creation into app stores sooner, with many ways to monetize.
http://pubads.g.doubleclick.net/gampad/clk?id=254741551&iu=/4140
_______________________________________________
Iprdd-devel mailing list
Iprdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iprdd-devel
------------------------------------------------------------------------------
_______________________________________________
Iprdd-devel mailing list
Iprdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iprdd-devel
