diff mbox

[dpdk-dev,v3,4/4] net/vhost: add NULL pointer checking

Message ID 20180321030343.64399-5-zhiyong.yang@intel.com (mailing list archive)
State Rejected, archived
Delegated to: Maxime Coquelin
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Yang, Zhiyong March 21, 2018, 3:03 a.m. UTC 
  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

Maxime Coquelin March 29, 2018, 1:19 p.m. UTC | #1 
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/
Yang, Zhiyong March 30, 2018, 2 a.m. UTC | #2 
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/
Yang, Zhiyong March 30, 2018, 7:41 a.m. UTC | #3 
> -----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
diff mbox

Patch

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