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
