Hi David & Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coque...@redhat.com>
> Sent: Tuesday, February 28, 2023 12:28 AM
> To: David Marchand <david.march...@redhat.com>; Xia, Chenbo
> <chenbo....@intel.com>
> Cc: dev@dpdk.org; tho...@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....@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);
