Re: [PATCH 2/2] net: mvpp2: Convert to managed interrupts to fix CPU HP issues

[Marc Zyngier]

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@xxxxxxxxxx>
> ---
>  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...