Re: [PATCH 1/5] util: make it optional to clear existing tc qdiscs/filters in virNetDevBandwidthSet()
On 11/25/24 5:38 AM, Daniel P. Berrangé wrote: On Fri, Nov 22, 2024 at 04:16:35PM -0500, Laine Stump wrote: 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=false 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 --- 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, virNetDevBandwidthFree); int virNetDevBandwidthSet(const char *ifname, const virNetDevBandwidth *bandwidth, + bool clear, bool hierarchical_class, bool swapped) G_GNUC_WARN_UNUSED_RESULT; I'm not such a fan of magic "bool" parameters that have signifcant behaviour changes, because when reading the code it is far from clear what effect passing "true" is having. This is compounded when we have 3 "bool" parameters in a row. Yeah, me too. The only reason I didn't do that is because I was concerned about reducing change in order to avoid a regression, and also because I was lazy. Your mentioning it is enough to put it over the top though, so I'll redo this patch using a single flags arg. I think this would be better change to have 'int flags', and then define an enum bitset VIR_NETDEV_BANDWIDTH_CLEAR_CLASSES = (1 << 0) VIR_NETDEV_BANDWIDTH_HIERARCHICAL_CLASS = (1 << 1) VIR_NETDEV_BANDWIDTH_DIR_SWAPPED = (1 << 2) With regards, Daniel
Re: [PATCH 1/5] util: make it optional to clear existing tc qdiscs/filters in virNetDevBandwidthSet()
On Fri, Nov 22, 2024 at 04:16:35PM -0500, Laine Stump wrote: > 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=false 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 > --- > 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, > virNetDevBandwidthFree); > > int virNetDevBandwidthSet(const char *ifname, >const virNetDevBandwidth *bandwidth, > + bool clear, >bool hierarchical_class, >bool swapped) > G_GNUC_WARN_UNUSED_RESULT; I'm not such a fan of magic "bool" parameters that have signifcant behaviour changes, because when reading the code it is far from clear what effect passing "true" is having. This is compounded when we have 3 "bool" parameters in a row. I think this would be better change to have 'int flags', and then define an enum bitset VIR_NETDEV_BANDWIDTH_CLEAR_CLASSES = (1 << 0) VIR_NETDEV_BANDWIDTH_HIERARCHICAL_CLASS = (1 << 1) VIR_NETDEV_BANDWIDTH_DIR_SWAPPED = (1 << 2) With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
