[dpdk-dev] [PATCH v3 2/2] net/virtio: support GUEST ANNOUNCE

[Tiwei Bie]

Hi Xiao,

On Wed, Jan 03, 2018 at 11:41:40PM -0800, Xiao Wang wrote:
[...]
> +static int
> +virtio_dev_pause(struct rte_eth_dev *dev)
> +{
> +	struct virtio_hw *hw = dev->data->dev_private;
> +
> +	if (hw->started == 0)
> +		return -1;
> +	hw->started = 0;
> +	/*
> +	 * Prevent the worker thread from touching queues to avoid contention,
> +	 * 1 ms should be enough for the ongoing Tx function to finish.
> +	 */
> +	rte_delay_ms(1);
> +	return 0;
> +}
> +
> +static void
> +virtio_dev_resume(struct rte_eth_dev *dev)
> +{
> +	struct virtio_hw *hw = dev->data->dev_private;
> +
> +	hw->started = 1;
> +}

Based on your current implementation, hw->state_lock needs to
be held during a call of virtio_dev_pause()..virtio_dev_resume().
So I think the code would be more readable and much easier to
use if we take the lock in virtio_dev_pause() and release the
lock in virtio_dev_resume().

> +
> +static void
> +virtio_notify_peers(struct rte_eth_dev *dev)
> +{
> +	struct virtio_hw *hw = dev->data->dev_private;
> +	struct virtnet_tx *txvq = dev->data->tx_queues[0];
> +	struct virtnet_rx *rxvq = dev->data->rx_queues[0];
> +
> +	hw->rarp_buf[0] = rte_mbuf_raw_alloc(rxvq->mpool);
> +	if (hw->rarp_buf[0] == NULL) {
> +		PMD_DRV_LOG(ERR, "first mbuf allocate failed");
> +		return;
> +	}
> +
> +	if (make_rarp_packet(hw->rarp_buf[0],
> +				(struct ether_addr *)hw->mac_addr)) {
> +		rte_pktmbuf_free(hw->rarp_buf[0]);
> +		return;
> +	}
> +
> +	/* If virtio port just stopped, no need to send RARP */
> +	if (virtio_dev_pause(dev) < 0) {
> +		rte_pktmbuf_free(hw->rarp_buf[0]);
> +		return;
> +	}
> +
> +	dev->tx_pkt_burst(txvq, hw->rarp_buf, 1);

You have already provided virtio_dev_pause()/virtio_dev_resume().
I think you can also make this part generic and provide an inject
function, e.g.:

uint16_t
virtio_inject_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t
nb_pkts)
{
	......

	txvq->inject_pkts = tx_pkts;
	nb_tx = dev->tx_pkt_burst(txvq, tx_pkts, nb_pkts);
	txvq->inject_pkts = NULL;

	return nb_tx;
}

And you can introduce virtio_dev_pause()/virtio_dev_resume()/
virtio_injec... in a separate patch. And introduce the GUEST
ANNOUNCE support in the third patch.

Thanks,
Tiwei