> -----Original Message-----
> From: Benjamin Poirier <bpoir...@suse.com>
> Sent: Monday, June 17, 2019 1:19 PM
> To: Manish Chopra <mani...@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> nic-...@marvell.com>; netdev@vger.kernel.org
> Subject: [PATCH net-next 01/16] qlge: Remove irq_cnt
>
> qlge uses an irq enable/disable refcounting scheme that is:
> * poorly implemented
> Uses a spin_lock to protect accesses to the irq_cnt atomic variable
> * buggy
> Breaks when there is not a 1:1 sequence of irq - napi_poll, such as
> when using SO_BUSY_POLL.
> * unnecessary
> The purpose or irq_cnt is to reduce irq control writes when
> multiple work items result from one irq: the irq is re-enabled
> after all work is done.
> Analysis of the irq handler shows that there is only one case where
> there might be two workers scheduled at once, and those have
> separate irq masking bits.
>
> Therefore, remove irq_cnt.
>
> Additionally, we get a performance improvement:
> perf stat -e cycles -a -r5 super_netperf 100 -H 192.168.33.1 -t TCP_RR
>
> Before:
> 628560
> 628056
> 622103
> 622744
> 627202
> [...]
> 268,803,947,669 cycles ( +- 0.09% )
>
> After:
> 636300
> 634106
> 634984
> 638555
> 634188
> [...]
> 259,237,291,449 cycles ( +- 0.19% )
>
> Signed-off-by: Benjamin Poirier <bpoir...@suse.com>
> ---
> drivers/net/ethernet/qlogic/qlge/qlge.h | 7 --
> drivers/net/ethernet/qlogic/qlge/qlge_main.c | 98 ++++++--------------
> drivers/net/ethernet/qlogic/qlge/qlge_mpi.c | 1 -
> 3 files changed, 27 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h
> b/drivers/net/ethernet/qlogic/qlge/qlge.h
> index ad7c5eb8a3b6..5d9a36deda08 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge.h
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
> @@ -1982,11 +1982,6 @@ struct intr_context {
> u32 intr_dis_mask; /* value/mask used to disable this intr */
> u32 intr_read_mask; /* value/mask used to read this intr */
> char name[IFNAMSIZ * 2];
> - atomic_t irq_cnt; /* irq_cnt is used in single vector
> - * environment. It's incremented for each
> - * irq handler that is scheduled. When each
> - * handler finishes it decrements irq_cnt and
> - * enables interrupts if it's zero. */
> irq_handler_t handler;
> };
>
> @@ -2074,7 +2069,6 @@ struct ql_adapter {
> u32 port; /* Port number this adapter */
>
> spinlock_t adapter_lock;
> - spinlock_t hw_lock;
> spinlock_t stats_lock;
>
> /* PCI Bus Relative Register Addresses */ @@ -2235,7 +2229,6 @@
> void ql_mpi_reset_work(struct work_struct *work); void
> ql_mpi_core_to_log(struct work_struct *work); int ql_wait_reg_rdy(struct
> ql_adapter *qdev, u32 reg, u32 bit, u32 ebit); void
> ql_queue_asic_error(struct ql_adapter *qdev);
> -u32 ql_enable_completion_interrupt(struct ql_adapter *qdev, u32 intr);
> void ql_set_ethtool_ops(struct net_device *ndev); int
> ql_read_xgmac_reg64(struct ql_adapter *qdev, u32 reg, u64 *data); void
> ql_mpi_idc_work(struct work_struct *work); diff --git
> a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index 6cae33072496..0bfbe11db795 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -625,75 +625,26 @@ static void ql_disable_interrupts(struct ql_adapter
> *qdev)
> ql_write32(qdev, INTR_EN, (INTR_EN_EI << 16)); }
>
> -/* If we're running with multiple MSI-X vectors then we enable on the fly.
> - * Otherwise, we may have multiple outstanding workers and don't want to
> - * enable until the last one finishes. In this case, the irq_cnt gets
> - * incremented every time we queue a worker and decremented every time
> - * a worker finishes. Once it hits zero we enable the interrupt.
> - */
> -u32 ql_enable_completion_interrupt(struct ql_adapter *qdev, u32 intr)
> +static void ql_enable_completion_interrupt(struct ql_adapter *qdev, u32
> +intr)
> {
> - u32 var = 0;
> - unsigned long hw_flags = 0;
> - struct intr_context *ctx = qdev->intr_context + intr;
> -
> - if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr)) {
> - /* Always enable if we're MSIX multi interrupts and
> - * it's not the default (zeroeth) interrupt.
> - */
> - ql_write32(qdev, INTR_EN,
> - ctx->intr_en_mask);
> - var = ql_read32(qdev, STS);
> - return var;
> - }
> + struct intr_context *ctx = &qdev->intr_context[intr];
>
> - spin_lock_irqsave(&qdev->hw_lock, hw_flags);
> - if (atomic_dec_and_test(&ctx->irq_cnt)) {
> - ql_write32(qdev, INTR_EN,
> - ctx->intr_en_mask);
> - var = ql_read32(qdev, STS);
> - }
> - spin_unlock_irqrestore(&qdev->hw_lock, hw_flags);
> - return var;
> + ql_write32(qdev, INTR_EN, ctx->intr_en_mask);
> }
>
> -static u32 ql_disable_completion_interrupt(struct ql_adapter *qdev, u32
> intr)
> +static void ql_disable_completion_interrupt(struct ql_adapter *qdev,
> +u32 intr)
> {
> - u32 var = 0;
> - struct intr_context *ctx;
> + struct intr_context *ctx = &qdev->intr_context[intr];
>
> - /* HW disables for us if we're MSIX multi interrupts and
> - * it's not the default (zeroeth) interrupt.
> - */
> - if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr))
> - return 0;
> -
> - ctx = qdev->intr_context + intr;
> - spin_lock(&qdev->hw_lock);
> - if (!atomic_read(&ctx->irq_cnt)) {
> - ql_write32(qdev, INTR_EN,
> - ctx->intr_dis_mask);
> - var = ql_read32(qdev, STS);
> - }
> - atomic_inc(&ctx->irq_cnt);
> - spin_unlock(&qdev->hw_lock);
> - return var;
> + ql_write32(qdev, INTR_EN, ctx->intr_dis_mask);
> }
>
> static void ql_enable_all_completion_interrupts(struct ql_adapter *qdev) {
> int i;
> - for (i = 0; i < qdev->intr_count; i++) {
> - /* The enable call does a atomic_dec_and_test
> - * and enables only if the result is zero.
> - * So we precharge it here.
> - */
> - if (unlikely(!test_bit(QL_MSIX_ENABLED, &qdev->flags) ||
> - i == 0))
> - atomic_set(&qdev->intr_context[i].irq_cnt, 1);
> - ql_enable_completion_interrupt(qdev, i);
> - }
>
> + for (i = 0; i < qdev->intr_count; i++)
> + ql_enable_completion_interrupt(qdev, i);
> }
>
> static int ql_validate_flash(struct ql_adapter *qdev, u32 size, const char
> *str) @@ -2500,21 +2451,22 @@ static irqreturn_t qlge_isr(int irq, void
> *dev_id)
> u32 var;
> int work_done = 0;
>
> - spin_lock(&qdev->hw_lock);
> - if (atomic_read(&qdev->intr_context[0].irq_cnt)) {
> - netif_printk(qdev, intr, KERN_DEBUG, qdev->ndev,
> - "Shared Interrupt, Not ours!\n");
> - spin_unlock(&qdev->hw_lock);
> - return IRQ_NONE;
> - }
> - spin_unlock(&qdev->hw_lock);
> + /* Experience shows that when using INTx interrupts, the device
> does
> + * not always auto-mask the interrupt.
> + * When using MSI mode, the interrupt must be explicitly disabled
> + * (even though it is auto-masked), otherwise a later command to
> + * enable it is not effective.
> + */
> + if (!test_bit(QL_MSIX_ENABLED, &qdev->flags))
> + ql_disable_completion_interrupt(qdev, 0);
>
> - var = ql_disable_completion_interrupt(qdev, intr_context->intr);
> + var = ql_read32(qdev, STS);
>
> /*
> * Check for fatal error.
> */
> if (var & STS_FE) {
> + ql_disable_completion_interrupt(qdev, 0);
> ql_queue_asic_error(qdev);
> netdev_err(qdev->ndev, "Got fatal error, STS = %x.\n", var);
> var = ql_read32(qdev, ERR_STS);
> @@ -2534,7 +2486,6 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
> */
> netif_err(qdev, intr, qdev->ndev,
> "Got MPI processor interrupt.\n");
> - ql_disable_completion_interrupt(qdev, intr_context->intr);
> ql_write32(qdev, INTR_MASK, (INTR_MASK_PI << 16));
> queue_delayed_work_on(smp_processor_id(),
> qdev->workqueue, &qdev->mpi_work, 0);
> @@ -2550,11 +2501,18 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
> if (var & intr_context->irq_mask) {
> netif_info(qdev, intr, qdev->ndev,
> "Waking handler for rx_ring[0].\n");
> - ql_disable_completion_interrupt(qdev, intr_context->intr);
> napi_schedule(&rx_ring->napi);
> work_done++;
> + } else {
> + /* Experience shows that the device sometimes signals an
> + * interrupt but no work is scheduled from this function.
> + * Nevertheless, the interrupt is auto-masked. Therefore, we
> + * systematically re-enable the interrupt if we didn't
> + * schedule napi.
> + */
> + ql_enable_completion_interrupt(qdev, 0);
> }
> - ql_enable_completion_interrupt(qdev, intr_context->intr);
> +
> return work_done ? IRQ_HANDLED : IRQ_NONE; }
>
> @@ -3557,7 +3515,6 @@ static int ql_request_irq(struct ql_adapter *qdev)
> ql_resolve_queues_to_irqs(qdev);
>
> for (i = 0; i < qdev->intr_count; i++, intr_context++) {
> - atomic_set(&intr_context->irq_cnt, 0);
> if (test_bit(QL_MSIX_ENABLED, &qdev->flags)) {
> status = request_irq(qdev->msi_x_entry[i].vector,
> intr_context->handler,
> @@ -4642,7 +4599,6 @@ static int ql_init_device(struct pci_dev *pdev,
> struct net_device *ndev,
> goto err_out2;
> }
> qdev->msg_enable = netif_msg_init(debug, default_msg);
> - spin_lock_init(&qdev->hw_lock);
> spin_lock_init(&qdev->stats_lock);
>
> if (qlge_mpi_coredump) {
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_mpi.c
> b/drivers/net/ethernet/qlogic/qlge/qlge_mpi.c
> index 957c72985a06..9e422bbbb6ab 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_mpi.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_mpi.c
> @@ -1257,7 +1257,6 @@ void ql_mpi_work(struct work_struct *work)
> /* End polled mode for MPI */
> ql_write32(qdev, INTR_MASK, (INTR_MASK_PI << 16) |
> INTR_MASK_PI);
> mutex_unlock(&qdev->mpi_mutex);
> - ql_enable_completion_interrupt(qdev, 0);
> }
>
> void ql_mpi_reset_work(struct work_struct *work)
> --
> 2.21.0
Hello Benjamin,
Just FYI. I am OOO for a week, so reviewing and testing these patches will take
time.
Thanks,
Manish
