Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Date:   Wed, 16 Feb 2022 11:38:20 +0000
From:   Marc Zyngier <maz@kernel.org>
To:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Marcin Wojtas <mw@semihalf.com>,
        Russell King <linux@armlinux.org.uk>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        John Garry <john.garry@huawei.com>, kernel-team@android.com
Subject: Re: [PATCH 2/2] net: mvpp2: Convert to managed interrupts to fix CPU
 HP issues
In-Reply-To: <20220216090845.1278114-3-maz@kernel.org>
References: <20220216090845.1278114-1-maz@kernel.org>
 <20220216090845.1278114-3-maz@kernel.org>
User-Agent: Roundcube Webmail/1.4.13
Message-ID: <44983ba2bfa801543db72872b5775701@kernel.org>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2022-02-16 09:08, Marc Zyngier wrote:
> The MVPP2 driver uses a set of per-CPU interrupts and relies on
> each particular interrupt to fire *only* on the CPU it has been
> assigned to.
> 
> Although the affinity setting is restricted to prevent userspace
> to move interrupts around, this all falls apart when using CPU
> hotplug, as this breaks the affinity. Depending on how lucky you
> are, the interrupt will then scream on the wrong CPU, eventually
> leading to an ugly crash.
> 
> Ideally, the interrupt assigned to a given CPU would simply be left
> where it is, only masked when the CPU goes down, and brought back
> up when the CPU is alive again. As it turns out, this is the model
> used for most multi-queue devices, and we'd be better off using it
> for the MVPP2 driver.
> 
> Drop the home-baked affinity settings in favour of the ready-made
> irq_set_affinity_masks() helper, making things slightly simpler.
> 
> With this change, the driver able to sustain CPUs being taken away.
> What is still missing is a way to tell the device that it should
> stop sending traffic to a given CPU.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |  1 -
>  .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 67 ++++++++++---------
>  2 files changed, 34 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> index ad73a488fc5f..86f8feaf5350 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> @@ -1143,7 +1143,6 @@ struct mvpp2_queue_vector {
>  	int nrxqs;
>  	u32 pending_cause_rx;
>  	struct mvpp2_port *port;
> -	struct cpumask *mask;
>  };
> 
>  /* Internal represention of a Flow Steering rule */
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 7cdbf8b8bbf6..cdc519583e86 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -4674,49 +4674,54 @@ static void mvpp21_get_mac_address(struct
> mvpp2_port *port, unsigned char *addr)
> 
>  static int mvpp2_irqs_init(struct mvpp2_port *port)
>  {
> -	int err, i;
> +	struct irq_affinity affd = {
> +		/* No pre/post-vectors, single set */
> +	};
> +	int err, i, nvec, *irqs;
> 
> -	for (i = 0; i < port->nqvecs; i++) {
> +	for (i = nvec = 0; i < port->nqvecs; i++) {
>  		struct mvpp2_queue_vector *qv = port->qvecs + i;
> 
> -		if (qv->type == MVPP2_QUEUE_VECTOR_PRIVATE) {
> -			qv->mask = kzalloc(cpumask_size(), GFP_KERNEL);
> -			if (!qv->mask) {
> -				err = -ENOMEM;
> -				goto err;
> -			}
> +		if (qv->type == MVPP2_QUEUE_VECTOR_PRIVATE)
> +			nvec++;
> +	}
> 
> -			irq_set_status_flags(qv->irq, IRQ_NO_BALANCING);
> -		}
> +	irqs = kmalloc(sizeof(*irqs) * nvec, GFP_KERNEL);
> +	if (!irqs)
> +		return -ENOMEM;
> 
> -		err = request_irq(qv->irq, mvpp2_isr, 0, port->dev->name, qv);
> -		if (err)
> -			goto err;
> +	for (i = 0; i < port->nqvecs; i++) {
> +		struct mvpp2_queue_vector *qv = port->qvecs + i;
> 
> -		if (qv->type == MVPP2_QUEUE_VECTOR_PRIVATE) {
> -			unsigned int cpu;
> +		if (qv->type == MVPP2_QUEUE_VECTOR_PRIVATE)
> +			irqs[i] = qv->irq;
> +	}

Errr, this is broken. non-private interrupts are not accounted for
in the sizing of the irqs[] array, so using 'i' as the index is
plain wrong.

I have added this on top:

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c 
b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index cdc519583e86..518ef07a067b 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -4690,11 +4690,11 @@ static int mvpp2_irqs_init(struct mvpp2_port 
*port)
  	if (!irqs)
  		return -ENOMEM;

-	for (i = 0; i < port->nqvecs; i++) {
+	for (i = nvec = 0; i < port->nqvecs; i++) {
  		struct mvpp2_queue_vector *qv = port->qvecs + i;

  		if (qv->type == MVPP2_QUEUE_VECTOR_PRIVATE)
-			irqs[i] = qv->irq;
+			irqs[nvec++] = qv->irq;
  	}

  	err = irq_set_affinity_masks(&affd, irqs, nvec);

Thanks to Russell for pointing out that something was amiss.

         M.
-- 
Jazz is not dead. It just smells funny...