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

Re: [libvirt] [PATCH 06/26] conf: Simplify slot allocation



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 laine org>
>  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 */
>  };
> 


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