| Message ID | 20220216090845.1278114-3-maz@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mvpp2: Survive CPU hotplug events | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 6 this patch: 6 |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 6 this patch: 6 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 100 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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.
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; + } - for_each_present_cpu(cpu) { - if (mvpp2_cpu_to_thread(port->priv, cpu) == - qv->sw_thread_id) - cpumask_set_cpu(cpu, qv->mask); - } + err = irq_set_affinity_masks(&affd, irqs, nvec); + if (err) + goto err; - irq_set_affinity_hint(qv->irq, qv->mask); + for (i = 0; i < port->nqvecs; i++) { + struct mvpp2_queue_vector *qv = port->qvecs + i; + + err = request_irq(qv->irq, mvpp2_isr, 0, port->dev->name, qv); + if (err) { + nvec = i; + break; } } - return 0; -err: - for (i = 0; i < port->nqvecs; i++) { - struct mvpp2_queue_vector *qv = port->qvecs + i; + if (err) { + for (i = 0; i < nvec; i++) { + struct mvpp2_queue_vector *qv = port->qvecs + i; - irq_set_affinity_hint(qv->irq, NULL); - kfree(qv->mask); - qv->mask = NULL; - free_irq(qv->irq, qv); + free_irq(qv->irq, qv); + } } +err: + kfree(irqs); + return err; } @@ -4727,10 +4732,6 @@ static void mvpp2_irqs_deinit(struct mvpp2_port *port) for (i = 0; i < port->nqvecs; i++) { struct mvpp2_queue_vector *qv = port->qvecs + i; - irq_set_affinity_hint(qv->irq, NULL); - kfree(qv->mask); - qv->mask = NULL; - irq_clear_status_flags(qv->irq, IRQ_NO_BALANCING); free_irq(qv->irq, qv); } }
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(-)