| Message ID | 20230227200344.1308604-2-i.maximets@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | northd: Optimize parsing of LSP addresses. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Mon, Feb 27, 2023 at 09:03:42PM +0100, Ilya Maximets wrote: > In many cases OVN components are using strlen() to check if the string > is not empty. In that case, unnecessary scan of the whole string is > performed and the length is calculated, while it's enough to just check > the first byte. > > Some functions also have completely unnecessary strlen() calls where > needed functionality can be achieved without them. > > The most impactful changes in this commit are around extract_addresses() > function, and extract_lsp_addresses() in particular. It is called by > northd for every logical port and involves 2 unnecessary strlen() calls. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Reviewed-by: Simon Horman <simon.horman@corigine.com>
Acked-by: Mark Michelson <mmichels@redhat.com> On 2/27/23 15:03, Ilya Maximets wrote: > In many cases OVN components are using strlen() to check if the string > is not empty. In that case, unnecessary scan of the whole string is > performed and the length is calculated, while it's enough to just check > the first byte. > > Some functions also have completely unnecessary strlen() calls where > needed functionality can be achieved without them. > > The most impactful changes in this commit are around extract_addresses() > function, and extract_lsp_addresses() in particular. It is called by > northd for every logical port and involves 2 unnecessary strlen() calls. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > controller/chassis.c | 4 ++-- > controller/encaps.c | 2 +- > controller/physical.c | 2 +- > controller/pinctrl.c | 6 +++--- > ic/ovn-ic.c | 4 ++-- > lib/ovn-util.c | 5 ++--- > northd/northd.c | 10 +++++----- > utilities/ovn-nbctl.c | 14 +++++++------- > 8 files changed, 23 insertions(+), 24 deletions(-) > > diff --git a/controller/chassis.c b/controller/chassis.c > index acde00849..0484e2fb9 100644 > --- a/controller/chassis.c > +++ b/controller/chassis.c > @@ -99,9 +99,9 @@ static const char * > get_hostname(const struct smap *ext_ids, const char *chassis_id) > { > const char *hostname = get_chassis_external_id_value(ext_ids, chassis_id, > - "hostname", ""); > + "hostname", NULL); > > - if (strlen(hostname) == 0) { > + if (!hostname) { > static char hostname_[HOST_NAME_MAX + 1]; > > if (gethostname(hostname_, sizeof(hostname_))) { > diff --git a/controller/encaps.c b/controller/encaps.c > index 5d383401d..2662eaf98 100644 > --- a/controller/encaps.c > +++ b/controller/encaps.c > @@ -77,7 +77,7 @@ tunnel_create_name(struct tunnel_ctx *tc, const char *chassis_id) > for (int i = 0; i < UINT16_MAX; i++) { > const char *idx = get_chassis_idx(tc->ovs_table); > char *port_name = xasprintf( > - "ovn%s-%.*s-%x", idx, strlen(idx) ? 5 : 6, chassis_id, i); > + "ovn%s-%.*s-%x", idx, idx[0] ? 5 : 6, chassis_id, i); > > if (!sset_contains(&tc->port_names, port_name)) { > return port_name; > diff --git a/controller/physical.c b/controller/physical.c > index e718268ea..ec861f49c 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -505,7 +505,7 @@ populate_remote_chassis_macs(const struct sbrec_chassis *my_chassis, > const char *tokens > = get_chassis_mac_mappings(&chassis->other_config, chassis->name); > > - if (!strlen(tokens)) { > + if (!tokens[0]) { > continue; > } > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 248b1a0e7..795847729 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -3775,12 +3775,12 @@ ipv6_ra_update_config(const struct sbrec_port_binding *pb) > > const char *dnssl = smap_get(&pb->options, "ipv6_ra_dnssl"); > if (dnssl) { > - ds_put_buffer(&config->dnssl, dnssl, strlen(dnssl)); > + ds_put_cstr(&config->dnssl, dnssl); > } > > const char *route_info = smap_get(&pb->options, "ipv6_ra_route_info"); > if (route_info) { > - ds_put_buffer(&config->route_info, route_info, strlen(route_info)); > + ds_put_cstr(&config->route_info, route_info); > } > > return config; > @@ -5825,7 +5825,7 @@ extract_addresses_with_port(const char *addresses, > int ofs; > if (!extract_addresses(addresses, laddrs, &ofs)) { > return false; > - } else if (ofs >= strlen(addresses)) { > + } else if (!addresses[ofs]) { > return true; > } > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > index 9a80a7f68..1d0a062f6 100644 > --- a/ic/ovn-ic.c > +++ b/ic/ovn-ic.c > @@ -1162,7 +1162,7 @@ add_static_to_routes_ad( > ipv6_format_addr(&nexthop, &msg); > } > > - ds_put_format(&msg, ", route_table: %s", strlen(nb_route->route_table) > + ds_put_format(&msg, ", route_table: %s", nb_route->route_table[0] > ? nb_route->route_table > : "<main>"); > > @@ -1348,7 +1348,7 @@ sync_learned_routes(struct ic_context *ctx, > continue; > } > > - if (strlen(isb_route->route_table) && > + if (isb_route->route_table[0] && > strcmp(isb_route->route_table, ts_route_table)) { > if (VLOG_IS_DBG_ENABLED()) { > VLOG_DBG("Skip learning static route %s -> %s as either " > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > index b12c027b9..47484ef01 100644 > --- a/lib/ovn-util.c > +++ b/lib/ovn-util.c > @@ -128,7 +128,6 @@ parse_and_store_addresses(const char *address, struct lport_addresses *laddrs, > const char *buf = address; > const char *const start = buf; > int buf_index = 0; > - const char *buf_end = buf + strlen(address); > > if (extract_eth_addr) { > if (!ovs_scan_len(buf, &buf_index, ETH_ADDR_SCAN_FMT, > @@ -151,7 +150,7 @@ parse_and_store_addresses(const char *address, struct lport_addresses *laddrs, > * and store in the 'laddrs'. Break the loop if invalid data is found. > */ > buf += buf_index; > - while (buf < buf_end) { > + while (*buf != '\0') { > buf_index = 0; > error = ip_parse_cidr_len(buf, &buf_index, &ip4, &plen); > if (!error) { > @@ -205,7 +204,7 @@ extract_lsp_addresses(const char *address, struct lport_addresses *laddrs) > int ofs; > bool success = extract_addresses(address, laddrs, &ofs); > > - if (success && ofs < strlen(address)) { > + if (success && address[ofs]) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > VLOG_INFO_RL(&rl, "invalid syntax '%s' in address", address); > } > diff --git a/northd/northd.c b/northd/northd.c > index 770a5b50e..6d37ea713 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -9623,7 +9623,7 @@ route_table_add(struct simap *route_tables, const char *route_table_name) > static uint32_t > get_route_table_id(struct simap *route_tables, const char *route_table_name) > { > - if (!route_table_name || !strlen(route_table_name)) { > + if (!route_table_name || !route_table_name[0]) { > return 0; > } > > @@ -9698,7 +9698,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *ports, > struct in6_addr nexthop; > unsigned int plen; > bool is_discard_route = !strcmp(route->nexthop, "discard"); > - bool valid_nexthop = strlen(route->nexthop) && !is_discard_route; > + bool valid_nexthop = route->nexthop[0] && !is_discard_route; > if (valid_nexthop) { > if (!ip46_parse_cidr(route->nexthop, &nexthop, &plen)) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > @@ -9989,7 +9989,7 @@ find_static_route_outport(struct ovn_datapath *od, const struct hmap *ports, > route->output_port, route->ip_prefix); > return false; > } > - if (strlen(route->nexthop)) { > + if (route->nexthop[0]) { > lrp_addr_s = find_lrp_member_ip(out_port, route->nexthop); > } > if (!lrp_addr_s) { > @@ -10021,7 +10021,7 @@ find_static_route_outport(struct ovn_datapath *od, const struct hmap *ports, > continue; > } > > - if (strlen(route->nexthop)) { > + if (route->nexthop[0]) { > lrp_addr_s = find_lrp_member_ip(out_port, route->nexthop); > } > if (lrp_addr_s) { > @@ -10337,7 +10337,7 @@ add_route(struct hmap *lflows, struct ovn_datapath *od, > } else { > ds_put_format(&common_actions, REG_ECMP_GROUP_ID" = 0; %s = ", > is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6); > - if (gateway && strlen(gateway)) { > + if (gateway && gateway[0]) { > ds_put_cstr(&common_actions, gateway); > } else { > ds_put_format(&common_actions, "ip%s.dst", is_ipv4 ? "4" : "6"); > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index ae4d6c403..45572fd30 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -4553,7 +4553,7 @@ nbctl_lr_route_add(struct ctl_context *ctx) > } > > cleanup: > - if (next_hop && strlen(next_hop)) { > + if (next_hop && next_hop[0]) { > free(next_hop); > } > free(prefix); > @@ -6590,12 +6590,12 @@ print_route(const struct nbrec_logical_router_static_route *route, > > if (!strcmp(route->nexthop, "discard")) { > next_hop = xasprintf("discard"); > - } else if (strlen(route->nexthop)) { > + } else if (route->nexthop[0]) { > next_hop = normalize_prefix_str(route->nexthop); > } > ds_put_format(s, "%25s %25s", prefix, next_hop); > free(prefix); > - if (strlen(next_hop)) { > + if (next_hop[0]) { > free(next_hop); > } > > @@ -6734,8 +6734,8 @@ nbctl_lr_route_list(struct ctl_context *ctx) > if (!i || (i > 0 && strcmp(route->route_table, > ipv4_routes[i - 1].route->route_table))) { > ds_put_format(&ctx->output, "%sRoute Table %s:\n", i ? "\n" : "", > - strlen(route->route_table) ? route->route_table > - : "<main>"); > + route->route_table[0] ? route->route_table > + : "<main>"); > } > > print_route(ipv4_routes[i].route, &ctx->output, ecmp); > @@ -6760,8 +6760,8 @@ nbctl_lr_route_list(struct ctl_context *ctx) > if (!i || (i > 0 && strcmp(route->route_table, > ipv6_routes[i - 1].route->route_table))) { > ds_put_format(&ctx->output, "%sRoute Table %s:\n", i ? "\n" : "", > - strlen(route->route_table) ? route->route_table > - : "<main>"); > + route->route_table[0] ? route->route_table > + : "<main>"); > } > > print_route(ipv6_routes[i].route, &ctx->output, ecmp);
diff --git a/controller/chassis.c b/controller/chassis.c index acde00849..0484e2fb9 100644 --- a/controller/chassis.c +++ b/controller/chassis.c @@ -99,9 +99,9 @@ static const char * get_hostname(const struct smap *ext_ids, const char *chassis_id) { const char *hostname = get_chassis_external_id_value(ext_ids, chassis_id, - "hostname", ""); + "hostname", NULL); - if (strlen(hostname) == 0) { + if (!hostname) { static char hostname_[HOST_NAME_MAX + 1]; if (gethostname(hostname_, sizeof(hostname_))) { diff --git a/controller/encaps.c b/controller/encaps.c index 5d383401d..2662eaf98 100644 --- a/controller/encaps.c +++ b/controller/encaps.c @@ -77,7 +77,7 @@ tunnel_create_name(struct tunnel_ctx *tc, const char *chassis_id) for (int i = 0; i < UINT16_MAX; i++) { const char *idx = get_chassis_idx(tc->ovs_table); char *port_name = xasprintf( - "ovn%s-%.*s-%x", idx, strlen(idx) ? 5 : 6, chassis_id, i); + "ovn%s-%.*s-%x", idx, idx[0] ? 5 : 6, chassis_id, i); if (!sset_contains(&tc->port_names, port_name)) { return port_name; diff --git a/controller/physical.c b/controller/physical.c index e718268ea..ec861f49c 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -505,7 +505,7 @@ populate_remote_chassis_macs(const struct sbrec_chassis *my_chassis, const char *tokens = get_chassis_mac_mappings(&chassis->other_config, chassis->name); - if (!strlen(tokens)) { + if (!tokens[0]) { continue; } diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 248b1a0e7..795847729 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -3775,12 +3775,12 @@ ipv6_ra_update_config(const struct sbrec_port_binding *pb) const char *dnssl = smap_get(&pb->options, "ipv6_ra_dnssl"); if (dnssl) { - ds_put_buffer(&config->dnssl, dnssl, strlen(dnssl)); + ds_put_cstr(&config->dnssl, dnssl); } const char *route_info = smap_get(&pb->options, "ipv6_ra_route_info"); if (route_info) { - ds_put_buffer(&config->route_info, route_info, strlen(route_info)); + ds_put_cstr(&config->route_info, route_info); } return config; @@ -5825,7 +5825,7 @@ extract_addresses_with_port(const char *addresses, int ofs; if (!extract_addresses(addresses, laddrs, &ofs)) { return false; - } else if (ofs >= strlen(addresses)) { + } else if (!addresses[ofs]) { return true; } diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c index 9a80a7f68..1d0a062f6 100644 --- a/ic/ovn-ic.c +++ b/ic/ovn-ic.c @@ -1162,7 +1162,7 @@ add_static_to_routes_ad( ipv6_format_addr(&nexthop, &msg); } - ds_put_format(&msg, ", route_table: %s", strlen(nb_route->route_table) + ds_put_format(&msg, ", route_table: %s", nb_route->route_table[0] ? nb_route->route_table : "<main>"); @@ -1348,7 +1348,7 @@ sync_learned_routes(struct ic_context *ctx, continue; } - if (strlen(isb_route->route_table) && + if (isb_route->route_table[0] && strcmp(isb_route->route_table, ts_route_table)) { if (VLOG_IS_DBG_ENABLED()) { VLOG_DBG("Skip learning static route %s -> %s as either " diff --git a/lib/ovn-util.c b/lib/ovn-util.c index b12c027b9..47484ef01 100644 --- a/lib/ovn-util.c +++ b/lib/ovn-util.c @@ -128,7 +128,6 @@ parse_and_store_addresses(const char *address, struct lport_addresses *laddrs, const char *buf = address; const char *const start = buf; int buf_index = 0; - const char *buf_end = buf + strlen(address); if (extract_eth_addr) { if (!ovs_scan_len(buf, &buf_index, ETH_ADDR_SCAN_FMT, @@ -151,7 +150,7 @@ parse_and_store_addresses(const char *address, struct lport_addresses *laddrs, * and store in the 'laddrs'. Break the loop if invalid data is found. */ buf += buf_index; - while (buf < buf_end) { + while (*buf != '\0') { buf_index = 0; error = ip_parse_cidr_len(buf, &buf_index, &ip4, &plen); if (!error) { @@ -205,7 +204,7 @@ extract_lsp_addresses(const char *address, struct lport_addresses *laddrs) int ofs; bool success = extract_addresses(address, laddrs, &ofs); - if (success && ofs < strlen(address)) { + if (success && address[ofs]) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); VLOG_INFO_RL(&rl, "invalid syntax '%s' in address", address); } diff --git a/northd/northd.c b/northd/northd.c index 770a5b50e..6d37ea713 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -9623,7 +9623,7 @@ route_table_add(struct simap *route_tables, const char *route_table_name) static uint32_t get_route_table_id(struct simap *route_tables, const char *route_table_name) { - if (!route_table_name || !strlen(route_table_name)) { + if (!route_table_name || !route_table_name[0]) { return 0; } @@ -9698,7 +9698,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *ports, struct in6_addr nexthop; unsigned int plen; bool is_discard_route = !strcmp(route->nexthop, "discard"); - bool valid_nexthop = strlen(route->nexthop) && !is_discard_route; + bool valid_nexthop = route->nexthop[0] && !is_discard_route; if (valid_nexthop) { if (!ip46_parse_cidr(route->nexthop, &nexthop, &plen)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); @@ -9989,7 +9989,7 @@ find_static_route_outport(struct ovn_datapath *od, const struct hmap *ports, route->output_port, route->ip_prefix); return false; } - if (strlen(route->nexthop)) { + if (route->nexthop[0]) { lrp_addr_s = find_lrp_member_ip(out_port, route->nexthop); } if (!lrp_addr_s) { @@ -10021,7 +10021,7 @@ find_static_route_outport(struct ovn_datapath *od, const struct hmap *ports, continue; } - if (strlen(route->nexthop)) { + if (route->nexthop[0]) { lrp_addr_s = find_lrp_member_ip(out_port, route->nexthop); } if (lrp_addr_s) { @@ -10337,7 +10337,7 @@ add_route(struct hmap *lflows, struct ovn_datapath *od, } else { ds_put_format(&common_actions, REG_ECMP_GROUP_ID" = 0; %s = ", is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6); - if (gateway && strlen(gateway)) { + if (gateway && gateway[0]) { ds_put_cstr(&common_actions, gateway); } else { ds_put_format(&common_actions, "ip%s.dst", is_ipv4 ? "4" : "6"); diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c index ae4d6c403..45572fd30 100644 --- a/utilities/ovn-nbctl.c +++ b/utilities/ovn-nbctl.c @@ -4553,7 +4553,7 @@ nbctl_lr_route_add(struct ctl_context *ctx) } cleanup: - if (next_hop && strlen(next_hop)) { + if (next_hop && next_hop[0]) { free(next_hop); } free(prefix); @@ -6590,12 +6590,12 @@ print_route(const struct nbrec_logical_router_static_route *route, if (!strcmp(route->nexthop, "discard")) { next_hop = xasprintf("discard"); - } else if (strlen(route->nexthop)) { + } else if (route->nexthop[0]) { next_hop = normalize_prefix_str(route->nexthop); } ds_put_format(s, "%25s %25s", prefix, next_hop); free(prefix); - if (strlen(next_hop)) { + if (next_hop[0]) { free(next_hop); } @@ -6734,8 +6734,8 @@ nbctl_lr_route_list(struct ctl_context *ctx) if (!i || (i > 0 && strcmp(route->route_table, ipv4_routes[i - 1].route->route_table))) { ds_put_format(&ctx->output, "%sRoute Table %s:\n", i ? "\n" : "", - strlen(route->route_table) ? route->route_table - : "<main>"); + route->route_table[0] ? route->route_table + : "<main>"); } print_route(ipv4_routes[i].route, &ctx->output, ecmp); @@ -6760,8 +6760,8 @@ nbctl_lr_route_list(struct ctl_context *ctx) if (!i || (i > 0 && strcmp(route->route_table, ipv6_routes[i - 1].route->route_table))) { ds_put_format(&ctx->output, "%sRoute Table %s:\n", i ? "\n" : "", - strlen(route->route_table) ? route->route_table - : "<main>"); + route->route_table[0] ? route->route_table + : "<main>"); } print_route(ipv6_routes[i].route, &ctx->output, ecmp);
In many cases OVN components are using strlen() to check if the string is not empty. In that case, unnecessary scan of the whole string is performed and the length is calculated, while it's enough to just check the first byte. Some functions also have completely unnecessary strlen() calls where needed functionality can be achieved without them. The most impactful changes in this commit are around extract_addresses() function, and extract_lsp_addresses() in particular. It is called by northd for every logical port and involves 2 unnecessary strlen() calls. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- controller/chassis.c | 4 ++-- controller/encaps.c | 2 +- controller/physical.c | 2 +- controller/pinctrl.c | 6 +++--- ic/ovn-ic.c | 4 ++-- lib/ovn-util.c | 5 ++--- northd/northd.c | 10 +++++----- utilities/ovn-nbctl.c | 14 +++++++------- 8 files changed, 23 insertions(+), 24 deletions(-)