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

["Xia, Chenbo" <chenbo.xia@intel.com>]

Hi David & Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, February 28, 2023 12:28 AM
> To: David Marchand <david.marchand@redhat.com>; Xia, Chenbo
> <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; thomas@monjalon.net
> Subject: Re: [PATCH v2 11/20] net/virtio: annotate lock for guest announce
> 
> 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?

This LGTM, it makes things easier..

Thanks,
Chenbo

> 
> 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);