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

[Wang, Xiao W]

Hi Tiwei,

> -----Original Message-----
> From: Bie, Tiwei
> Sent: Thursday, January 4, 2018 10:51 AM
> To: Wang, Xiao W <xiao.w.wang at intel.com>
> Cc: dev at dpdk.org; yliu at fridaylinux.org; stephen at networkplumber.org
> Subject: Re: [PATCH v3 2/2] net/virtio: support GUEST ANNOUNCE
> 
> 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().

Agree, will improve it in next version.

> 
> > +
> > +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.

That would be a better patch organization, thanks! Will make a v4.

BRs,
Xiao