From nobody Sun May 5 01:03:32 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of redhat.com designates 207.211.31.120 as permitted sender) client-ip=207.211.31.120; envelope-from=libvir-list-bounces@redhat.com; helo=us-smtp-1.mimecast.com; Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of redhat.com designates 207.211.31.120 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass(p=none dis=none) header.from=redhat.com ARC-Seal: i=1; a=rsa-sha256; t=1579869631; cv=none; d=zohomail.com; s=zohoarc; b=jh4yEiB9MMtAI1S/ce+AWPqwl1xCxvH4OaY0PKdRbaWkS3vQ0ZerG53ZeQgHwoY/kJ+J9NzduzMiIzn1D5jdyLwwbfI80i4CgwR2I9DzdBoqKQjlCr4kWa0XdiEHdjI4B/wllFRotxPwsEP32oqa8JMsZqPAtrpQyCao9yNqOsA= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1579869631; h=Content-Type:Content-Transfer-Encoding:Cc:Date:From:In-Reply-To:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:References:Sender:Subject:To; bh=gh/pMmKUyRT2J8aBqGqAAt8uoOWAUJw0apseLuocbcQ=; b=nzM4TVf8U2zJmzD7CPMdqDd5V2XUR7OIYv2bPtdmccc6uRoN6egKtwkFECexH3+phV/qGdw0LqERZU9X6FDsfqIzOdVyorOre1PF1BYB+BW1kybwG+P1MY861ZgOAlMhTOHNUCQU9jxO8W0qPfe7Tm/bTqiuYO8hrmi/IoDSCyE= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of redhat.com designates 207.211.31.120 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass header.from= (p=none dis=none) header.from= Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by mx.zohomail.com with SMTPS id 157986963120056.39158584926224; Fri, 24 Jan 2020 04:40:31 -0800 (PST) Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-87-gjUtFjRlNNWSz2vxDYv8TQ-1; Fri, 24 Jan 2020 07:40:26 -0500 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C05731937FEE; Fri, 24 Jan 2020 12:40:20 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8601660BFB; Fri, 24 Jan 2020 12:40:20 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id A253D1809567; Fri, 24 Jan 2020 12:40:19 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id 00OCeIrh031649 for ; Fri, 24 Jan 2020 07:40:18 -0500 Received: by smtp.corp.redhat.com (Postfix) id 20AF58CCDE; Fri, 24 Jan 2020 12:40:18 +0000 (UTC) Received: from moe.redhat.com (unknown [10.43.2.30]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4F4BF8E9E0; Fri, 24 Jan 2020 12:40:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1579869629; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:list-id:list-help: list-unsubscribe:list-subscribe:list-post; bh=gh/pMmKUyRT2J8aBqGqAAt8uoOWAUJw0apseLuocbcQ=; b=LUtf0vjS2iUe3zgkKBItsYfPkNS9f6/PlVV7NY1PcP9dkWwoTxjSYS1LkZPs7hMpkZBiIx A03a0yLbQ/x2eYISfoTyFenH+SgnUM6MbZLjc+tgjlAihrt3ktr1W4UACqSOPOHSgvbea2 rdX3noakCJABXw88uJvNyp34NNHqgN4= From: Michal Privoznik To: libvir-list@redhat.com Subject: [PATCH v2] qemu_capabilities: Rework domain caps cache Date: Fri, 24 Jan 2020 13:40:03 +0100 Message-Id: In-Reply-To: References: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-loop: libvir-list@redhat.com Cc: pkrempa@redhat.com X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Development discussions about the libvirt library & tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-MC-Unique: gjUtFjRlNNWSz2vxDYv8TQ-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable X-ZohoMail-DKIM: pass (identity @redhat.com) Content-Type: text/plain; charset="utf-8" Since v5.6.0-48-g270583ed98 we try to cache domain capabilities, i.e. store filled virDomainCaps in a hash table in virQEMUCaps for future use. However, there's a race condition in the way it's implemented. We use virQEMUCapsGetDomainCapsCache() to obtain the pointer to the hash table, then we search the hash table for cached data and if none is found the domcaps is constructed and put into the table. Problem is that this is all done without any locking, so if there are two threads trying to do the same, one will succeed and the other will fail inserting the data into the table. Also, the API looks a bit fishy - obtaining pointer to the hash table is dangerous. The solution is to use a mutex that guards the whole operation with the hash table. Then, the API can be changes to return virDomainCapsPtr directly. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=3D1791790 Signed-off-by: Michal Privoznik Reviewed-by: Peter Krempa --- diff to v1: - Dropped the callback in favor of passing necessary values as arguments src/qemu/qemu_capabilities.c | 122 +++++++++++++++++++++++++++++++++-- src/qemu/qemu_capabilities.h | 12 +++- src/qemu/qemu_conf.c | 65 +++---------------- 3 files changed, 136 insertions(+), 63 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 17b0134ab8..4d7008396e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -598,6 +598,26 @@ struct _virQEMUCapsAccel { qemuMonitorCPUDefsPtr cpuModels; }; =20 + +typedef struct _virQEMUDomainCapsCache virQEMUDomainCapsCache; +typedef virQEMUDomainCapsCache *virQEMUDomainCapsCachePtr; +struct _virQEMUDomainCapsCache { + virObjectLockable parent; + + virHashTablePtr cache; +}; + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUDomainCapsCache, virObjectUnref); + +static virClassPtr virQEMUDomainCapsCacheClass; +static void virQEMUDomainCapsCacheDispose(void *obj) +{ + virQEMUDomainCapsCachePtr cache =3D obj; + + virHashFree(cache->cache); +} + + /* * Update the XML parser/formatter when adding more * information to this struct so that it gets cached @@ -629,7 +649,7 @@ struct _virQEMUCaps { =20 virArch arch; =20 - virHashTablePtr domCapsCache; + virQEMUDomainCapsCachePtr domCapsCache; =20 size_t ngicCapabilities; virGICCapability *gicCapabilities; @@ -655,6 +675,9 @@ static int virQEMUCapsOnceInit(void) if (!VIR_CLASS_NEW(virQEMUCaps, virClassForObject())) return -1; =20 + if (!(VIR_CLASS_NEW(virQEMUDomainCapsCache, virClassForObjectLockable(= )))) + return -1; + return 0; } =20 @@ -1625,6 +1648,22 @@ int virQEMUCapsGetDefaultVersion(virCapsPtr caps, } =20 =20 +static virQEMUDomainCapsCachePtr +virQEMUDomainCapsCacheNew(void) +{ + g_autoptr(virQEMUDomainCapsCache) cache =3D NULL; + + if (virQEMUCapsInitialize() < 0) + return NULL; + + if (!(cache =3D virObjectLockableNew(virQEMUDomainCapsCacheClass))) + return NULL; + + if (!(cache->cache =3D virHashCreate(5, virObjectFreeHashData))) + return NULL; + + return g_steal_pointer(&cache); +} =20 =20 virQEMUCapsPtr @@ -1642,7 +1681,7 @@ virQEMUCapsNew(void) if (!(qemuCaps->flags =3D virBitmapNew(QEMU_CAPS_LAST))) goto error; =20 - if (!(qemuCaps->domCapsCache =3D virHashCreate(5, virObjectFreeHashDat= a))) + if (!(qemuCaps->domCapsCache =3D virQEMUDomainCapsCacheNew())) goto error; =20 return qemuCaps; @@ -1832,7 +1871,7 @@ void virQEMUCapsDispose(void *obj) { virQEMUCapsPtr qemuCaps =3D obj; =20 - virHashFree(qemuCaps->domCapsCache); + virObjectUnref(qemuCaps->domCapsCache); virBitmapFree(qemuCaps->flags); =20 VIR_FREE(qemuCaps->package); @@ -1992,9 +2031,82 @@ const char *virQEMUCapsGetPackage(virQEMUCapsPtr qem= uCaps) } =20 =20 -virHashTablePtr virQEMUCapsGetDomainCapsCache(virQEMUCapsPtr qemuCaps) +struct virQEMUCapsSearchDomcapsData { + const char *path; + const char *machine; + virArch arch; + virDomainVirtType virttype; +}; + + +static int +virQEMUCapsSearchDomcaps(const void *payload, + const void *name G_GNUC_UNUSED, + const void *opaque) { - return qemuCaps->domCapsCache; + virDomainCapsPtr domCaps =3D (virDomainCapsPtr) payload; + struct virQEMUCapsSearchDomcapsData *data =3D (struct virQEMUCapsSearc= hDomcapsData *) opaque; + + if (STREQ_NULLABLE(data->path, domCaps->path) && + STREQ_NULLABLE(data->machine, domCaps->machine) && + data->arch =3D=3D domCaps->arch && + data->virttype =3D=3D domCaps->virttype) + return 1; + + return 0; +} + + +virDomainCapsPtr +virQEMUCapsGetDomainCapsCache(virQEMUCapsPtr qemuCaps, + const char *machine, + virArch arch, + virDomainVirtType virttype, + virArch hostarch, + bool privileged, + virFirmwarePtr *firmwares, + size_t nfirmwares) +{ + virQEMUDomainCapsCachePtr cache =3D qemuCaps->domCapsCache; + virDomainCapsPtr domCaps =3D NULL; + const char *path =3D virQEMUCapsGetBinary(qemuCaps); + struct virQEMUCapsSearchDomcapsData data =3D { + .path =3D path, + .machine =3D machine, + .arch =3D arch, + .virttype =3D virttype, + }; + + virObjectLock(cache); + + domCaps =3D virHashSearch(cache->cache, virQEMUCapsSearchDomcaps, &dat= a, NULL); + + if (!domCaps) { + g_autoptr(virDomainCaps) tempDomCaps =3D NULL; + g_autofree char *key =3D NULL; + + /* hash miss, build new domcaps */ + if (!(tempDomCaps =3D virDomainCapsNew(path, machine, + arch, virttype))) + goto cleanup; + + if (virQEMUCapsFillDomainCaps(qemuCaps, hostarch, tempDomCaps, + privileged, firmwares, nfirmwares) <= 0) + goto cleanup; + + key =3D g_strdup_printf("%d:%d:%s:%s", arch, virttype, + NULLSTR(machine), path); + + if (virHashAddEntry(cache->cache, key, tempDomCaps) < 0) + goto cleanup; + + domCaps =3D g_steal_pointer(&tempDomCaps); + } + + virObjectRef(domCaps); + cleanup: + virObjectUnlock(cache); + return domCaps; } =20 =20 diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index ebfdb4b981..e110805456 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -575,7 +575,17 @@ const char *virQEMUCapsGetBinary(virQEMUCapsPtr qemuCa= ps); virArch virQEMUCapsGetArch(virQEMUCapsPtr qemuCaps); unsigned int virQEMUCapsGetVersion(virQEMUCapsPtr qemuCaps); const char *virQEMUCapsGetPackage(virQEMUCapsPtr qemuCaps); -virHashTablePtr virQEMUCapsGetDomainCapsCache(virQEMUCapsPtr qemuCaps); + +virDomainCapsPtr +virQEMUCapsGetDomainCapsCache(virQEMUCapsPtr qemuCaps, + const char *machine, + virArch arch, + virDomainVirtType virttype, + virArch hostarch, + bool privileged, + virFirmwarePtr *firmwares, + size_t nfirmwares); + unsigned int virQEMUCapsGetKVMVersion(virQEMUCapsPtr qemuCaps); int virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps, virDomainVirtType type, diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 1204b189fa..e500da1515 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1338,31 +1338,6 @@ virCapsPtr virQEMUDriverGetCapabilities(virQEMUDrive= rPtr driver, } =20 =20 -struct virQEMUDriverSearchDomcapsData { - const char *path; - const char *machine; - virArch arch; - virDomainVirtType virttype; -}; - - -static int -virQEMUDriverSearchDomcaps(const void *payload, - const void *name G_GNUC_UNUSED, - const void *opaque) -{ - virDomainCapsPtr domCaps =3D (virDomainCapsPtr) payload; - struct virQEMUDriverSearchDomcapsData *data =3D (struct virQEMUDriverS= earchDomcapsData *) opaque; - - if (STREQ_NULLABLE(data->path, domCaps->path) && - STREQ_NULLABLE(data->machine, domCaps->machine) && - data->arch =3D=3D domCaps->arch && - data->virttype =3D=3D domCaps->virttype) - return 1; - - return 0; -} - /** * virQEMUDriverGetDomainCapabilities: * @@ -1381,40 +1356,16 @@ virQEMUDriverGetDomainCapabilities(virQEMUDriverPtr= driver, virArch arch, virDomainVirtType virttype) { - g_autoptr(virDomainCaps) domCaps =3D NULL; g_autoptr(virQEMUDriverConfig) cfg =3D virQEMUDriverGetConfig(driver); - virHashTablePtr domCapsCache =3D virQEMUCapsGetDomainCapsCache(qemuCap= s); - struct virQEMUDriverSearchDomcapsData data =3D { - .path =3D virQEMUCapsGetBinary(qemuCaps), - .machine =3D machine, - .arch =3D arch, - .virttype =3D virttype, - }; - - domCaps =3D virHashSearch(domCapsCache, - virQEMUDriverSearchDomcaps, &data, NULL); - if (!domCaps) { - g_autofree char *key =3D NULL; - - /* hash miss, build new domcaps */ - if (!(domCaps =3D virDomainCapsNew(data.path, data.machine, - data.arch, data.virttype))) - return NULL; - - if (virQEMUCapsFillDomainCaps(qemuCaps, driver->hostarch, domCaps, - driver->privileged, - cfg->firmwares, cfg->nfirmwares) < 0) - return NULL; - - key =3D g_strdup_printf("%d:%d:%s:%s", data.arch, data.virttype, - NULLSTR(data.machine), NULLSTR(data.path)); - - if (virHashAddEntry(domCapsCache, key, domCaps) < 0) - return NULL; - } =20 - virObjectRef(domCaps); - return g_steal_pointer(&domCaps); + return virQEMUCapsGetDomainCapsCache(qemuCaps, + machine, + arch, + virttype, + driver->hostarch, + driver->privileged, + cfg->firmwares, + cfg->nfirmwares); } =20 =20 --=20 2.24.1