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

[Yuanhan Liu]

On Tue, Nov 24, 2015 at 11:48:04AM +0900, Tetsuya Mukawa wrote:
> On 2015/11/20 20:43, Yuanhan Liu wrote:
> > 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?
> 
> Hi Yuanhan,
> 
> I appreciate your carefully reviewing.
> I will fix issues you commented, and submit it again.
> 
> I added below 2 comments.
> Could you please check it?
> 
> >> +
> >> +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?
> 
> I guess we don't need to free all packet buffers.
> Could you please check l2fwd_send_burst() in l2fwd example?
> It seems DPDK application frees packet buffers that failed to send.

Yes, you are right. I was thinking it's just a vhost app, and forgot
that this is for rte_eth_tx_burst, sigh ...

> 
> >> +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.
> 
> This is a kind of helper function.

Yeah, I know that. I was thinking that an API prefixed with rte_eth_
should be a common interface for all eth drivers. Here this one is
for vhost PMD only, though.

I then had a quick check of DPDK code, and found a similar example,
bond, such as rte_eth_bond_create(). So, it might be okay to introduce
PMD specific eth APIs?

Anyway, I would suggest you to put it into another patch, so that
it can be reworked (or even dropped) if someone else doesn't like
it (or doesn't think it's necessary).

	--yliu

> 
> I assume that DPDK applications want to know relation between port_id
> and virtio device structure.
> But, in "new" callback handler that DPDK application registers,
> application can receive virtio device structure, but it doesn't tell
> which port is.
> 
> To know it, probably here are steps that DPDK application needs to do.
> 
> 1. Store interface name that is specified when vhost pmd is invoked.
> (For example, store information like /tmp/socket0 is for port0, and
> /tmp/socket1 is for port1)
> 2. Compare above interface name and dev->ifname that is stored in virtio
> device structure, then DPDK application can know which port is.
> 
> If DPDK application uses Port Hotplug, I guess above steps are easy.
> But if they don't, interface name will be specified in "--vdev" EAL
> command line option.
> So probably it's not so easy to handle interface name in DPDK application.
> This is why I added the function.
> 
> Thanks,
> Tetsuya