[ovs-dev,3/3] ovn-northd: Always allocate ipam_info for an ovn_datapath.

Until now, the ipam_info struct for a datapath has been allocated on
demand.  This leads to slightly complication in the code in places, and
there is hardly any benefit since ipam_info is only about 48 bytes anyway.
This commit just inlines it into struct ovn_datapath.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ovn/northd/ovn-northd.c | 90 +++++++++++++++++++------------------------------
 1 file changed, 35 insertions(+), 55 deletions(-)

Hi Ben,

Just a couple of findings in-line below.

On 06/15/2018 07:11 PM, Ben Pfaff wrote:
> Until now, the ipam_info struct for a datapath has been allocated on
> demand.  This leads to slightly complication in the code in places, and
> there is hardly any benefit since ipam_info is only about 48 bytes anyway.
> This commit just inlines it into struct ovn_datapath.
> 
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>   ovn/northd/ovn-northd.c | 90 +++++++++++++++++++------------------------------
>   1 file changed, 35 insertions(+), 55 deletions(-)
> 
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 8755fa1f40c0..2eb6ace39cba 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -427,7 +427,7 @@ struct ovn_datapath {
>       bool has_unknown;
>   
>       /* IPAM data. */
> -    struct ipam_info *ipam_info;
> +    struct ipam_info ipam_info;
>   
>       /* OVN northd only needs to know about the logical router gateway port for
>        * NAT on a distributed router.  This "distributed gateway port" is
> @@ -480,10 +480,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od)
>            * use it. */
>           hmap_remove(datapaths, &od->key_node);
>           destroy_tnlids(&od->port_tnlids);
> -        if (od->ipam_info) {
> -            bitmap_free(od->ipam_info->allocated_ipv4s);
> -            free(od->ipam_info);
> -        }
> +        bitmap_free(od->ipam_info.allocated_ipv4s);
>           free(od->router_ports);
>           free(od);
>       }
> @@ -535,26 +532,14 @@ init_ipam_info_for_datapath(struct ovn_datapath *od)
>           return;
>       }
>   
> -    const char *subnet_str = smap_get(&od->nbs->other_config, "subnet");
>       const char *ipv6_prefix = smap_get(&od->nbs->other_config, "ipv6_prefix");
> +    od->ipam_info.ipv6_prefix_set = ipv6_prefix && ipv6_parse(
> +            ipv6_prefix, &od->ipam_info.ipv6_prefix);
>   
> -    if (ipv6_prefix) {
> -        if (!od->ipam_info) {
> -            od->ipam_info = xzalloc(sizeof *od->ipam_info);
> -        }
> -        od->ipam_info->ipv6_prefix_set = ipv6_parse(
> -            ipv6_prefix, &od->ipam_info->ipv6_prefix);
> -    } else {
> -        if (od->ipam_info) {
> -            od->ipam_info->ipv6_prefix_set = false;
> -        }
> -    }
> -
> +    const char *subnet_str = smap_get(&od->nbs->other_config, "subnet");
>       if (!subnet_str) {
> -        if (od->ipam_info) {
> -            bitmap_free(od->ipam_info->allocated_ipv4s);
> -            od->ipam_info->allocated_ipv4s = NULL;
> -        }
> +        bitmap_free(od->ipam_info.allocated_ipv4s);
> +        od->ipam_info.allocated_ipv4s = NULL;

These two lines aren't necessary. Since od will be newly-allocated when 
init_ipam_info_for_datapath() is called, od->ipam_info.allocated_ipv4s 
will already be NULL at this point.

>           return;
>       }
>   
> @@ -566,24 +551,18 @@ init_ipam_info_for_datapath(struct ovn_datapath *od)
>           VLOG_WARN_RL(&rl, "bad 'subnet' %s", subnet_str);
>           free(error);
>   
> -        if (od->ipam_info) {
> -            bitmap_free(od->ipam_info->allocated_ipv4s);
> -            od->ipam_info->allocated_ipv4s = NULL;
> -        }
> +        bitmap_free(od->ipam_info.allocated_ipv4s);
> +        od->ipam_info.allocated_ipv4s = NULL;

The same goes here, this is not necessary since 
od->ipam_info.allocated_ipv4s is not allocated yet.

>   
>           return;
>       }
> -
> -    if (!od->ipam_info) {
> -        od->ipam_info = xzalloc(sizeof *od->ipam_info);
> -    }
> -    od->ipam_info->start_ipv4 = ntohl(subnet) + 1;
> -    od->ipam_info->total_ipv4s = ~ntohl(mask);
> -    od->ipam_info->allocated_ipv4s =
> -        bitmap_allocate(od->ipam_info->total_ipv4s);
> +    od->ipam_info.start_ipv4 = ntohl(subnet) + 1;
> +    od->ipam_info.total_ipv4s = ~ntohl(mask);
> +    od->ipam_info.allocated_ipv4s =
> +        bitmap_allocate(od->ipam_info.total_ipv4s);
>   
>       /* Mark first IP as taken */
> -    bitmap_set1(od->ipam_info->allocated_ipv4s, 0);
> +    bitmap_set1(od->ipam_info.allocated_ipv4s, 0);
>   
>       /* Check if there are any reserver IPs (list) to be excluded from IPAM */
>       const char *exclude_ip_list = smap_get(&od->nbs->other_config,
> @@ -617,11 +596,11 @@ init_ipam_info_for_datapath(struct ovn_datapath *od)
>           }
>   
>           /* Clamp start...end to fit the subnet. */
> -        start = MAX(od->ipam_info->start_ipv4, start);
> -        end = MIN(od->ipam_info->start_ipv4 + od->ipam_info->total_ipv4s, end);
> +        start = MAX(od->ipam_info.start_ipv4, start);
> +        end = MIN(od->ipam_info.start_ipv4 + od->ipam_info.total_ipv4s, end);
>           if (end > start) {
> -            bitmap_set_multiple(od->ipam_info->allocated_ipv4s,
> -                                start - od->ipam_info->start_ipv4,
> +            bitmap_set_multiple(od->ipam_info.allocated_ipv4s,
> +                                start - od->ipam_info.start_ipv4,
>                                   end - start, 1);
>           } else {
>               lexer_error(&lexer, "excluded addresses not in subnet");
> @@ -953,14 +932,14 @@ ipam_insert_mac(struct eth_addr *ea, bool check)
>   static void
>   ipam_insert_ip(struct ovn_datapath *od, uint32_t ip)
>   {
> -    if (!od || !od->ipam_info || !od->ipam_info->allocated_ipv4s) {
> +    if (!od || !od->ipam_info.allocated_ipv4s) {
>           return;
>       }
>   
> -    if (ip >= od->ipam_info->start_ipv4 &&
> -        ip < (od->ipam_info->start_ipv4 + od->ipam_info->total_ipv4s)) {
> -        bitmap_set1(od->ipam_info->allocated_ipv4s,
> -                    ip - od->ipam_info->start_ipv4);
> +    if (ip >= od->ipam_info.start_ipv4 &&
> +        ip < (od->ipam_info.start_ipv4 + od->ipam_info.total_ipv4s)) {
> +        bitmap_set1(od->ipam_info.allocated_ipv4s,
> +                    ip - od->ipam_info.start_ipv4);
>       }
>   }
>   
> @@ -983,7 +962,7 @@ ipam_insert_lsp_addresses(struct ovn_datapath *od, struct ovn_port *op,
>   
>       /* IP is only added to IPAM if the switch's subnet option
>        * is set, whereas MAC is always added to MACAM. */
> -    if (!od->ipam_info || !od->ipam_info->allocated_ipv4s) {
> +    if (!od->ipam_info.allocated_ipv4s) {
>           destroy_lport_addresses(&laddrs);
>           return;
>       }
> @@ -1068,26 +1047,26 @@ ipam_get_unused_mac(void)
>   static uint32_t
>   ipam_get_unused_ip(struct ovn_datapath *od)
>   {
> -    if (!od || !od->ipam_info || !od->ipam_info->allocated_ipv4s) {
> +    if (!od || !od->ipam_info.allocated_ipv4s) {
>           return 0;
>       }
>   
> -    size_t new_ip_index = bitmap_scan(od->ipam_info->allocated_ipv4s, 0, 0,
> -                                      od->ipam_info->total_ipv4s - 1);
> -    if (new_ip_index == od->ipam_info->total_ipv4s - 1) {
> +    size_t new_ip_index = bitmap_scan(od->ipam_info.allocated_ipv4s, 0, 0,
> +                                      od->ipam_info.total_ipv4s - 1);
> +    if (new_ip_index == od->ipam_info.total_ipv4s - 1) {
>           static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>           VLOG_WARN_RL( &rl, "Subnet address space has been exhausted.");
>           return 0;
>       }
>   
> -    return od->ipam_info->start_ipv4 + new_ip_index;
> +    return od->ipam_info.start_ipv4 + new_ip_index;
>   }
>   
>   static bool
>   ipam_allocate_addresses(struct ovn_datapath *od, struct ovn_port *op,
>                           const char *addrspec)
>   {
> -    if (!op->nbsp || !od->ipam_info) {
> +    if (!op->nbsp) {
>           return false;
>       }
>   
> @@ -1109,14 +1088,14 @@ ipam_allocate_addresses(struct ovn_datapath *od, struct ovn_port *op,
>       }
>   
>       /* Generate IPv4 address, if desirable. */
> -    bool dynamic_ip4 = od->ipam_info->allocated_ipv4s != NULL;
> +    bool dynamic_ip4 = od->ipam_info.allocated_ipv4s != NULL;
>       uint32_t ip4 = dynamic_ip4 ? ipam_get_unused_ip(od) : 0;
>   
>       /* Generate IPv6 address, if desirable. */
> -    bool dynamic_ip6 = od->ipam_info->ipv6_prefix_set;
> +    bool dynamic_ip6 = od->ipam_info.ipv6_prefix_set;
>       struct in6_addr ip6;
>       if (dynamic_ip6) {
> -        in6_generate_eui64(mac, &od->ipam_info->ipv6_prefix, &ip6);
> +        in6_generate_eui64(mac, &od->ipam_info.ipv6_prefix, &ip6);
>       }
>   
>       /* If we didn't generate anything, bail out. */
> @@ -1156,7 +1135,8 @@ build_ipam(struct hmap *datapaths, struct hmap *ports)
>        * ports that have the "dynamic" keyword in their addresses column. */
>       struct ovn_datapath *od;
>       HMAP_FOR_EACH (od, key_node, datapaths) {
> -        if (!od->nbs || !od->ipam_info) {
> +        if (!od->nbs || (!od->ipam_info.allocated_ipv4s &&
> +                         !od->ipam_info.ipv6_prefix_set)) {
>               continue;
>           }
>   
>