From nobody Fri Dec 27 14:25:40 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) client-ip=8.43.85.245; envelope-from=devel-bounces@lists.libvirt.org; helo=lists.libvirt.org; Authentication-Results: mx.zohomail.com; dkim=fail; spf=pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) smtp.mailfrom=devel-bounces@lists.libvirt.org; dmarc=fail(p=none dis=none) header.from=redhat.com Return-Path: Received: from lists.libvirt.org (lists.libvirt.org [8.43.85.245]) by mx.zohomail.com with SMTPS id 1732310268415910.223353334513; Fri, 22 Nov 2024 13:17:48 -0800 (PST) Received: by lists.libvirt.org (Postfix, from userid 996) id CF0AB1638; Fri, 22 Nov 2024 16:17:47 -0500 (EST) Received: from lists.libvirt.org (localhost [IPv6:::1]) by lists.libvirt.org (Postfix) with ESMTP id 6A4B115D8; Fri, 22 Nov 2024 16:16:51 -0500 (EST) Received: by lists.libvirt.org (Postfix, from userid 996) id 9CA9713EE; Fri, 22 Nov 2024 16:16:46 -0500 (EST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.libvirt.org (Postfix) with ESMTPS id DDA01137F for ; Fri, 22 Nov 2024 16:16:45 -0500 (EST) Received: from mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-118-A9OVuK4PMyyGVN25CNZ4Yg-1; Fri, 22 Nov 2024 16:16:44 -0500 Received: from mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 5F83E19560AA for ; Fri, 22 Nov 2024 21:16:43 +0000 (UTC) Received: from vhost3.router.laine.org (unknown [10.22.88.88]) by mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id 3BBBD30000DF; Fri, 22 Nov 2024 21:16:42 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on lists.libvirt.org X-Spam-Level: X-Spam-Status: No, score=-0.5 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,RCVD_IN_VALIDITY_RPBL_BLOCKED, RCVD_IN_VALIDITY_SAFE_BLOCKED,SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=3.4.4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1732310205; h=from:from: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; bh=UgB42z2LMzycgjzEMVPVh2gOMMkHFVLyBO79fyXt1NQ=; b=VvQRRonUGyAFTbR3NVKA5lai06WRK0m/GnvYBjbCwq5uqFG8yvZdB936S48Q860zNQaafd GZr9p4NtrwyhGlQhUQAmP4wGTBq0KKJYK/BsDFdYWENZx/BlTJnRT39C7LIyCxMn+oLnLe Rk0lK8rFYkVZtqQz0+tUZ/FRHaaGh00= X-MC-Unique: A9OVuK4PMyyGVN25CNZ4Yg-1 X-Mimecast-MFC-AGG-ID: A9OVuK4PMyyGVN25CNZ4Yg From: Laine Stump To: devel@lists.libvirt.org Subject: [PATCH 1/5] util: make it optional to clear existing tc qdiscs/filters in virNetDevBandwidthSet() Date: Fri, 22 Nov 2024 16:16:35 -0500 Message-ID: <20241122211639.615518-2-laine@redhat.com> In-Reply-To: <20241122211639.615518-1-laine@redhat.com> References: <20241122211639.615518-1-laine@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.30.177.4 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: cuRmBb-88vJrEbX_rHt5kJFTbuFSNxsSy93X60Pdg0Q_1732310203 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable Message-ID-Hash: VHZOGBF5H6OR2IUHDULHMPGOE3S724L2 X-Message-ID-Hash: VHZOGBF5H6OR2IUHDULHMPGOE3S724L2 X-MailFrom: laine@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-config-1; header-match-config-2; header-match-config-3; header-match-devel.lists.libvirt.org-0; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; suspicious-header CC: mprivozn@redhat.com, psutter@redhat.com, abologna@redhat.com, egarver@redhat.com X-Mailman-Version: 3.2.2 Precedence: list List-Id: Development discussions about the libvirt library & tools Archived-At: List-Archive: List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-ZohoMail-DKIM: fail (Header signature does not verify) X-ZM-MESSAGEID: 1732310271215116600 Content-Type: text/plain; charset="utf-8"; x-default="true" virNetDevBandwidthSet() always clears all existing qdiscs and their subordinate filters before adding all the new qdiscs/filters. This is normally exactly what we want, but there is one case (the network driver) where the Qdisc added by virNetDevBandwidthSet() may already be in use by the nftables backend (which will add a rule to fix the checksum of dhcp packets); in that case, we *don't* want virNetDevBandwidthSet() to clear out the qdisc that was already added for nftables, and none of the bandwidth filters have been added yet, so there already aren't any "old" filters that need to be removed either - it is safe to just skip virNetDevBandwidthClear() in this case. To allow the network driver to set bandwidth without first clearing it, this patch adds a "clear" bool to the args of virNetDevBandwidthSet() - if clear-true (for almost all usages this is the case) virNetDevBandwidth() will call virNetDevBandwidthClear() just as it always has. But if clear=3Dfalse it *won't* call virNetDevBandwidthClear(). As suggested above, clear is set to false for all calls to virNetdevBandwidthSet() except for two places in the network driver. Signed-off-by: Laine Stump --- src/lxc/lxc_driver.c | 2 +- src/lxc/lxc_process.c | 2 +- src/network/bridge_driver.c | 4 ++-- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_hotplug.c | 4 ++-- src/util/virnetdevbandwidth.c | 19 ++++++++++++++++++- src/util/virnetdevbandwidth.h | 1 + tests/virnetdevbandwidthtest.c | 2 +- 9 files changed, 29 insertions(+), 10 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 0e31e5e4b9..6910651f95 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3570,7 +3570,7 @@ lxcDomainAttachDeviceNetLive(virLXCDriver *driver, actualBandwidth =3D virDomainNetGetActualBandwidth(net); if (actualBandwidth) { if (virNetDevSupportsBandwidth(actualType)) { - if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false, + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, true, = false, !virDomainNetTypeSharesHostView(net)= ) < 0) goto cleanup; } else { diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 083ab83ec6..e1a310029d 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -609,7 +609,7 @@ virLXCProcessSetupInterfaces(virLXCDriver *driver, actualBandwidth =3D virDomainNetGetActualBandwidth(net); if (actualBandwidth) { if (virNetDevSupportsBandwidth(type)) { - if (virNetDevBandwidthSet(net->ifname, actualBandwidth, fa= lse, + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, tr= ue, false, !virDomainNetTypeSharesHostView(= net)) < 0) goto cleanup; } else { diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d408f17de7..698146dd8c 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2109,7 +2109,7 @@ networkStartNetworkVirtual(virNetworkDriverState *dri= ver, } } =20 - if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) + if (virNetDevBandwidthSet(def->bridge, def->bandwidth, false, true, tr= ue) < 0) goto error; =20 return 0; @@ -2190,7 +2190,7 @@ networkStartNetworkBridge(virNetworkObj *obj) * type BRIDGE, is started. On failure, undo anything you've done, * and return -1. On success return 0. */ - if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) + if (virNetDevBandwidthSet(def->bridge, def->bandwidth, false, true, tr= ue) < 0) goto error; =20 if (networkStartHandleMACTableManagerMode(obj) < 0) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f4430275dc..3afdc72d05 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8694,7 +8694,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, def->uuid, !virDomainNetTypeS= haresHostView(net)) < 0) goto cleanup; - } else if (virNetDevBandwidthSet(net->ifname, actualBandwidth,= false, + } else if (virNetDevBandwidthSet(net->ifname, actualBandwidth,= true, false, !virDomainNetTypeSharesHostVi= ew(net)) < 0) { goto cleanup; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5b9c55f704..abea0799f2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9938,13 +9938,14 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, virErrorRestore(&orig_err); goto endjob; } - } else if (virNetDevBandwidthSet(net->ifname, newBandwidth, false, + } else if (virNetDevBandwidthSet(net->ifname, newBandwidth, true, = false, !virDomainNetTypeSharesHostView(n= et)) < 0) { virErrorPtr orig_err; =20 virErrorPreserveLast(&orig_err); ignore_value(virNetDevBandwidthSet(net->ifname, net->bandwidth, + true, false, !virDomainNetTypeSharesHost= View(net))); if (net->bandwidth) { diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 55512476e4..5b35f724dd 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1331,7 +1331,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, vm->def->uuid, !virDomainNetTypeS= haresHostView(net)) < 0) goto cleanup; - } else if (virNetDevBandwidthSet(net->ifname, actualBandwidth,= false, + } else if (virNetDevBandwidthSet(net->ifname, actualBandwidth,= true, false, !virDomainNetTypeSharesHostVi= ew(net)) < 0) { goto cleanup; } @@ -4181,7 +4181,7 @@ qemuDomainChangeNet(virQEMUDriver *driver, vm->def->uuid, !virDomainNetTypeS= haresHostView(newdev)) < 0) goto cleanup; - } else if (virNetDevBandwidthSet(newdev->ifname, newb, false, + } else if (virNetDevBandwidthSet(newdev->ifname, newb, true, f= alse, !virDomainNetTypeSharesHostVi= ew(newdev)) < 0) { goto cleanup; } diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index 2b58c58d3e..d62a85a06c 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -173,6 +173,7 @@ virNetDevBandwidthManipulateFilter(const char *ifname, * virNetDevBandwidthSet: * @ifname: on which interface * @bandwidth: rates to set (may be NULL) + * @clear: true if we should first clear all tc qdiscs/filters already on = the interface * @hierarchical_class: whether to create hierarchical class * @swapped: true if IN/OUT should be set contrariwise * @@ -183,6 +184,17 @@ virNetDevBandwidthManipulateFilter(const char *ifname, * hierarchical class. It is used to guarantee minimal * throughput ('floor' attribute in NIC). * + * If @clear is true, then the root qdisc is deleted, which causes any + * already existing filters to also be deleted. If false, then it's + * assumed that there are no existing rules. The caller should use + * clear=3Dtrue for an existing interface that is having its bandwidth + * setting modified, but can use clear=3Dfalse if the interface was + * newly created, and this is the first time bandwidth has been set, + * but someone else might have already added the qdisc (e.g. this is + * the case when the network driver is setting bandwidth for a virtual + * network bridge device - the nftables backend may have already added + * qdisc handle 1:0 and a filter, and we don't want to delete them) + * * If @swapped is set, the IN part of @bandwidth is set on * @ifname's TX, and vice versa. If it is not set, IN is set on * RX and OUT on TX. This is because for some types of interfaces @@ -195,6 +207,7 @@ virNetDevBandwidthManipulateFilter(const char *ifname, int virNetDevBandwidthSet(const char *ifname, const virNetDevBandwidth *bandwidth, + bool clear, bool hierarchical_class, bool swapped) { @@ -232,7 +245,11 @@ virNetDevBandwidthSet(const char *ifname, tx =3D bandwidth->out; } =20 - virNetDevBandwidthClear(ifname); + /* Only if the caller requests, clear everything including root + * qdisc and all filters before adding everything. + */ + if (clear) + virNetDevBandwidthClear(ifname); =20 if (tx && tx->average) { average =3D g_strdup_printf("%llukbps", tx->average); diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h index 6d268fb119..68344016c5 100644 --- a/src/util/virnetdevbandwidth.h +++ b/src/util/virnetdevbandwidth.h @@ -41,6 +41,7 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevBandwidth, virNetD= evBandwidthFree); =20 int virNetDevBandwidthSet(const char *ifname, const virNetDevBandwidth *bandwidth, + bool clear, bool hierarchical_class, bool swapped) G_GNUC_WARN_UNUSED_RESULT; diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index f7c38faa2e..75f960e402 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -83,7 +83,7 @@ testVirNetDevBandwidthSet(const void *data) return -1; } else { exp_cmd =3D info->exp_cmd_tc; - if (virNetDevBandwidthSet(iface, band, info->hierarchical_class, t= rue) < 0) + if (virNetDevBandwidthSet(iface, band, true, info->hierarchical_cl= ass, true) < 0) return -1; } =20 --=20 2.47.0