diff mbox

[dpdk-dev,v4,1/1] net/virtio-user: add support for server mode

Message ID 20180404053717.trp4pchameeirymv@debian (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers

Checks

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

Commit Message

Tiwei Bie April 4, 2018, 5:37 a.m. UTC 
  On Tue, Apr 03, 2018 at 11:16:26PM +0800, Tan, Jianfeng wrote:
> On 4/3/2018 8:20 PM, zhiyong.yang@intel.com wrote:
[...]
> > @@ -267,21 +270,27 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
> >   	dev->vhostfds = NULL;
> >   	dev->tapfds = NULL;
> 
> Add a check here:
> if (dev->is_server && !is_vhost_user_by_type(dev->path))
>         return error;
> 
> > -	if (is_vhost_user_by_type(dev->path)) {
> > -		dev->ops = &ops_user;
> > +	if (dev->is_server) {
> > +		dev->ops = &ops_user;/* server mode only supports vhost user*/
> >   	} else {
> > -		dev->ops = &ops_kernel;
> > -
> > -		dev->vhostfds = malloc(dev->max_queue_pairs * sizeof(int));
> > -		dev->tapfds = malloc(dev->max_queue_pairs * sizeof(int));
> > -		if (!dev->vhostfds || !dev->tapfds) {
> > -			PMD_INIT_LOG(ERR, "Failed to malloc");
> > -			return -1;
> > -		}
> > -
> > -		for (q = 0; q < dev->max_queue_pairs; ++q) {
> > -			dev->vhostfds[q] = -1;
> > -			dev->tapfds[q] = -1;
> > +		if (is_vhost_user_by_type(dev->path)) {
> > +			dev->ops = &ops_user;
> > +		} else {
> > +			dev->ops = &ops_kernel;
> > +
> > +			dev->vhostfds = malloc(dev->max_queue_pairs *
> > +					       sizeof(int));
> > +			dev->tapfds = malloc(dev->max_queue_pairs *
> > +					     sizeof(int));
> > +			if (!dev->vhostfds || !dev->tapfds) {
> > +				PMD_INIT_LOG(ERR, "Failed to malloc");
> > +				return -1;
> > +			}
> > +
> > +			for (q = 0; q < dev->max_queue_pairs; ++q) {
> > +				dev->vhostfds[q] = -1;
> > +				dev->tapfds[q] = -1;
> > +			}
> >   		}
> >   	}

Hi Zhiyong,

I think we can keep using is_vhost_user_by_type() to
determine the ops for dev->ops. And you just need to
add a check in the vhost-kernel case. Something like
this:



Thanks

Comments

Yang, Zhiyong April 4, 2018, 9:59 a.m. UTC | #1 
> -----Original Message-----

> From: Bie, Tiwei

> Sent: Wednesday, April 4, 2018 1:37 PM

> To: Yang, Zhiyong <zhiyong.yang@intel.com>

> Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; dev@dpdk.org;

> maxime.coquelin@redhat.com; thomas@monjalon.net; Wang, Zhihong

> <zhihong.wang@intel.com>

> Subject: Re: [PATCH v4 1/1] net/virtio-user: add support for server mode

> 

> On Tue, Apr 03, 2018 at 11:16:26PM +0800, Tan, Jianfeng wrote:

> > On 4/3/2018 8:20 PM, zhiyong.yang@intel.com wrote:

> [...]

> > > @@ -267,21 +270,27 @@ virtio_user_dev_setup(struct virtio_user_dev

> *dev)

> > >   	dev->vhostfds = NULL;

> > >   	dev->tapfds = NULL;

> >

> > Add a check here:

> > if (dev->is_server && !is_vhost_user_by_type(dev->path))

> >         return error;

> >

> > > -	if (is_vhost_user_by_type(dev->path)) {

> > > -		dev->ops = &ops_user;

> > > +	if (dev->is_server) {

> > > +		dev->ops = &ops_user;/* server mode only supports vhost

> user*/

> > >   	} else {

> > > -		dev->ops = &ops_kernel;

> > > -

> > > -		dev->vhostfds = malloc(dev->max_queue_pairs *

> sizeof(int));

> > > -		dev->tapfds = malloc(dev->max_queue_pairs * sizeof(int));

> > > -		if (!dev->vhostfds || !dev->tapfds) {

> > > -			PMD_INIT_LOG(ERR, "Failed to malloc");

> > > -			return -1;

> > > -		}

> > > -

> > > -		for (q = 0; q < dev->max_queue_pairs; ++q) {

> > > -			dev->vhostfds[q] = -1;

> > > -			dev->tapfds[q] = -1;

> > > +		if (is_vhost_user_by_type(dev->path)) {

> > > +			dev->ops = &ops_user;

> > > +		} else {

> > > +			dev->ops = &ops_kernel;

> > > +

> > > +			dev->vhostfds = malloc(dev->max_queue_pairs *

> > > +					       sizeof(int));

> > > +			dev->tapfds = malloc(dev->max_queue_pairs *

> > > +					     sizeof(int));

> > > +			if (!dev->vhostfds || !dev->tapfds) {

> > > +				PMD_INIT_LOG(ERR, "Failed to malloc");

> > > +				return -1;

> > > +			}

> > > +

> > > +			for (q = 0; q < dev->max_queue_pairs; ++q) {

> > > +				dev->vhostfds[q] = -1;

> > > +				dev->tapfds[q] = -1;

> > > +			}

> > >   		}

> > >   	}

> 

> Hi Zhiyong,

> 

> I think we can keep using is_vhost_user_by_type() to determine the ops for

> dev->ops. And you just need to add a check in the vhost-kernel case.

> Something like

> this:

> 

> --- i/drivers/net/virtio/virtio_user/virtio_user_dev.c

> +++ w/drivers/net/virtio/virtio_user/virtio_user_dev.c

> @@ -270,6 +270,9 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)

>  	if (is_vhost_user_by_type(dev->path)) {

>  		dev->ops = &ops_user;

>  	} else {

> +		if (dev->is_server)

> +			return -1;

> +

>  		dev->ops = &ops_kernel;

> 

>  		dev->vhostfds = malloc(dev->max_queue_pairs *

> sizeof(int));

> 


Ok, thanks, tiwei.
Yang, Zhiyong April 4, 2018, 2:57 p.m. UTC | #2 
> -----Original Message-----

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yang, Zhiyong

> Sent: Wednesday, April 4, 2018 5:59 PM

> To: Bie, Tiwei <tiwei.bie@intel.com>

> Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; dev@dpdk.org;

> maxime.coquelin@redhat.com; thomas@monjalon.net; Wang, Zhihong

> <zhihong.wang@intel.com>

> Subject: Re: [dpdk-dev] [PATCH v4 1/1] net/virtio-user: add support for

> server mode

> 

> 

> 

> > -----Original Message-----

> > From: Bie, Tiwei

> > Sent: Wednesday, April 4, 2018 1:37 PM

> > To: Yang, Zhiyong <zhiyong.yang@intel.com>

> > Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; dev@dpdk.org;

> > maxime.coquelin@redhat.com; thomas@monjalon.net; Wang, Zhihong

> > <zhihong.wang@intel.com>

> > Subject: Re: [PATCH v4 1/1] net/virtio-user: add support for server

> > mode

> >

> > On Tue, Apr 03, 2018 at 11:16:26PM +0800, Tan, Jianfeng wrote:

> > > On 4/3/2018 8:20 PM, zhiyong.yang@intel.com wrote:

> > [...]

> > > > @@ -267,21 +270,27 @@ virtio_user_dev_setup(struct virtio_user_dev

> > *dev)

> > > >   	dev->vhostfds = NULL;

> > > >   	dev->tapfds = NULL;

> > >

> > > Add a check here:

> > > if (dev->is_server && !is_vhost_user_by_type(dev->path))

> > >         return error;

> > >

> > > > -	if (is_vhost_user_by_type(dev->path)) {

> > > > -		dev->ops = &ops_user;

> > > > +	if (dev->is_server) {

> > > > +		dev->ops = &ops_user;/* server mode only supports vhost

> > user*/

> > > >   	} else {

> > > > -		dev->ops = &ops_kernel;

> > > > -

> > > > -		dev->vhostfds = malloc(dev->max_queue_pairs *

> > sizeof(int));

> > > > -		dev->tapfds = malloc(dev->max_queue_pairs * sizeof(int));

> > > > -		if (!dev->vhostfds || !dev->tapfds) {

> > > > -			PMD_INIT_LOG(ERR, "Failed to malloc");

> > > > -			return -1;

> > > > -		}

> > > > -

> > > > -		for (q = 0; q < dev->max_queue_pairs; ++q) {

> > > > -			dev->vhostfds[q] = -1;

> > > > -			dev->tapfds[q] = -1;

> > > > +		if (is_vhost_user_by_type(dev->path)) {

> > > > +			dev->ops = &ops_user;

> > > > +		} else {

> > > > +			dev->ops = &ops_kernel;

> > > > +

> > > > +			dev->vhostfds = malloc(dev->max_queue_pairs *

> > > > +					       sizeof(int));

> > > > +			dev->tapfds = malloc(dev->max_queue_pairs *

> > > > +					     sizeof(int));

> > > > +			if (!dev->vhostfds || !dev->tapfds) {

> > > > +				PMD_INIT_LOG(ERR, "Failed to malloc");

> > > > +				return -1;

> > > > +			}

> > > > +

> > > > +			for (q = 0; q < dev->max_queue_pairs; ++q) {

> > > > +				dev->vhostfds[q] = -1;

> > > > +				dev->tapfds[q] = -1;

> > > > +			}

> > > >   		}

> > > >   	}

> >

> > Hi Zhiyong,

> >

> > I think we can keep using is_vhost_user_by_type() to determine the ops

> > for

> > dev->ops. And you just need to add a check in the vhost-kernel case.

> > Something like

> > this:

> >

> > --- i/drivers/net/virtio/virtio_user/virtio_user_dev.c

> > +++ w/drivers/net/virtio/virtio_user/virtio_user_dev.c

> > @@ -270,6 +270,9 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)

> >  	if (is_vhost_user_by_type(dev->path)) {

> >  		dev->ops = &ops_user;

> >  	} else {

> > +		if (dev->is_server)

> > +			return -1;

> > +

> >  		dev->ops = &ops_kernel;

> >

> >  		dev->vhostfds = malloc(dev->max_queue_pairs *

> sizeof(int));


Tiwei, Jianfeng,
Think again, is_vhost_user_by_type() does not help judge user space or kernel driver for server mode.
As no socket file exists here for both.

Thanks
Zhiyong
diff mbox

Patch

--- i/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ w/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -270,6 +270,9 @@  virtio_user_dev_setup(struct virtio_user_dev *dev)
 	if (is_vhost_user_by_type(dev->path)) {
 		dev->ops = &ops_user;
 	} else {
+		if (dev->is_server)
+			return -1;
+
 		dev->ops = &ops_kernel;
 
 		dev->vhostfds = malloc(dev->max_queue_pairs * sizeof(int));