Re: [PATCH v2 11/20] net/virtio: annotate lock for guest announce

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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.xia@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);