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?
--
David Marchand
