On 06/02/2017 12:07 PM, Andrea Bolognani wrote:
> The current algorithm for slot allocation tries to be clever
> and avoid looking at buses / slots more than once unless it's
> necessary. Unfortunately that makes the code more complex,
> and it will cause problem later on in some situations unless
> even more complex code is added.
>
> Since the performance gains are going to be pretty modest
> anyway, we can just get rid of the extra complexity and use a
> completely straighforward implementation instead.
> ---
>From looking at the current git blame you might assume that I had
something to do with this optimization, but actually it was there before
I got involved with PCI address allocation - I only preserved that
behavior. So I have no quarrel with simplifying it as you've done.
I guess the only concern I might have would be if the new algorithm
ended up give out a different address in some case (just in case someone
using transient domains with no pre-assigned PCI addresses was expecting
address stability), but I can't think of any way that would happen, and
the fact that the unit tests all pass indicates that it isn't happening.
Reviewed-by: Laine Stump <laine@xxxxxxxxx>
> src/conf/domain_addr.c | 43 +++++++------------------------------------
> src/conf/domain_addr.h | 2 --
> 2 files changed, 7 insertions(+), 38 deletions(-)
>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 6fd8bfc..d173766 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -741,34 +741,26 @@ virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr addrs,
> virDomainPCIConnectFlags flags,
> int function)
> {
> - /* default to starting the search for a free slot from
> - * the first slot of domain 0 bus 0...
> - */
> virPCIDeviceAddress a = { 0 };
> - bool found = false;
>
> if (addrs->nbuses == 0) {
> virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available"));
> goto error;
> }
>
> - /* ...unless this search is for the exact same type of device as
> - * last time, then continue the search from the slot where we
> - * found the previous match (it's possible there will still be a
> - * function available on that slot).
> - */
> - if (flags == addrs->lastFlags)
> - a = addrs->lastaddr;
> - else
> - a.slot = addrs->buses[0].minSlot;
> -
> /* if the caller asks for "any function", give them function 0 */
> if (function == -1)
> a.function = 0;
> else
> a.function = function;
>
> - while (a.bus < addrs->nbuses) {
> + /* "Begin at the beginning," the King said, very gravely, "and go on
> + * till you come to the end: then stop." */
> + for (a.bus = 0; a.bus < addrs->nbuses; a.bus++) {
> + bool found = false;
> +
> + a.slot = addrs->buses[a.bus].minSlot;
> +
> if (virDomainPCIAddressFindUnusedFunctionOnBus(&addrs->buses[a.bus],
> &a, function,
> flags, &found) < 0) {
> @@ -777,10 +769,6 @@ virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr addrs,
>
> if (found)
> goto success;
> -
> - /* nothing on this bus, go to the next bus */
> - if (++a.bus < addrs->nbuses)
> - a.slot = addrs->buses[a.bus].minSlot;
> }
>
> /* There were no free slots after the last used one */
> @@ -791,20 +779,6 @@ virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr addrs,
> /* this device will use the first slot of the new bus */
> a.slot = addrs->buses[a.bus].minSlot;
> goto success;
> - } else if (flags == addrs->lastFlags) {
> - /* Check the buses from 0 up to the last used one */
> - for (a.bus = 0; a.bus <= addrs->lastaddr.bus; a.bus++) {
> - a.slot = addrs->buses[a.bus].minSlot;
> -
> - if (virDomainPCIAddressFindUnusedFunctionOnBus(&addrs->buses[a.bus],
> - &a, function,
> - flags, &found) < 0) {
> - goto error;
> - }
> -
> - if (found)
> - goto success;
> - }
> }
>
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -851,9 +825,6 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> if (virDomainPCIAddressReserveAddrInternal(addrs, &addr, flags, false) < 0)
> return -1;
>
> - addrs->lastaddr = addr;
> - addrs->lastFlags = flags;
> -
> if (!addrs->dryRun) {
> dev->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> dev->addr.pci = addr;
> diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
> index 77e19bb..7704061 100644
> --- a/src/conf/domain_addr.h
> +++ b/src/conf/domain_addr.h
> @@ -106,8 +106,6 @@ typedef virDomainPCIAddressBus *virDomainPCIAddressBusPtr;
> struct _virDomainPCIAddressSet {
> virDomainPCIAddressBus *buses;
> size_t nbuses;
> - virPCIDeviceAddress lastaddr;
> - virDomainPCIConnectFlags lastFlags;
> bool dryRun; /* on a dry run, new buses are auto-added
> and addresses aren't saved in device infos */
> };
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
