From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 23585378E for ; Fri, 20 Nov 2015 12:41:46 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP; 20 Nov 2015 03:41:46 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,322,1444719600"; d="scan'208";a="855716583" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49]) by fmsmga002.fm.intel.com with ESMTP; 20 Nov 2015 03:41:45 -0800 Date: Fri, 20 Nov 2015 19:43:04 +0800 From: Yuanhan Liu To: Tetsuya Mukawa Message-ID: <20151120114304.GB2325@yliu-dev.sh.intel.com> References: <1447046221-20811-3-git-send-email-mukawa@igel.co.jp> <1447392031-24970-1-git-send-email-mukawa@igel.co.jp> <1447392031-24970-3-git-send-email-mukawa@igel.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1447392031-24970-3-git-send-email-mukawa@igel.co.jp> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: dev@dpdk.org, ann.zhuangyanying@huawei.com Subject: Re: [dpdk-dev] [PATCH v4 2/2] vhost: Add VHOST PMD X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 Nov 2015 11:41:47 -0000 On Fri, Nov 13, 2015 at 02:20:31PM +0900, Tetsuya Mukawa wrote: .... > +static pthread_mutex_t internal_list_lock = PTHREAD_MUTEX_INITIALIZER; > + > +static rte_atomic16_t nb_started_ports; > +pthread_t session_th; static? > + > +static struct rte_eth_link pmd_link = { > + .link_speed = 10000, > + .link_duplex = ETH_LINK_FULL_DUPLEX, > + .link_status = 0 > +}; > + > +static uint16_t > +eth_vhost_rx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs) > +{ > + struct vhost_queue *r = q; > + uint16_t nb_rx = 0; > + > + if (unlikely(rte_atomic32_read(&r->allow_queuing) == 0)) > + return 0; > + > + rte_atomic32_set(&r->while_queuing, 1); > + > + if (unlikely(rte_atomic32_read(&r->allow_queuing) == 0)) > + goto out; > + > + /* Dequeue packets from guest TX queue */ > + nb_rx = rte_vhost_dequeue_burst(r->device, > + r->virtqueue_id, r->mb_pool, bufs, nb_bufs); > + > + r->rx_pkts += nb_rx; > + > +out: > + rte_atomic32_set(&r->while_queuing, 0); > + > + return nb_rx; > +} > + > +static uint16_t > +eth_vhost_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs) > +{ > + struct vhost_queue *r = q; > + uint16_t i, nb_tx = 0; > + > + if (unlikely(rte_atomic32_read(&r->allow_queuing) == 0)) > + return 0; > + > + rte_atomic32_set(&r->while_queuing, 1); > + > + if (unlikely(rte_atomic32_read(&r->allow_queuing) == 0)) > + goto out; > + > + /* Enqueue packets to guest RX queue */ > + nb_tx = rte_vhost_enqueue_burst(r->device, > + r->virtqueue_id, bufs, nb_bufs); > + > + r->tx_pkts += nb_tx; > + r->err_pkts += nb_bufs - nb_tx; > + > + for (i = 0; likely(i < nb_tx); i++) > + rte_pktmbuf_free(bufs[i]); We should free upto nb_bufs here, but not nb_tx, right? > + > +out: > + rte_atomic32_set(&r->while_queuing, 0); > + > + return nb_tx; > +} > + > +static int > +eth_dev_configure(struct rte_eth_dev *dev __rte_unused) > +{ > + return 0; > +} > + > +static inline struct pmd_internal * > +find_internal_resource(char *ifname) > +{ > + int found = 0; > + struct pmd_internal *internal; > + > + if (ifname == NULL) > + return NULL; > + > + pthread_mutex_lock(&internal_list_lock); > + > + TAILQ_FOREACH(internal, &internals_list, next) { > + if (!strcmp(internal->iface_name, ifname)) { > + found = 1; > + break; > + } > + } > + > + pthread_mutex_unlock(&internal_list_lock); > + > + if (!found) > + return NULL; > + > + return internal; > +} > + ... > +static void *vhost_driver_session(void *param __rte_unused) static void * vhost_driver_session_start(..) > +{ > + static struct virtio_net_device_ops *vhost_ops; > + > + vhost_ops = rte_zmalloc(NULL, sizeof(*vhost_ops), 0); > + if (vhost_ops == NULL) > + rte_panic("Can't allocate memory\n"); Why not making them static? > + > + /* set vhost arguments */ > + vhost_ops->new_device = new_device; > + vhost_ops->destroy_device = destroy_device; > + if (rte_vhost_driver_pmd_callback_register(vhost_ops) < 0) > + rte_panic("Can't register callbacks\n"); > + > + /* start event handling */ > + rte_vhost_driver_session_start(); > + > + rte_free(vhost_ops); > + pthread_exit(0); > +} > + > +static void vhost_driver_session_start(void) ditto. > +{ > + int ret; > + > + ret = pthread_create(&session_th, > + NULL, vhost_driver_session, NULL); > + if (ret) > + rte_panic("Can't create a thread\n"); > +} > + > +static void vhost_driver_session_stop(void) Ditto. > +{ > + int ret; > + > + ret = pthread_cancel(session_th); > + if (ret) > + rte_panic("Can't cancel the thread\n"); > + > + ret = pthread_join(session_th, NULL); > + if (ret) > + rte_panic("Can't join the thread\n"); > +} > + > +static int > +eth_dev_start(struct rte_eth_dev *dev) ... > + internal->nb_rx_queues = queues; > + internal->nb_tx_queues = queues; > + internal->dev_name = strdup(name); > + if (internal->dev_name == NULL) > + goto error; > + internal->iface_name = strdup(iface_name); > + if (internal->iface_name == NULL) { > + free(internal->dev_name); > + goto error; > + } You still didn't resolve my comments from last email: if allocation failed here, internal->dev_name is not freed. > + > + pthread_mutex_lock(&internal_list_lock); > + TAILQ_INSERT_TAIL(&internals_list, internal, next); > + pthread_mutex_unlock(&internal_list_lock); > + ... > +static struct rte_driver pmd_vhost_drv = { > + .name = "eth_vhost", > + .type = PMD_VDEV, > + .init = rte_pmd_vhost_devinit, > + .uninit = rte_pmd_vhost_devuninit, > +}; > + > +struct > +virtio_net *rte_eth_vhost_portid2vdev(uint16_t port_id) struct virtio_net * rte_eth_vhost_portid2vdev() BTW, why making a speical eth API for virtio? This doesn't make too much sense to me. Besides those minor nits, this patch looks good to me. Thanks for the work! --yliu