[dpdk-dev,v3,4/4] net/vhost: add NULL pointer checking
Checks
Commit Message
When vhost user PMD works in client mode to connect/reconnect virtio-user
with server mode, new thread sometimes may run to new_device before
queue_setup has been done, So have to wait until memory allocation is
done.
Release note is updated in the patch.
Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
---
doc/guides/rel_notes/release_18_05.rst | 7 +++++++
drivers/net/vhost/rte_eth_vhost.c | 9 +++++++++
2 files changed, 16 insertions(+)
Comments
Hi,
On 03/21/2018 04:03 AM, zhiyong.yang@intel.com wrote:
> When vhost user PMD works in client mode to connect/reconnect virtio-user
> with server mode, new thread sometimes may run to new_device before
> queue_setup has been done, So have to wait until memory allocation is
> done.
>
> Release note is updated in the patch.
>
> Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
> ---
> doc/guides/rel_notes/release_18_05.rst | 7 +++++++
> drivers/net/vhost/rte_eth_vhost.c | 9 +++++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/doc/guides/rel_notes/release_18_05.rst b/doc/guides/rel_notes/release_18_05.rst
> index 3923dc253..7b301f021 100644
> --- a/doc/guides/rel_notes/release_18_05.rst
> +++ b/doc/guides/rel_notes/release_18_05.rst
> @@ -41,6 +41,13 @@ New Features
> Also, make sure to start the actual text at the margin.
> =========================================================
>
> +* **Added support for virtio-user server mode.**
> +
> + In a container environment if the vhost-user backend restarts, there's no way
> + for it to reconnect to virtio-user. To address this, support for server mode
> + is added. In this mode the socket file is created by virtio-user, which the
> + backend then connects to. This means that if the backend restarts, it can
> + reconnect to virtio-user and continue communications.
I think this shouldn't be part of this patch.
>
> API Changes
> -----------
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 3aae01c39..2490bad0b 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -580,6 +580,15 @@ new_device(int vid)
> eth_dev->data->numa_node = newnode;
> #endif
>
> + /* The thread may run here before eth_dev->data->rx_queues or
> + * eth_dev->data->tx_queues have gotten valid memory, so have to
> + * wait until memory allocation is done.
> + */
> + while (!eth_dev->data->rx_queues ||
> + !eth_dev->data->tx_queues) {
> + usleep(1);
> + }
> +
> for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> vq = eth_dev->data->rx_queues[i];
> if (vq == NULL)
>
I don't like the idea of polling here.
It looks like Junjie is addressing the problem in a different way [0],
do you confirm it would work in your case?
Thanks,
Maxime
[0]: http://dpdk.org/dev/patchwork/patch/36643/
Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Thursday, March 29, 2018 9:20 PM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>; dev@dpdk.org
> Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; Wang, Zhihong
> <zhihong.wang@intel.com>; thomas@monjalon.net; Wang, Dong1
> <dong1.wang@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>
> Subject: Re: [PATCH v3 4/4] net/vhost: add NULL pointer checking
>
> Hi,
>
> On 03/21/2018 04:03 AM, zhiyong.yang@intel.com wrote:
> > When vhost user PMD works in client mode to connect/reconnect
> > virtio-user with server mode, new thread sometimes may run to
> > new_device before queue_setup has been done, So have to wait until
> > memory allocation is done.
> >
> > Release note is updated in the patch.
> >
> > Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
> > ---
> > doc/guides/rel_notes/release_18_05.rst | 7 +++++++
> > drivers/net/vhost/rte_eth_vhost.c | 9 +++++++++
> > 2 files changed, 16 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/release_18_05.rst
> > b/doc/guides/rel_notes/release_18_05.rst
> > index 3923dc253..7b301f021 100644
> > --- a/doc/guides/rel_notes/release_18_05.rst
> > +++ b/doc/guides/rel_notes/release_18_05.rst
> > @@ -41,6 +41,13 @@ New Features
> > Also, make sure to start the actual text at the margin.
> >
> =========================================================
> >
> > +* **Added support for virtio-user server mode.**
> > +
> > + In a container environment if the vhost-user backend restarts,
> > + there's no way for it to reconnect to virtio-user. To address this,
> > + support for server mode is added. In this mode the socket file is
> > + created by virtio-user, which the backend then connects to. This
> > + means that if the backend restarts, it can reconnect to virtio-user and
> continue communications.
>
> I think this shouldn't be part of this patch.
>
Ok, I can merge it with the previous patch 3/4.
> >
> > API Changes
> > -----------
> > diff --git a/drivers/net/vhost/rte_eth_vhost.c
> > b/drivers/net/vhost/rte_eth_vhost.c
> > index 3aae01c39..2490bad0b 100644
> > --- a/drivers/net/vhost/rte_eth_vhost.c
> > +++ b/drivers/net/vhost/rte_eth_vhost.c
> > @@ -580,6 +580,15 @@ new_device(int vid)
> > eth_dev->data->numa_node = newnode;
> > #endif
> >
> > + /* The thread may run here before eth_dev->data->rx_queues or
> > + * eth_dev->data->tx_queues have gotten valid memory, so have to
> > + * wait until memory allocation is done.
> > + */
> > + while (!eth_dev->data->rx_queues ||
> > + !eth_dev->data->tx_queues) {
> > + usleep(1);
> > + }
> > +
> > for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> > vq = eth_dev->data->rx_queues[i];
> > if (vq == NULL)
> >
>
> I don't like the idea of polling here.
> It looks like Junjie is addressing the problem in a different way [0], do you
> confirm it would work in your case?
>
Great to hear that. I have to fix it when the issue is found.
It's better to have another solution. I will test it later.
Thanks
Zhiyong
> Thanks,
> Maxime
>
> [0]: http://dpdk.org/dev/patchwork/patch/36643/
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yang, Zhiyong
> Sent: Friday, March 30, 2018 10:01 AM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; dev@dpdk.org
> Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; Wang, Zhihong
> <zhihong.wang@intel.com>; thomas@monjalon.net; Wang, Dong1
> <dong1.wang@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 4/4] net/vhost: add NULL pointer
> checking
>
> Hi Maxime,
>
> > -----Original Message-----
> > From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> > Sent: Thursday, March 29, 2018 9:20 PM
> > To: Yang, Zhiyong <zhiyong.yang@intel.com>; dev@dpdk.org
> > Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; Wang, Zhihong
> > <zhihong.wang@intel.com>; thomas@monjalon.net; Wang, Dong1
> > <dong1.wang@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>
> > Subject: Re: [PATCH v3 4/4] net/vhost: add NULL pointer checking
> >
> > Hi,
> >
> > On 03/21/2018 04:03 AM, zhiyong.yang@intel.com wrote:
> > > When vhost user PMD works in client mode to connect/reconnect
> > > virtio-user with server mode, new thread sometimes may run to
> > > new_device before queue_setup has been done, So have to wait until
> > > memory allocation is done.
> > >
> > > Release note is updated in the patch.
> > >
> > > Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
> > > ---
> > > doc/guides/rel_notes/release_18_05.rst | 7 +++++++
> > > drivers/net/vhost/rte_eth_vhost.c | 9 +++++++++
> > > 2 files changed, 16 insertions(+)
> > >
> > > diff --git a/doc/guides/rel_notes/release_18_05.rst
> > > b/doc/guides/rel_notes/release_18_05.rst
> > > index 3923dc253..7b301f021 100644
> > > --- a/doc/guides/rel_notes/release_18_05.rst
> > > +++ b/doc/guides/rel_notes/release_18_05.rst
> > > @@ -41,6 +41,13 @@ New Features
> > > Also, make sure to start the actual text at the margin.
> > >
> > =========================================================
> > >
> > > +* **Added support for virtio-user server mode.**
> > > +
> > > + In a container environment if the vhost-user backend restarts,
> > > + there's no way for it to reconnect to virtio-user. To address
> > > + this, support for server mode is added. In this mode the socket
> > > + file is created by virtio-user, which the backend then connects
> > > + to. This means that if the backend restarts, it can reconnect to
> > > + virtio-user and
> > continue communications.
> >
> > I think this shouldn't be part of this patch.
> >
> Ok, I can merge it with the previous patch 3/4.
>
> > >
> > > API Changes
> > > -----------
> > > diff --git a/drivers/net/vhost/rte_eth_vhost.c
> > > b/drivers/net/vhost/rte_eth_vhost.c
> > > index 3aae01c39..2490bad0b 100644
> > > --- a/drivers/net/vhost/rte_eth_vhost.c
> > > +++ b/drivers/net/vhost/rte_eth_vhost.c
> > > @@ -580,6 +580,15 @@ new_device(int vid)
> > > eth_dev->data->numa_node = newnode;
> > > #endif
> > >
> > > + /* The thread may run here before eth_dev->data->rx_queues or
> > > + * eth_dev->data->tx_queues have gotten valid memory, so have to
> > > + * wait until memory allocation is done.
> > > + */
> > > + while (!eth_dev->data->rx_queues ||
> > > + !eth_dev->data->tx_queues) {
> > > + usleep(1);
> > > + }
> > > +
> > > for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> > > vq = eth_dev->data->rx_queues[i];
> > > if (vq == NULL)
> > >
> >
> > I don't like the idea of polling here.
> > It looks like Junjie is addressing the problem in a different way [0],
> > do you confirm it would work in your case?
> >
>
> Great to hear that. I have to fix it when the issue is found.
> It's better to have another solution. I will test it later.
>
Junjie's patch can fix the existing issue and then drop this patch.
Here is the link.
http://www.dpdk.org/dev/patchwork/patch/36766/
thanks
Zhiyong
@@ -41,6 +41,13 @@ New Features
Also, make sure to start the actual text at the margin.
=========================================================
+* **Added support for virtio-user server mode.**
+
+ In a container environment if the vhost-user backend restarts, there's no way
+ for it to reconnect to virtio-user. To address this, support for server mode
+ is added. In this mode the socket file is created by virtio-user, which the
+ backend then connects to. This means that if the backend restarts, it can
+ reconnect to virtio-user and continue communications.
API Changes
-----------
@@ -580,6 +580,15 @@ new_device(int vid)
eth_dev->data->numa_node = newnode;
#endif
+ /* The thread may run here before eth_dev->data->rx_queues or
+ * eth_dev->data->tx_queues have gotten valid memory, so have to
+ * wait until memory allocation is done.
+ */
+ while (!eth_dev->data->rx_queues ||
+ !eth_dev->data->tx_queues) {
+ usleep(1);
+ }
+
for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
vq = eth_dev->data->rx_queues[i];
if (vq == NULL)