From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id A746E237 for ; Mon, 4 Dec 2017 09:47:19 +0100 (CET) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Dec 2017 00:47:17 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,358,1508828400"; d="scan'208";a="181017398" Received: from deepin-15.sh.intel.com (HELO debian-xvivbkq) ([10.67.104.165]) by orsmga005.jf.intel.com with ESMTP; 04 Dec 2017 00:47:04 -0800 Date: Mon, 4 Dec 2017 16:46:34 +0800 From: Tiwei Bie To: Xiao Wang Cc: yliu@fridaylinux.org, dev@dpdk.org, stephen@networkplumber.org Message-ID: <20171204084634.p6kdqpwlyau76v4a@debian-xvivbkq> References: <1511521440-57724-2-git-send-email-xiao.w.wang@intel.com> <1512396128-119985-1-git-send-email-xiao.w.wang@intel.com> <1512396128-119985-3-git-send-email-xiao.w.wang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1512396128-119985-3-git-send-email-xiao.w.wang@intel.com> User-Agent: NeoMutt/20170609 (1.8.3) Subject: Re: [dpdk-dev] [PATCH v2 2/2] net/virtio: support GUEST ANNOUNCE X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Dec 2017 08:47:20 -0000 On Mon, Dec 04, 2017 at 06:02:08AM -0800, Xiao Wang wrote: > When live migration is done, for the backup VM, either the virtio > frontend or the vhost backend needs to send out gratuitous RARP packet > to announce its new network location. > To support GUEST ANNOUNCE, do we just need to send a RARP packet? Will it work in an IPv6-only network? > This patch enables VIRTIO_NET_F_GUEST_ANNOUNCE feature to support live [...] > + > +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 condition, Typo. 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; > +} > + > +static void > +generate_rarp(struct rte_eth_dev *dev) You can give it a better name, e.g. virtio_notify_peers(). > +{ > + struct virtio_hw *hw = dev->data->dev_private; > + struct rte_mbuf *rarp_mbuf = NULL; > + struct virtnet_tx *txvq = dev->data->tx_queues[0]; > + struct virtnet_rx *rxvq = dev->data->rx_queues[0]; > + > + rarp_mbuf = rte_mbuf_raw_alloc(rxvq->mpool); > + if (rarp_mbuf == NULL) { > + PMD_DRV_LOG(ERR, "mbuf allocate failed"); > + return; > + } > + > + if (make_rarp_packet(rarp_mbuf, (struct ether_addr *)hw->mac_addr)) { > + rte_pktmbuf_free(rarp_mbuf); > + rarp_mbuf = NULL; > + return; > + } > + > + /* If virtio port just stopped, no need to send RARP */ > + if (virtio_dev_pause(dev) < 0) > + return; > + > + virtio_inject_pkts(txvq, &rarp_mbuf, 1); > + /* Recover the stored hw status to let worker thread continue */ > + virtio_dev_resume(dev); > +} > + > +static void > +virtnet_ack_link_announce(struct rte_eth_dev *dev) Why use "virtnet_" prefix? I think "virtio_" would be better. > +{ > + struct virtio_hw *hw = dev->data->dev_private; > + struct virtio_pmd_ctrl ctrl; > + int len; > + > + ctrl.hdr.class = VIRTIO_NET_CTRL_ANNOUNCE; > + ctrl.hdr.cmd = VIRTIO_NET_CTRL_ANNOUNCE_ACK; > + len = 0; > + > + virtio_send_command(hw->cvq, &ctrl, &len, 0); > +} > + > /* > - * Process Virtio Config changed interrupt and call the callback > - * if link state changed. > + * Process virtio config changed interrupt. Call the callback > + * if link state changed; generate gratuitous RARP packet if Better to replace ";" with "," > + * the status indicates an ANNOUNCE. > */ > void > virtio_interrupt_handler(void *param) > @@ -1274,6 +1391,12 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev, > NULL, NULL); > } > > + if (isr & VIRTIO_NET_S_ANNOUNCE) { > + rte_spinlock_lock(&hw->sl); > + generate_rarp(dev); Just curious. Do you need to make sure that the RARP packet would be sent successfully? > + virtnet_ack_link_announce(dev); > + rte_spinlock_unlock(&hw->sl); > + } > } [...] > diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h > index 3c5ce66..3cd367e 100644 > --- a/drivers/net/virtio/virtio_pci.h > +++ b/drivers/net/virtio/virtio_pci.h > @@ -270,6 +270,7 @@ struct virtio_hw { > struct virtio_pci_common_cfg *common_cfg; > struct virtio_net_config *dev_cfg; > void *virtio_user_dev; > + rte_spinlock_t sl; Some detailed comments need to be added in the code to document the usage of this lock. > > struct virtqueue **vqs; > }; [...] > diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h > index 2305d91..ed420e9 100644 > --- a/drivers/net/virtio/virtqueue.h > +++ b/drivers/net/virtio/virtqueue.h > @@ -158,6 +158,17 @@ struct virtio_net_ctrl_mac { > #define VIRTIO_NET_CTRL_VLAN_ADD 0 > #define VIRTIO_NET_CTRL_VLAN_DEL 1 > > +/* > + * Control link announce acknowledgement > + * > + * The command VIRTIO_NET_CTRL_ANNOUNCE_ACK is used to indicate that > + * driver has recevied the notification; device would clear the > + * VIRTIO_NET_S_ANNOUNCE bit in the status field after it receives > + * this command. > + */ > +#define VIRTIO_NET_CTRL_ANNOUNCE 3 > +#define VIRTIO_NET_CTRL_ANNOUNCE_ACK 0 You can just keep 3 and 0 in the same column. Best regards, Tiwei Bie