From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 10C9537B4 for ; Mon, 25 Sep 2017 04:33:38 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Sep 2017 19:33:37 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,434,1500966000"; d="scan'208";a="903461338" Received: from unknown (HELO localhost.localdomain) ([10.240.224.185]) by FMSMGA003.fm.intel.com with ESMTP; 24 Sep 2017 19:33:36 -0700 Date: Mon, 25 Sep 2017 10:29:25 +0800 From: "Yang, Yi" To: "Tan, Jianfeng" Cc: "yliu@fridaylinux.org" , "dev@dpdk.org" Message-ID: <20170925022924.GA106755@localhost.localdomain> References: <1505896323-125943-1-git-send-email-yi.y.yang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] vhost: fix vhost_user_set_mem_table error 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, 25 Sep 2017 02:33:40 -0000 On Fri, Sep 22, 2017 at 01:28:48PM +0800, Tan, Jianfeng wrote: > > > > + } > > + reg->guest_user_addr = 0; > > + j++; > > + } else { > > + i++; > > + j++; > > + } > > + } > > The algorithm here could be problematic for hot plug. For example, > > T0: we have two regions with address 1, 3. > T1: we add another region, 2, which makes the regions as 1, 2, 3. > > 1st iteration, 1 matches 1, i++, j++; > 2nd iteration, 2 does not match 3, unmap 3, j++. Jianfeng, thank you so much for your comments, but per my understanding such hot add is impossible, this will result in discontinuous physical address. Do you know how we can trigger such use case? > > > + > > + /* munmap these regions because they have been hot > > unplugged */ > > + for (; j < dev->mem->nregions; j++) { > > + reg = &dev->mem->regions[j]; > > + if (reg->host_user_addr) { > > + munmap(reg->mmap_addr, reg- > > >mmap_size); > > + close(reg->fd); > > + } > > + reg->guest_user_addr = 0; > > + } > > } > > > > - dev->nr_guest_pages = 0; > > if (!dev->guest_pages) { > > + dev->nr_guest_pages = 0; > > We cannot just move this line here, which results in a wrong nr_guest_pages if we do hot plug/unplug instead of initialization. For initialization, dev->guest_pages is 0, for hot plug/unplug, dev->guest_pages isn't 0, so it is ok. > > > dev->max_guest_pages = 8; > > dev->guest_pages = malloc(dev->max_guest_pages * > > sizeof(struct guest_page)); > > @@ -532,7 +558,7 @@ vhost_user_set_mem_table(struct virtio_net *dev, > > struct VhostUserMsg *pmsg) > > } > > } > > > > - dev->mem = rte_zmalloc("vhost-mem-table", sizeof(struct > > rte_vhost_memory) + > > + dev->mem = rte_realloc(dev->mem, sizeof(struct > > rte_vhost_memory) + > > sizeof(struct rte_vhost_mem_region) * memory.nregions, 0); > > if (dev->mem == NULL) { > > RTE_LOG(ERR, VHOST_CONFIG, > > @@ -546,6 +572,38 @@ vhost_user_set_mem_table(struct virtio_net *dev, > > struct VhostUserMsg *pmsg) > > fd = pmsg->fds[i]; > > reg = &dev->mem->regions[i]; > > > > + /* This region should be skipped if it is initialized before */ > > + if (reg->guest_user_addr == > > memory.regions[i].userspace_addr) { > > Also problematic for hot unplug. For example, > T0: we have three regions, 1, 2, 3. > T1: remove region 2, with only 1, 3 left. > > 1st iteration, 1 matches 1, skip old region 1. > 2nd iteration, 3 does not match 2, do the mmap. It is ok, I have verified it, let us have a discussion in meeting.