[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v1 2/2] xen: mapcache: Fix unmapping of first entries in buckets


  • To: Alex Bennée <alex.bennee@xxxxxxxxxx>
  • From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxx>
  • Date: Thu, 4 Jul 2024 21:48:37 +0300
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=linaro.org smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0)
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=ZgFO+CJcjsUoi7CKvUCcl5rizxjr5Xw2OZ/mnxTWvdc=; b=PqH+jGSMHvnx2sQHgXvuw/6viBP3VI6wKW2c9tu4/tr69rfURkwYl3OFWqwa3ohqK4h5zsCTUedwnf2YXX1/tlMOFcTpqJRfKX7qv24E/ziPierbBtG65B2BxdZsqwH8GxxNPuTxLMPfYAlE5Immr10cbVqwNeB/RB9FMYJhwzdEO+kRtRi+yEqSezk87P9JWAANJnidb829jwijyTez2dV8i9qIf2R9eSCw3Bebqhed6CoLlmM3DZOu08omp21F+v5yh9az3HMB9+nqIIm83GE+Fxsv30YX/0mTISq1QyIqnyrdJfW+rkebsK0C8HNzFbQ/alcnqChsd8lCm7Um7A==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=XVOpyD+PvdgEJKM60hdhvnuEj1CALD8DYMa5APVVmdkG4DROjiVRaIF8yQDrvrMceTCk7a+QUWHd/nw2utwP+q/tw+V5rxTkzmSeJQvBmg3obQvhFqKycw4/zvSIlCz2CtGKwPEY1ZtppQnkQwZbn/ynWVTnHui9+YOUdCOZaw1hcW7nzprLsWEEvFLAjMof48T23Y31jtWNZ8yQwY8PBh5NBBjdKSomuICoQDJ1L0e6NmLCCm2irigl0U7oz0bSohMyk7Wpq6AwJSxRflb6QCisuCVFUQtqdekn0ubyTXf4wdPCoh1m8iQS8GjTrpEaubmIjzKwLhihEyba/cu/jA==
  • Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>, "Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxx>, <qemu-devel@xxxxxxxxxx>, <sstabellini@xxxxxxxxxx>, <paul@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 05 Jul 2024 05:51:51 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Thu, Jul 04, 2024 at 05:44:52PM +0100, Alex Bennée wrote:
> Anthony PERARD <anthony.perard@xxxxxxxxxx> writes:
> 
> > On Tue, Jul 02, 2024 at 12:44:21AM +0200, Edgar E. Iglesias wrote:
> >> From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxx>
> >> 
> >> This fixes the clobbering of the entry->next pointer when
> >> unmapping the first entry in a bucket of a mapcache.
> >> 
> >> Fixes: 123acd816d ("xen: mapcache: Unmap first entries in buckets")
> >> Reported-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> >> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxx>
> >> ---
> >>  hw/xen/xen-mapcache.c | 12 +++++++++++-
> >>  1 file changed, 11 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
> >> index 5f23b0adbe..18ba7b1d8f 100644
> >> --- a/hw/xen/xen-mapcache.c
> >> +++ b/hw/xen/xen-mapcache.c
> >> @@ -597,7 +597,17 @@ static void 
> >> xen_invalidate_map_cache_entry_unlocked(MapCache *mc,
> >>          pentry->next = entry->next;
> >>          g_free(entry);
> >>      } else {
> >> -        memset(entry, 0, sizeof *entry);
> >> +        /*
> >> +         * Invalidate mapping but keep entry->next pointing to the rest
> >> +         * of the list.
> >> +         *
> >> +         * Note that lock is already zero here, otherwise we don't unmap.
> >> +         */
> >> +        entry->paddr_index = 0;
> >> +        entry->vaddr_base = NULL;
> >> +        entry->valid_mapping = NULL;
> >> +        entry->flags = 0;
> >> +        entry->size = 0;
> >
> > This kind of feels like mc->entry should be an array of pointer rather
> > than an array of MapCacheEntry but that seems to work well enough and
> > not the first time entries are been cleared like that.
> 
> The use of a hand rolled list is a bit of a concern considering QEMU and
> Glib both provide various abstractions used around the rest of the code
> base. The original patch that introduces the mapcache doesn't tell me
> much about access patterns for the cache, just that it is trying to
> solve memory exhaustion issues with lots of dynamic small mappings.
> 
> Maybe a simpler structure is desirable?
> 
> We also have an interval tree implementation ("qemu/interval-tree.h") if
> what we really want is a sorted tree of memory that can be iterated
> locklessly.
> 

Yes, it would be interesting to benchmark other options.
I agree that we should at minimum reuse existing lists/hash tables.

We've also had some discussions around removing it partially or alltogether but
there are some concerns around that. We're going to need something to
keep track of grants. For 32-bit hosts, it's a problem to exhaust virtual
address-space if mapping all of the guest (are folks still using 32-bit hosts?).
There may be other issues aswell.

Some benefits are that we'll remove some of the complexity and latency for 
mapping
and unmapping stuff continously.

Cheers,
Edgar



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.