[ovs-dev] [PATCH RFC ovn 0/3] Thread-local storage for logical flow matches.

[Mark Michelson]

NOTE: This is an RFC. This absolutely should not be committed as-is.

Based on recent patches that have been submitted to ovn-northd.c, I
noticed that dynamic string creation and destruction are hot topics.
Patches have focused on re-using dynamic strings when possible.

I took this to a logical extreme for this series. Rather than declaring
"match" dynamic strings on the stack, I instead create a thread-local
dynamic string that can be requested at will. This way, rather than
declaring dynamic strings on the stack, I grab the thread-local buffer
and modify it. This means that the vast majority of the time, we're
working with the same dynamic string the entire time.

To give an idea in the savings this patch series provides, I
instrumented the OVS code to add a coverage counter for dynamic string
resizing. Each time that ds_reserve() needs to allocate space, the
coverage counter increases. In a sandbox, I ran a script (included at
the bottom of this message) in master and in a branch containing my
changes.

unchanged master:
ds_resize                185641.6/sec 87241.600/sec     1454.0267/sec   total: 5389079

modfiied ovn-northd:
ds_resize                110786.8/sec 39190.017/sec      653.1669/sec   total: 2700484

That is an approximate 50% decrease in the overall number of dynamic
string resize operations.

Keep in mind that this patch only targets the "easy" conversions for
matches. It's possible that through some more complex changes, we
could target more dynamic strings [1]. If the same approach were taken for
the "actions" dynamic strings, we might see an even more drastic decrease.

It's not all upsides, though. When I run the test script, there is no
discernable difference in execution time between master and when this
patch series is used. It's possible that at a higher scale, we'd see a
difference, but who knows.

The other, bigger, issue is that this comes with the downsides that
global variables bring. It removes referential transparency from a lot of
functions. Even when taking care of "easy" replacements in this patch
series I found myself introducing bugs in the process that I had to fix.

The goal of this RFC is to determine if the positives of this approach
outweigh the negatives. Should we implement something like this for
matches and actions in ovn-northd.c?

[1] It would be *really* nice if dynamic strings had a prepend
operation.

---
#!/bin/bash

# When I ran this test, I used the default values for all parameters.
#
# Don't try to make sense of the logical network, NATS, Load balancers,
# etc. This was designed just to use a lot of common OVN features, and
# therefore exercise logical flow creation.

num_switches=${NUM_SWITCHES:-50}
ports_per_switch=${PORTS_PER_SWITCH:-10}
num_gw_routers=${NUM_GW_ROUTERS:-$num_switches}

ovn-nbctl ls-add join

for i in $(seq 1 "$num_switches")
do
	switch=ls"$i"
	ihex=$(printf %02x $i)
	ovn-nbctl ls-add "$switch"
	for j in $(seq 1 "$ports_per_switch")
	do
		jhex=$(printf %02x $j)
		port=lsp"$i"-"$j"
		mac=00:00:00:00:"$ihex":"$jhex"
		ip=192.168."$i"."$j"
		ovn-nbctl lsp-add "$switch" "$port"
		ovn-nbctl lsp-set-addresses "$port" "$mac $ip"
		ovn-nbctl lsp-set-port-security "$port" "$mac $ip"

		ovn-nbctl acl-add "$switch" to-lport 300 "ip4.src == $ip && ip4.dst == 4.4.4.4" allow
		ovn-nbctl acl-add "$switch" from-lport 400 "ip4.dst == $ip && tcp.port == 80" allow-related
	done
	router=lr"$i"
	router_port=lr"$i"-"$switch"
	switch_port=ls"$i"-"$router"
	ovn-nbctl lr-add "$router"
	ovn-nbctl lrp-add "$router" "$router_port" "$ihex":00:00:00:00:01 10.0.0."$i"/8
	ovn-nbctl lsp-add "$switch" "$switch_port" -- \
		      lsp-set-options "$switch_port" router-port="$router_port" -- \
			  lsp-set-type "$switch_port" router
	join_router_port=lr"$i"-join
	join_switch_port=join-lr"$i"
	ovn-nbctl lrp-add "$router" "$join_router_port" 00:"$ihex":00:00:00:01 172.16.0."$i"/16
	ovn-nbctl lsp-add join "$join_switch_port" -- \
		      lsp-set-options "$join_switch_port" router-port="$join_router_port" -- \
			  lsp-set-type "$join_switch_port" router
done

for i in $(seq 1 "$num_gw_routers")
do
	ihex=$(printf %02x $i)
	gw_router=lrgw"$i"
	gw_router_port=lrp-"$gw_router"
	chassis=hv"$i"
	join_gw_router_port="$gw_router"-join
	join_gw_switch_port=join-"$gw_router"
	lb=lb-"$gw_router"
	ovn-nbctl lr-add "$gw_router"
	ovn-nbctl lrp-add "$gw_router" "$gw_router_port" 00:00:"$ihex":00:00:01 50.0.0."$i"
	ovn-nbctl lrp-set-gateway-chassis "$gw_router_port" "$chassis" 400
	ovn-nbctl lrp-add "$gw_router" "$join_gw_router_port" 00:00:00:"$ihex":00:01 172.16."$i".1/16
	ovn-nbctl lsp-add join "$join_gw_switch_port" -- \
	          lsp-set-options "$join_gw_switch_port" router-port="$join_gw_router_port" -- \
			  lsp-set-type "$join_gw_switch_port" router
	ovn-nbctl lr-nat-add "$gw_router" dnat 50.0.0."$i" 192.168.0."$i"
	ovn-nbctl lr-nat-add "$gw_router" snat 50.0.0."$i" 192.168.0."$i"
	ovn-nbctl lb-add "$lb" 50.0."$i".1:80 192.168.0."$i":80 tcp
	ovn-nbctl lr-lb-add "$gw_router" "$lb"
done
---
Mark Michelson (3):
  northd: Use thread-local storage for logical flow matches.
  northd: Do some of the more complex match dynamic string alterations.
  northd: Memory cleanup for thread-local match.

 northd/ovn-northd.c | 596 ++++++++++++++++++++++----------------------
 1 file changed, 292 insertions(+), 304 deletions(-)

-- 
2.31.1