[v2,11/20] net/virtio: annotate lock for guest announce
Checks
Commit Message
Expose requirements for helpers dealing with the
VIRTIO_DEV_TO_HW(dev)->state_lock lock.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
drivers/net/virtio/virtio_ethdev.c | 8 ++++----
drivers/net/virtio/virtio_ethdev.h | 7 +++++--
2 files changed, 9 insertions(+), 6 deletions(-)
Comments
Hi David,
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, February 24, 2023 11:12 PM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; Maxime Coquelin <maxime.coquelin@redhat.com>; Xia,
> Chenbo <chenbo.xia@intel.com>
> Subject: [PATCH v2 11/20] net/virtio: annotate lock for guest announce
>
> Expose requirements for helpers dealing with the
> VIRTIO_DEV_TO_HW(dev)->state_lock lock.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 8 ++++----
> drivers/net/virtio/virtio_ethdev.h | 7 +++++--
> 2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index 0103d95920..a3de44958c 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1149,11 +1149,11 @@ virtio_dev_pause(struct rte_eth_dev *dev)
> {
> struct virtio_hw *hw = dev->data->dev_private;
>
> - rte_spinlock_lock(&hw->state_lock);
> + rte_spinlock_lock(&VIRTIO_DEV_TO_HW(dev)->state_lock);
>
> if (hw->started == 0) {
> /* Device is just stopped. */
> - rte_spinlock_unlock(&hw->state_lock);
> + rte_spinlock_unlock(&VIRTIO_DEV_TO_HW(dev)->state_lock);
> return -1;
> }
> hw->started = 0;
> @@ -1174,7 +1174,7 @@ 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);
> + rte_spinlock_unlock(&VIRTIO_DEV_TO_HW(dev)->state_lock);
> }
>
> /*
> @@ -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?
Thanks,
Chenbo
> +void virtio_dev_resume(struct rte_eth_dev *dev)
> + __rte_unlock_function(&VIRTIO_DEV_TO_HW(dev)->state_lock);
> 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,
> --
> 2.39.2
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?
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);
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);
On Mon, Feb 27, 2023 at 5:28 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> 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).
I was looking at doing this and, as we discussed offlist, realised
what the "inject" code was doing.
I don't like the idea of keeping virtio_inject_pkts as a helper.
This is tightly linked to the hw->started + usleep() trick, and this
code has no check about its requirement on hw->started == 0.
I'd rather "inline" and remove this helper too.
As a second step, I wrote a fix using an additional lock per txq to
remove this usleep() thing.
Though I think it is too late to consider merging this new change in
the release.
WDYT?
On 3/2/23 10:26, David Marchand wrote:
> On Mon, Feb 27, 2023 at 5:28 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>> 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).
>
> I was looking at doing this and, as we discussed offlist, realised
> what the "inject" code was doing.
>
> I don't like the idea of keeping virtio_inject_pkts as a helper.
> This is tightly linked to the hw->started + usleep() trick, and this
> code has no check about its requirement on hw->started == 0.
> I'd rather "inline" and remove this helper too.
Makes sense, do you want to submit it or are you willing me to do it?
> As a second step, I wrote a fix using an additional lock per txq to
> remove this usleep() thing.
> Though I think it is too late to consider merging this new change in
> the release.
> WDYT?
I agree this is too late for this release. Let's try this for v23.07.
Thanks,
Maxime
On Thu, Mar 2, 2023 at 10:28 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 3/2/23 10:26, David Marchand wrote:
> > On Mon, Feb 27, 2023 at 5:28 PM Maxime Coquelin
> > <maxime.coquelin@redhat.com> wrote:
> >> 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).
> >
> > I was looking at doing this and, as we discussed offlist, realised
> > what the "inject" code was doing.
> >
> > I don't like the idea of keeping virtio_inject_pkts as a helper.
> > This is tightly linked to the hw->started + usleep() trick, and this
> > code has no check about its requirement on hw->started == 0.
> > I'd rather "inline" and remove this helper too.
>
> Makes sense, do you want to submit it or are you willing me to do it?
I'll handle it, either during this release, or the next one.
This code has always been ugly, this can wait one more release.
@@ -1149,11 +1149,11 @@ virtio_dev_pause(struct rte_eth_dev *dev)
{
struct virtio_hw *hw = dev->data->dev_private;
- rte_spinlock_lock(&hw->state_lock);
+ rte_spinlock_lock(&VIRTIO_DEV_TO_HW(dev)->state_lock);
if (hw->started == 0) {
/* Device is just stopped. */
- rte_spinlock_unlock(&hw->state_lock);
+ rte_spinlock_unlock(&VIRTIO_DEV_TO_HW(dev)->state_lock);
return -1;
}
hw->started = 0;
@@ -1174,7 +1174,7 @@ 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);
+ rte_spinlock_unlock(&VIRTIO_DEV_TO_HW(dev)->state_lock);
}
/*
@@ -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;
}
@@ -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);
+void virtio_dev_resume(struct rte_eth_dev *dev)
+ __rte_unlock_function(&VIRTIO_DEV_TO_HW(dev)->state_lock);
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,