[dpdk-dev] [PATCH v4 2/2] vhost: Add VHOST PMD

Yuanhan Liu yuanhan.liu at linux.intel.com
Fri Nov 20 12:43:04 CET 2015


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


More information about the dev mailing list