From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 4631E1C9D2 for ; Thu, 5 Apr 2018 10:31:31 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Apr 2018 01:31:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,410,1517904000"; d="scan'208";a="40868528" Received: from debian.sh.intel.com (HELO debian) ([10.67.104.164]) by orsmga003.jf.intel.com with ESMTP; 05 Apr 2018 01:31:27 -0700 Date: Thu, 5 Apr 2018 16:29:29 +0800 From: Tiwei Bie To: zhiyong.yang@intel.com Cc: dev@dpdk.org, maxime.coquelin@redhat.com, thomas@monjalon.net, jianfeng.tan@intel.com, zhihong.wang@intel.com, dong1.wang@intel.com Message-ID: <20180405082929.d53geeppwianqmy4@debian> References: <20180403122009.52876-2-zhiyong.yang@intel.com> <20180404171753.43422-1-zhiyong.yang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180404171753.43422-1-zhiyong.yang@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v5] net/virtio-user: add support for server mode X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Apr 2018 08:31:31 -0000 On Thu, Apr 05, 2018 at 01:17:53AM +0800, zhiyong.yang@intel.com wrote: [...] > +static int > +virtio_user_start_server(struct virtio_user_dev *dev, struct sockaddr_un *un) > +{ > + int ret; > + int flag; > + int fd = dev->listenfd; > + > + ret = bind(fd, (struct sockaddr *)un, sizeof(*un)); > + if (ret < 0) { > + PMD_DRV_LOG(ERR, "failed to bind to %s: %s; remove it and try again\n", > + dev->path, strerror(errno)); > + goto err; > + } > + ret = listen(fd, MAX_VIRTIO_USER_BACKLOG); > + if (ret < 0) > + goto err; > + > + flag = fcntl(fd, F_GETFL); > + fcntl(fd, F_SETFL, flag | O_NONBLOCK); > + dev->vhostfd = -1; > + > + return 0; > +err: > + close(dev->listenfd); The dev->listenfd isn't created in this function, maybe it's better to avoid closing this file in this function. > + return -1; > +} > + > /** > * Set up environment to talk with a vhost user backend. > * > @@ -390,6 +418,7 @@ vhost_user_setup(struct virtio_user_dev *dev) > { > int fd; > int flag; > + int ret = 0; > struct sockaddr_un un; > > fd = socket(AF_UNIX, SOCK_STREAM, 0); > @@ -405,14 +434,20 @@ vhost_user_setup(struct virtio_user_dev *dev) > memset(&un, 0, sizeof(un)); > un.sun_family = AF_UNIX; > snprintf(un.sun_path, sizeof(un.sun_path), "%s", dev->path); > - if (connect(fd, (struct sockaddr *)&un, sizeof(un)) < 0) { > - PMD_DRV_LOG(ERR, "connect error, %s", strerror(errno)); > - close(fd); > - return -1; > + > + if (dev->is_server) { > + dev->listenfd = fd; > + ret = virtio_user_start_server(dev, &un); > + } else { Maybe it's better to keep the style consistent. How about something like this: if (dev->is_server) { if (virtio_user_start_server(fd, &un) < 0) { PMD_DRV_LOG(ERR, some messages...); close(fd); return -1; } dev->listenfd = fd; dev->vhostfd = -1; } else { > + dev->vhostfd = fd; > + if (connect(fd, (struct sockaddr *)&un, sizeof(un)) < 0) { > + PMD_DRV_LOG(ERR, "connect error, %s", strerror(errno)); > + close(fd); > + return -1; > + } > } > > - dev->vhostfd = fd; > - return 0; > + return ret; > } > > static int > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c > index f90fee9e5..45e324679 100644 > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c > @@ -254,7 +254,8 @@ virtio_user_fill_intr_handle(struct virtio_user_dev *dev) > eth_dev->intr_handle->fd = -1; > if (dev->vhostfd >= 0) > eth_dev->intr_handle->fd = dev->vhostfd; > - Maybe it's better to keep this empty line (keep it before the return 0). > + else if (dev->is_server) > + eth_dev->intr_handle->fd = dev->listenfd; > return 0; > } > > @@ -267,24 +268,29 @@ virtio_user_dev_setup(struct virtio_user_dev *dev) > dev->vhostfds = NULL; > dev->tapfds = NULL; > > - 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; > + } > } > } > - There is no need to remove this empty line. > if (dev->ops->setup(dev) < 0) > return -1; > > @@ -337,16 +343,21 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, > return -1; > } > > - if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER, NULL) < 0) { > - PMD_INIT_LOG(ERR, "set_owner fails: %s", strerror(errno)); > - return -1; > - } > + if (dev->vhostfd >= 0) { > + if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER, NULL) < 0) { > + PMD_INIT_LOG(ERR, "set_owner fails: %s", strerror(errno)); > + return -1; > + } > > - if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES, > - &dev->device_features) < 0) { > - PMD_INIT_LOG(ERR, "get_features failed: %s", strerror(errno)); > - return -1; > + if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES, > + &dev->device_features) < 0) { > + PMD_INIT_LOG(ERR, "get_features failed: %s", strerror(errno)); > + return -1; > + } > + } else { > + dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES; If the backend doesn't support e.g. VIRTIO_RING_F_INDIRECT_DESC. Will it cause any problem? > } > + > if (dev->mac_specified) > dev->device_features |= (1ull << VIRTIO_NET_F_MAC); > > @@ -388,6 +399,11 @@ virtio_user_dev_uninit(struct virtio_user_dev *dev) > > close(dev->vhostfd); > > + if (dev->is_server && dev->listenfd >= 0) { > + close(dev->listenfd); > + dev->listenfd = -1; > + } > + > if (dev->vhostfds) { > for (i = 0; i < dev->max_queue_pairs; ++i) > close(dev->vhostfds[i]); > @@ -396,6 +412,9 @@ virtio_user_dev_uninit(struct virtio_user_dev *dev) > } > > free(dev->ifname); > + > + if (dev->is_server) > + unlink(dev->path); > } [...] > > static int > get_string_arg(const char *key __rte_unused, > @@ -378,10 +438,12 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev) > 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; > + uint64_t server_mode = VIRTIO_USER_DEF_SERVER_MODE; > char *path = NULL; > char *ifname = NULL; > char *mac_addr = NULL; > int ret = -1; > + struct virtio_user_dev *vu_dev = NULL; Maybe it's better to move the definition of vu_dev after eth_dev. And there isn't no need to initialize it. Thanks > > kvlist = rte_kvargs_parse(rte_vdev_device_args(dev), valid_args); > if (!kvlist) { > @@ -445,6 +507,15 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev) > } > } > > + if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_SERVER_MODE) == 1) { > + if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_SERVER_MODE, > + &get_integer_arg, &server_mode) < 0) { > + PMD_INIT_LOG(ERR, "error to parse %s", > + VIRTIO_USER_ARG_SERVER_MODE); > + goto end; > + } > + } > + > if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_CQ_NUM) == 1) { > if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_CQ_NUM, > &get_integer_arg, &cq) < 0) { > @@ -476,6 +547,11 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev) > } > > hw = eth_dev->data->dev_private; > + vu_dev = virtio_user_get_dev(hw); > + if (server_mode == 1) > + vu_dev->is_server = true; > + else > + vu_dev->is_server = false; > if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq, > queue_size, mac_addr, &ifname) < 0) { > PMD_INIT_LOG(ERR, "virtio_user_dev_init fails"); > -- > 2.14.3 >