Re: [PATCH v4 2/2] vhost: Add VHOST PMD

[Tetsuya Mukawa <mukawa@igel.co.jp>]

On 2015/11/21 9:15, Rich Lane wrote:
> On Thu, Nov 12, 2015 at 9:20 PM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
>
>> +static uint16_t
>> +eth_vhost_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
>> +{
>>
> ...
>
>> +
>> +       /* 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;
>>
> I don't think a full TX queue is counted as an error by physical NIC PMDs
> like ixgbe and i40e. It is counted as an error by the af_packet, pcap, and
> ring PMDs. I'd suggest not counting it as an error because it's a common
> and expected condition, and the application might just retry the TX later.

Hi Rich,

Thanks for commenting.
I will count it as "imissed".

> Are the byte counts left out because it would be a performance hit? It
> seems like it would be a minimal cost given how much we're already touching
> each packet.

I just ignore it for performance.
But you are correct, I will add it.

>
>> +static int
>> +new_device(struct virtio_net *dev)
>> +{
>>
> ...
>
>> +
>> +       if ((dev->virt_qp_nb < internal->nb_rx_queues) ||
>> +                       (dev->virt_qp_nb < internal->nb_tx_queues)) {
>> +               RTE_LOG(INFO, PMD, "Not enough queues\n");
>> +               return -1;
>> +       }
>>
> Would it make sense to take the minimum of the guest and host queuepairs
> and use that below in place of nb_rx_queues/nb_tx_queues? That way the host
> can support a large maximum number of queues and each guest can choose how
> many it wants to use. The host application will receive vring_state_changed
> callbacks for each queue the guest activates.
>

Thanks for checking this.
I agree with you.

After reading your comment, here is my guess for this PMD behavior.

This PMD should assume that virtio-net device(QEMU) has same or more
queues than specified in vhost PMD option.
In a case that the assumption is break, application should handle
vring_state_change callback correctly.
(Then stop accessing to disabled queues not to spend CPU power.)

Anyway, I will just remove above if-condition, because of above PMD
assumption.

Thanks,
Tetsuya