git: ed037ffb2c05 - stable/12 - ena: Move ena_copy_eni_metrics into separate task
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Tue, 26 Jul 2022 19:33:26 UTC
The branch stable/12 has been updated by mw:
URL: https://cgit.FreeBSD.org/src/commit/?id=ed037ffb2c05f08ff1c1633ce7f9e92ffe4e6b07
commit ed037ffb2c05f08ff1c1633ce7f9e92ffe4e6b07
Author: Dawid Gorecki <dgr@semihalf.com>
AuthorDate: 2022-06-10 09:18:10 +0000
Commit: Marcin Wojtas <mw@FreeBSD.org>
CommitDate: 2022-07-26 19:33:03 +0000
ena: Move ena_copy_eni_metrics into separate task
Copying ENI metrics was done in callout context, this caused the driver
to panic when sample_interval was set to a value other than 0, as the
admin queue call which was executed could sleep while waiting on
a condition variable. Taskqueue, unlike callout, allows for sleeping, so
moving the function to a separate taskqueue fixes the problem.
ena_timer_service is still responsible for scheduling the taskqueue.
Stop draining the callout during ena_up/ena_down. This was done to
prevent a race between ena_up/down and ena_copy_eni_metrics admin queue
calls. Since ena_metrics_task is protected by ENA_LOCK there is no
possibility of a race between ena_up/down and ena_metrics_task.
Remove a comment about locking in ena_timer_service. With ENI metrics
in a separate task this comment became obsolete.
Obtained from: Semihalf
MFC after: 2 weeks
Sponsored by: Amazon, Inc.
(cherry picked from commit b899a02ad7330cae3c9bb08ad7975601dc3b9551)
---
sys/dev/ena/ena.c | 57 ++++++++++++++++++++++---------------------------------
sys/dev/ena/ena.h | 2 ++
2 files changed, 25 insertions(+), 34 deletions(-)
diff --git a/sys/dev/ena/ena.c b/sys/dev/ena/ena.c
index 82f8d42ea0b0..b28156ef2ab2 100644
--- a/sys/dev/ena/ena.c
+++ b/sys/dev/ena/ena.c
@@ -2104,13 +2104,6 @@ ena_up(struct ena_adapter *adapter)
ena_log(adapter->pdev, INFO, "device is going UP\n");
- /*
- * ena_timer_service can use functions, which write to the admin queue.
- * Those calls are not protected by ENA_LOCK, and because of that, the
- * timer should be stopped when bringing the device up or down.
- */
- ENA_TIMER_DRAIN(adapter);
-
/* setup interrupts for IO queues */
rc = ena_setup_io_intr(adapter);
if (unlikely(rc != 0)) {
@@ -2157,8 +2150,6 @@ ena_up(struct ena_adapter *adapter)
ena_unmask_all_io_irqs(adapter);
- ENA_TIMER_RESET(adapter);
-
return (0);
err_up_complete:
@@ -2168,8 +2159,6 @@ err_up_complete:
err_create_queues_with_backoff:
ena_free_io_irq(adapter);
error:
- ENA_TIMER_RESET(adapter);
-
return (rc);
}
@@ -2473,9 +2462,6 @@ ena_down(struct ena_adapter *adapter)
if (!ENA_FLAG_ISSET(ENA_FLAG_DEV_UP, adapter))
return;
- /* Drain timer service to avoid admin queue race condition. */
- ENA_TIMER_DRAIN(adapter);
-
ena_log(adapter->pdev, INFO, "device is going DOWN\n");
ENA_FLAG_CLEAR_ATOMIC(ENA_FLAG_DEV_UP, adapter);
@@ -2500,8 +2486,6 @@ ena_down(struct ena_adapter *adapter)
ena_free_all_rx_resources(adapter);
counter_u64_add(adapter->dev_stats.interface_down, 1);
-
- ENA_TIMER_RESET(adapter);
}
static uint32_t
@@ -3279,24 +3263,7 @@ ena_timer_service(void *data)
if ((adapter->eni_metrics_sample_interval != 0) &&
(++adapter->eni_metrics_sample_interval_cnt >=
adapter->eni_metrics_sample_interval)) {
- /*
- * There is no race with other admin queue calls, as:
- * - Timer service runs after attach function ends, so all
- * configuration calls to the admin queue are finished.
- * - Timer service is temporarily stopped when bringing
- * the interface up or down.
- * - After interface is up, the driver doesn't use (at least
- * for now) other functions writing to the admin queue.
- *
- * It may change in the future, so in that situation, the lock
- * will be needed. ENA_LOCK_*() cannot be used for that purpose,
- * as callout ena_timer_service is protected by them. It could
- * lead to the deadlock if callout_drain() would hold the lock
- * before ena_copy_eni_metrics() was executed. It's advised to
- * use separate lock in that situation which will be used only
- * for the admin queue.
- */
- (void)ena_copy_eni_metrics(adapter);
+ taskqueue_enqueue(adapter->metrics_tq, &adapter->metrics_task);
adapter->eni_metrics_sample_interval_cnt = 0;
}
@@ -3503,6 +3470,16 @@ err:
return (rc);
}
+static void
+ena_metrics_task(void *arg, int pending)
+{
+ struct ena_adapter *adapter = (struct ena_adapter *)arg;
+
+ ENA_LOCK_LOCK();
+ (void)ena_copy_eni_metrics(adapter);
+ ENA_LOCK_UNLOCK();
+}
+
static void
ena_reset_task(void *arg, int pending)
{
@@ -3723,6 +3700,13 @@ ena_attach(device_t pdev)
taskqueue_start_threads(&adapter->reset_tq, 1, PI_NET,
"%s rstq", device_get_nameunit(adapter->pdev));
+ /* Initialize metrics task queue */
+ TASK_INIT(&adapter->metrics_task, 0, ena_metrics_task, adapter);
+ adapter->metrics_tq = taskqueue_create("ena_metrics_enqueue",
+ M_WAITOK | M_ZERO, taskqueue_thread_enqueue, &adapter->metrics_tq);
+ taskqueue_start_threads(&adapter->metrics_tq, 1, PI_NET,
+ "%s metricsq", device_get_nameunit(adapter->pdev));
+
/* Initialize statistics */
ena_alloc_counters((counter_u64_t *)&adapter->dev_stats,
sizeof(struct ena_stats_dev));
@@ -3801,6 +3785,11 @@ ena_detach(device_t pdev)
ENA_TIMER_DRAIN(adapter);
ENA_LOCK_UNLOCK();
+ /* Release metrics task */
+ while (taskqueue_cancel(adapter->metrics_tq, &adapter->metrics_task, NULL))
+ taskqueue_drain(adapter->metrics_tq, &adapter->metrics_task);
+ taskqueue_free(adapter->metrics_tq);
+
/* Release reset task */
while (taskqueue_cancel(adapter->reset_tq, &adapter->reset_task, NULL))
taskqueue_drain(adapter->reset_tq, &adapter->reset_task);
diff --git a/sys/dev/ena/ena.h b/sys/dev/ena/ena.h
index b100b091c89f..3219c4cf7455 100644
--- a/sys/dev/ena/ena.h
+++ b/sys/dev/ena/ena.h
@@ -469,6 +469,8 @@ struct ena_adapter {
uint32_t next_monitored_tx_qid;
struct task reset_task;
struct taskqueue *reset_tq;
+ struct task metrics_task;
+ struct taskqueue *metrics_tq;
int wd_active;
sbintime_t keep_alive_timeout;
sbintime_t missing_tx_timeout;