DPDK patches and discussions
 help / color / mirror / Atom feed
From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: "Wang, Yinan" <yinan.wang@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"Xia, Chenbo" <chenbo.xia@intel.com>,
	"Fu, Patrick" <patrick.fu@intel.com>,
	"amorenoz@redhat.com" <amorenoz@redhat.com>
Subject: Re: [dpdk-dev] [PATCH v3 3/8] net/virtio: move backend type selection to ethdev
Date: Tue, 20 Oct 2020 09:11:31 +0200	[thread overview]
Message-ID: <d0220345-bf71-0ab3-9191-22a321c99f14@redhat.com> (raw)
In-Reply-To: <BYAPR11MB2648B727C393203203893B208F1F0@BYAPR11MB2648.namprd11.prod.outlook.com>

Hi Yinan,

On 10/20/20 3:52 AM, Wang, Yinan wrote:
> Hi Adrian/Maxime,
> 
> Could you help to take a look at this issue? Most of automated regression cases will be blocked due to this issue in 20.11 RC1 test.

We are working on it. Adrian managed to reproduce it, and fixed one
part of the issue. We''l continue the debugging today.

BTW, you are mentionning automated regression tests, any chances these
test land in the Intel functionnal CI at some point? I would be great to
catch these issues before the series are merged.

Thanks!
Maxime

> Thanks in advance!
> 
> BR,
> Yinan
> 
>> -----Original Message-----
>> From: Wang, Yinan
>> Sent: 2020?10?16? 13:42
>> To: Maxime Coquelin <maxime.coquelin@redhat.com>; dev@dpdk.org; Xia,
>> Chenbo <Chenbo.Xia@intel.com>; Fu, Patrick <patrick.fu@intel.com>;
>> amorenoz@redhat.com
>> Subject: RE: [dpdk-dev] [PATCH v3 3/8] net/virtio: move backend type selection
>> to ethdev
>>
>> Hi Adrian,
>>
>> This patch introduce a regression issue that vhost-user can't launch with server
>> mode, details in https://bugs.dpdk.org/show_bug.cgi?id=559.
>> This issue blocked amount of regression cases, could you help to take a look?
>> Thanks in advance!
>>
>>
>> BR,
>> Yinan
>>
>>
>>> -----Original Message-----
>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Maxime Coquelin
>>> Sent: 2020?9?30? 0:14
>>> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Fu, Patrick
>>> <patrick.fu@intel.com>; amorenoz@redhat.com
>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Subject: [dpdk-dev] [PATCH v3 3/8] net/virtio: move backend type selection to
>>> ethdev
>>>
>>> From: Adrian Moreno <amorenoz@redhat.com>
>>>
>>> This is a preparation patch with no functional change.
>>>
>>> Use an enum instead of a boolean for the backend type.
>>> Move the detection logic to the ethdev layer (where it is needed for the
>>> first time).
>>> The virtio_user_dev stores the backend type in the virtio_user_dev
>>> struct so the type is only determined once
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>> ---
>>>  .../net/virtio/virtio_user/virtio_user_dev.c  | 37 ++++++++-----------
>>>  .../net/virtio/virtio_user/virtio_user_dev.h  | 11 +++++-
>>>  drivers/net/virtio/virtio_user_ethdev.c       | 28 +++++++++++++-
>>>  3 files changed, 50 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> index 2a0c861085..b79a9f84aa 100644
>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> @@ -111,17 +111,6 @@ virtio_user_queue_setup(struct virtio_user_dev *dev,
>>>  	return 0;
>>>  }
>>>
>>> -int
>>> -is_vhost_user_by_type(const char *path)
>>> -{
>>> -	struct stat sb;
>>> -
>>> -	if (stat(path, &sb) == -1)
>>> -		return 0;
>>> -
>>> -	return S_ISSOCK(sb.st_mode);
>>> -}
>>> -
>>>  int
>>>  virtio_user_start_device(struct virtio_user_dev *dev)
>>>  {
>>> @@ -144,7 +133,8 @@ virtio_user_start_device(struct virtio_user_dev *dev)
>>>  	rte_mcfg_mem_read_lock();
>>>  	pthread_mutex_lock(&dev->mutex);
>>>
>>> -	if (is_vhost_user_by_type(dev->path) && dev->vhostfd < 0)
>>> +	if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER &&
>>> +			dev->vhostfd < 0)
>>>  		goto error;
>>>
>>>  	/* Step 0: tell vhost to create queues */
>>> @@ -360,16 +350,16 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
>>>  	dev->tapfds = NULL;
>>>
>>>  	if (dev->is_server) {
>>> -		if (access(dev->path, F_OK) == 0 &&
>>> -		    !is_vhost_user_by_type(dev->path)) {
>>> -			PMD_DRV_LOG(ERR, "Server mode doesn't support
>>> vhost-kernel!");
>>> +		if (dev->backend_type !=
>>> VIRTIO_USER_BACKEND_VHOST_USER) {
>>> +			PMD_DRV_LOG(ERR, "Server mode only supports
>>> vhost-user!");
>>>  			return -1;
>>>  		}
>>>  		dev->ops = &virtio_ops_user;
>>>  	} else {
>>> -		if (is_vhost_user_by_type(dev->path)) {
>>> +		if (dev->backend_type ==
>>> VIRTIO_USER_BACKEND_VHOST_USER) {
>>>  			dev->ops = &virtio_ops_user;
>>> -		} else {
>>> +		} else if (dev->backend_type ==
>>> +
>>> 	VIRTIO_USER_BACKEND_VHOST_KERNEL) {
>>>  			dev->ops = &virtio_ops_kernel;
>>>
>>>  			dev->vhostfds = malloc(dev->max_queue_pairs *
>>> @@ -430,7 +420,8 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
>>>  int
>>>  virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
>>>  		     int cq, int queue_size, const char *mac, char **ifname,
>>> -		     int server, int mrg_rxbuf, int in_order, int packed_vq)
>>> +		     int server, int mrg_rxbuf, int in_order, int packed_vq,
>>> +		     enum virtio_user_backend_type backend_type)
>>>  {
>>>  	uint64_t protocol_features = 0;
>>>
>>> @@ -445,6 +436,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>>> *path, int queues,
>>>  	dev->frontend_features = 0;
>>>  	dev->unsupported_features = ~VIRTIO_USER_SUPPORTED_FEATURES;
>>>  	dev->protocol_features =
>>> VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES;
>>> +	dev->backend_type = backend_type;
>>> +
>>>  	parse_mac(dev, mac);
>>>
>>>  	if (*ifname) {
>>> @@ -457,7 +450,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>>> *path, int queues,
>>>  		return -1;
>>>  	}
>>>
>>> -	if (!is_vhost_user_by_type(dev->path))
>>> +	if (dev->backend_type != VIRTIO_USER_BACKEND_VHOST_USER)
>>>  		dev->unsupported_features |=
>>>  			(1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
>>>
>>> @@ -539,7 +532,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>>> *path, int queues,
>>>  	}
>>>
>>>  	/* The backend will not report this feature, we add it explicitly */
>>> -	if (is_vhost_user_by_type(dev->path))
>>> +	if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER)
>>>  		dev->frontend_features |= (1ull << VIRTIO_NET_F_STATUS);
>>>
>>>  	/*
>>> @@ -792,7 +785,7 @@ virtio_user_send_status_update(struct virtio_user_dev
>>> *dev, uint8_t status)
>>>  	uint64_t arg = status;
>>>
>>>  	/* Vhost-user only for now */
>>> -	if (!is_vhost_user_by_type(dev->path))
>>> +	if (dev->backend_type != VIRTIO_USER_BACKEND_VHOST_USER)
>>>  		return 0;
>>>
>>>  	if (!(dev->protocol_features & (1ULL <<
>>> VHOST_USER_PROTOCOL_F_STATUS)))
>>> @@ -815,7 +808,7 @@ virtio_user_update_status(struct virtio_user_dev *dev)
>>>  	int err;
>>>
>>>  	/* Vhost-user only for now */
>>> -	if (!is_vhost_user_by_type(dev->path))
>>> +	if (dev->backend_type != VIRTIO_USER_BACKEND_VHOST_USER)
>>>  		return 0;
>>>
>>>  	if (!(dev->protocol_features & (1UL <<
>>> VHOST_USER_PROTOCOL_F_STATUS)))
>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>>> index 9377d5ba66..575bf430c0 100644
>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>>> @@ -10,6 +10,12 @@
>>>  #include "../virtio_pci.h"
>>>  #include "../virtio_ring.h"
>>>
>>> +enum virtio_user_backend_type {
>>> +	VIRTIO_USER_BACKEND_UNKNOWN,
>>> +	VIRTIO_USER_BACKEND_VHOST_USER,
>>> +	VIRTIO_USER_BACKEND_VHOST_KERNEL,
>>> +};
>>> +
>>>  struct virtio_user_queue {
>>>  	uint16_t used_idx;
>>>  	bool avail_wrap_counter;
>>> @@ -17,6 +23,7 @@ struct virtio_user_queue {
>>>  };
>>>
>>>  struct virtio_user_dev {
>>> +	enum virtio_user_backend_type backend_type;
>>>  	/* for vhost_user backend */
>>>  	int		vhostfd;
>>>  	int		listenfd;   /* listening fd */
>>> @@ -60,13 +67,13 @@ struct virtio_user_dev {
>>>  	bool		started;
>>>  };
>>>
>>> -int is_vhost_user_by_type(const char *path);
>>>  int virtio_user_start_device(struct virtio_user_dev *dev);
>>>  int virtio_user_stop_device(struct virtio_user_dev *dev);
>>>  int virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
>>>  			 int cq, int queue_size, const char *mac, char **ifname,
>>>  			 int server, int mrg_rxbuf, int in_order,
>>> -			 int packed_vq);
>>> +			 int packed_vq,
>>> +			 enum virtio_user_backend_type backend_type);
>>>  void virtio_user_dev_uninit(struct virtio_user_dev *dev);
>>>  void virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx);
>>>  void virtio_user_handle_cq_packed(struct virtio_user_dev *dev,
>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>>> b/drivers/net/virtio/virtio_user_ethdev.c
>>> index 60d17af888..38b49bad5f 100644
>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>> @@ -6,6 +6,7 @@
>>>  #include <sys/types.h>
>>>  #include <unistd.h>
>>>  #include <fcntl.h>
>>> +#include <sys/stat.h>
>>>  #include <sys/socket.h>
>>>
>>>  #include <rte_malloc.h>
>>> @@ -518,6 +519,19 @@ get_integer_arg(const char *key __rte_unused,
>>>  	return -errno;
>>>  }
>>>
>>> +static enum virtio_user_backend_type
>>> +virtio_user_backend_type(const char *path)
>>> +{
>>> +	struct stat sb;
>>> +
>>> +	if (stat(path, &sb) == -1)
>>> +		return VIRTIO_USER_BACKEND_UNKNOWN;
>>> +
>>> +	return S_ISSOCK(sb.st_mode) ?
>>> +		VIRTIO_USER_BACKEND_VHOST_USER :
>>> +		VIRTIO_USER_BACKEND_VHOST_KERNEL;
>>> +}
>>> +
>>>  static struct rte_eth_dev *
>>>  virtio_user_eth_dev_alloc(struct rte_vdev_device *vdev)
>>>  {
>>> @@ -579,6 +593,7 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
>>>  	struct rte_kvargs *kvlist = NULL;
>>>  	struct rte_eth_dev *eth_dev;
>>>  	struct virtio_hw *hw;
>>> +	enum virtio_user_backend_type backend_type =
>>> VIRTIO_USER_BACKEND_UNKNOWN;
>>>  	uint64_t queues = VIRTIO_USER_DEF_Q_NUM;
>>>  	uint64_t cq = VIRTIO_USER_DEF_CQ_EN;
>>>  	uint64_t queue_size = VIRTIO_USER_DEF_Q_SZ;
>>> @@ -631,8 +646,17 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
>>>  		goto end;
>>>  	}
>>>
>>> +	backend_type = virtio_user_backend_type(path);
>>> +	if (backend_type == VIRTIO_USER_BACKEND_UNKNOWN) {
>>> +		PMD_INIT_LOG(ERR,
>>> +			     "unable to determine backend type for path %s",
>>> +			path);
>>> +		goto end;
>>> +	}
>>> +
>>> +
>>>  	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_INTERFACE_NAME) == 1)
>>> {
>>> -		if (is_vhost_user_by_type(path)) {
>>> +		if (backend_type != VIRTIO_USER_BACKEND_VHOST_KERNEL) {
>>>  			PMD_INIT_LOG(ERR,
>>>  				"arg %s applies only to vhost-kernel backend",
>>>  				VIRTIO_USER_ARG_INTERFACE_NAME);
>>> @@ -751,7 +775,7 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
>>>  	hw = eth_dev->data->dev_private;
>>>  	if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
>>>  			 queue_size, mac_addr, &ifname, server_mode,
>>> -			 mrg_rxbuf, in_order, packed_vq) < 0) {
>>> +			 mrg_rxbuf, in_order, packed_vq, backend_type) < 0) {
>>>  		PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
>>>  		virtio_user_eth_dev_free(eth_dev);
>>>  		goto end;
>>> --
>>> 2.26.2
> 


  reply	other threads:[~2020-10-20  7:11 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-29 16:13 [dpdk-dev] [PATCH v3 0/8] virtio-user: introduce vhost-vdpa backend Maxime Coquelin
2020-09-29 16:13 ` [dpdk-dev] [PATCH v3 1/8] bus/vdev: add DMA mapping ops Maxime Coquelin
2020-09-30  5:47   ` Xia, Chenbo
2020-09-29 16:13 ` [dpdk-dev] [PATCH v3 2/8] net/virtio: introduce DMA ops Maxime Coquelin
2020-09-30  5:47   ` Xia, Chenbo
2020-09-29 16:13 ` [dpdk-dev] [PATCH v3 3/8] net/virtio: move backend type selection to ethdev Maxime Coquelin
2020-09-30  5:49   ` Xia, Chenbo
2020-10-16  5:42   ` Wang, Yinan
2020-10-16  5:58     ` Wang, Yinan
2020-10-20  1:52     ` Wang, Yinan
2020-10-20  7:11       ` Maxime Coquelin [this message]
2020-10-20  7:20         ` Adrian Moreno
2020-09-29 16:14 ` [dpdk-dev] [PATCH v3 4/8] net/virtio: introduce Vhost-vDPA backend type Maxime Coquelin
2020-09-30  5:49   ` Xia, Chenbo
2020-09-29 16:14 ` [dpdk-dev] [PATCH v3 5/8] net/virtio: check protocol feature in user backend Maxime Coquelin
2020-09-30  5:49   ` Xia, Chenbo
2020-09-29 16:14 ` [dpdk-dev] [PATCH v3 6/8] net/virtio: adapt Virtio-user status size Maxime Coquelin
2020-09-30  5:49   ` Xia, Chenbo
2020-09-29 16:14 ` [dpdk-dev] [PATCH v3 7/8] net/virtio: split virtio-user start Maxime Coquelin
2020-09-30  5:49   ` Xia, Chenbo
2020-09-29 16:14 ` [dpdk-dev] [PATCH v3 8/8] net/virtio: introduce Vhost-vDPA backend Maxime Coquelin
2020-09-30  5:49   ` Xia, Chenbo
2020-09-30 16:22 ` [dpdk-dev] [PATCH v3 0/8] virtio-user: introduce vhost-vdpa backend Maxime Coquelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d0220345-bf71-0ab3-9191-22a321c99f14@redhat.com \
    --to=maxime.coquelin@redhat.com \
    --cc=amorenoz@redhat.com \
    --cc=chenbo.xia@intel.com \
    --cc=dev@dpdk.org \
    --cc=patrick.fu@intel.com \
    --cc=yinan.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).