Hi,
On 2/27/23 09:24, David Marchand wrote:
Hello Chenbo,
Adding Maxime too.
On Mon, Feb 27, 2023 at 3:05 AM Xia, Chenbo <chenbo....@intel.com> wrote:
@@ -1217,7 +1217,7 @@ virtio_notify_peers(struct rte_eth_dev *dev)
}
/* If virtio port just stopped, no need to send RARP */
- if (virtio_dev_pause(dev) < 0) {
+ if (virtio_dev_pause(dev) != 0) {
rte_pktmbuf_free(rarp_mbuf);
return;
}
diff --git a/drivers/net/virtio/virtio_ethdev.h
b/drivers/net/virtio/virtio_ethdev.h
index c08f382791..ece0130603 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -112,8 +112,11 @@ int eth_virtio_dev_init(struct rte_eth_dev *eth_dev);
void virtio_interrupt_handler(void *param);
-int virtio_dev_pause(struct rte_eth_dev *dev);
-void virtio_dev_resume(struct rte_eth_dev *dev);
+#define VIRTIO_DEV_TO_HW(dev) ((struct virtio_hw *)(dev)->data-
dev_private)
+int virtio_dev_pause(struct rte_eth_dev *dev)
+ __rte_exclusive_trylock_function(0, &VIRTIO_DEV_TO_HW(dev)-
state_lock);
Just curious, why this is trylock instead of lock?
I wrote this patch some time ago.
At the time, I must say that I preferred removing those helpers (the
only caller is virtio_notify_peers()).
It seems those helpers were added as a kind of api for future
usecases, it seemed a reason for keeping them.
So I changed my mind and just annotated them.
For your question, annotating with "lock" would tell clang that the
function always takes the lock, regardless of the function return
value.
One alternative to this patch could be to always take the lock
(+annotate dev_pause as "lock"), and have the caller release the lock
if != 0 return value.
But it seems counterintuitive to me.
WDYT?
As discussed with David off-list, I think we could simplify and inline
virtio_dev_pause()/virtio_dev_resume() into virtio_notify_peers() since
there are no other users of these functions (see below).
Any objection?
diff --git a/drivers/net/virtio/virtio_ethdev.c
b/drivers/net/virtio/virtio_ethdev.c
index 0103d95920..dbd84e25ea 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1144,43 +1144,10 @@ virtio_ethdev_negotiate_features(struct
virtio_hw *hw, uint64_t req_features)
return 0;
}
-int
-virtio_dev_pause(struct rte_eth_dev *dev)
-{
- struct virtio_hw *hw = dev->data->dev_private;
-
- rte_spinlock_lock(&hw->state_lock);
-
- if (hw->started == 0) {
- /* Device is just stopped. */
- rte_spinlock_unlock(&hw->state_lock);
- return -1;
- }
- hw->started = 0;
- /*
- * Prevent the worker threads from touching queues to avoid
contention,
- * 1 ms should be enough for the ongoing Tx function to finish.
- */
- rte_delay_ms(1);
- return 0;
-}
-
-/*
- * Recover hw state to let the worker threads continue.
- */
-void
-virtio_dev_resume(struct rte_eth_dev *dev)
-{
- struct virtio_hw *hw = dev->data->dev_private;
-
- hw->started = 1;
- rte_spinlock_unlock(&hw->state_lock);
-}
-
/*
* Should be called only after device is paused.
*/
-int
+static int
virtio_inject_pkts(struct rte_eth_dev *dev, struct rte_mbuf **tx_pkts,
int nb_pkts)
{
@@ -1216,14 +1183,25 @@ virtio_notify_peers(struct rte_eth_dev *dev)
return;
}
- /* If virtio port just stopped, no need to send RARP */
- if (virtio_dev_pause(dev) < 0) {
+ rte_spinlock_lock(&hw->state_lock);
+
+ if (hw->started == 0) {
+ /* If virtio port just stopped, no need to send RARP */
rte_pktmbuf_free(rarp_mbuf);
- return;
+ goto out_unlock;
}
+ /*
+ * Prevent the worker threads from touching queues to avoid
contention,
+ * 1 ms should be enough for the ongoing Tx function to finish.
+ */
+ rte_delay_ms(1);
+
virtio_inject_pkts(dev, &rarp_mbuf, 1);
- virtio_dev_resume(dev);
+ hw->started = 1;
+
+out_unlock:
+ rte_spinlock_unlock(&hw->state_lock);
}
static void
diff --git a/drivers/net/virtio/virtio_ethdev.h
b/drivers/net/virtio/virtio_ethdev.h
index c08f382791..7be1c9acd0 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -112,12 +112,8 @@ int eth_virtio_dev_init(struct rte_eth_dev *eth_dev);
void virtio_interrupt_handler(void *param);
-int virtio_dev_pause(struct rte_eth_dev *dev);
-void virtio_dev_resume(struct rte_eth_dev *dev);
int virtio_dev_stop(struct rte_eth_dev *dev);
int virtio_dev_close(struct rte_eth_dev *dev);
-int virtio_inject_pkts(struct rte_eth_dev *dev, struct rte_mbuf **tx_pkts,
- int nb_pkts);
bool virtio_rx_check_scatter(uint16_t max_rx_pkt_len, uint16_t
rx_buf_size,
bool rx_scatter_enabled, const char **error);
