Hi Tiwei, > -----Original Message----- > From: Bie, Tiwei > Sent: Thursday, January 4, 2018 10:51 AM > To: Wang, Xiao W <xiao.w.w...@intel.com> > Cc: dev@dpdk.org; y...@fridaylinux.org; step...@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