On 1/29/21 2:22 PM, Dumitru Ceara wrote:
On 1/29/21 7:59 PM, Mark Michelson wrote:
Hi Dumitru,
Hi Mark,
Thanks for the review!
Overall, this looks like a good patch series. I don't have any
specific findings, but I have some general points/questions.
1) There are no tests. With the isolated lflow-cache.[hc] API, adding
unit tests seems like it wouldn't be too difficult.
You're right, I was a bit sloppy, I'll add tests in v2.
2) Has there been any proven benefit to LCACHE_T_CONJ_ID? Reading
through consider_logical_flow(), LCACHE_T_CONJ_ID and LCACHE_T_NONE
require the same work to be done. I suppose it allows more time before
the conj_id overflows, but is that really worth taking up potentially
limited cache space?
The reason for caching conj_id offsets is to not disrupt traffic during
lflow processing (incremental or not), and was added by:
2662498bfd13 ("ovn-controller: Persist the conjunction ids allocated for
conjuctive matches.")
We could add more knobs to configure what types of lflow cache entries
we want stored or not. On the other hand that means more knobs :)
What do you think?
No more knobs!
I reviewed that particular commit back before it went in. It had slipped
my mind about how persisting the conj_ids means not having to rewrite
the OF, and how that results in less interruptions in traffic.
So for the purposes of reducing ovn-controller CPU, LCACHE_T_CONJ_ID
isn't very beneficial, but it still has some importance.
3) It seems like cached lflows of type LCACHE_T_MATCHES are the most
valuable, since caching them saves the most amount of work in
consider_logical_flow(). Is there any way to give preference to these
types of lflows over others? It would be a shame if the cache filled
up with LCACHE_T_EXPR and LCACHE_T_CONJ_ID lflows, and that prevented
LCACHE_T_MATCHES flows from being cached as a result.
That's a good idea, I'll think of a way to evacuate "less important"
cache entries if we're at the limit of the cache size.
4) We calculate the memory of each lflow_cache_value whenever it is
added to the cache. Does this incur a noticeable cost? If the
lflow_cache has no memory limit, then would it be more beneficial not
to calculate the memory when the value is added, but instead wait
until a memory report is requested?
Initially I tried computing the expr/matches sizes on the fly while
parsing/building matches but that turned out to be quite complex due to
how expressions are parsed/combined/simplified. I'll have another go at
it though.
Otherwise, your suggestion is reasonable, if there's no memory limit it
might make sense to postpone computing sizes.
I did run some scale tests and I didn't notice any noticeable hit in
performance even with the additional size computations.
My suggestion hinges on noticeable performance drops. If scale tests
don't show anything obvious from calculating memory usage on each cache
add, then it probably isn't necessary to change what you've already written.
Thanks,
Dumitru
On 1/28/21 11:25 AM, Dumitru Ceara wrote:
Scale tests have identified the lflow cache to be one of the main memory
consumers in ovn-controller. This series refactors the lflow cache code
and adds configuration knobs to limit the size (in lines and/or memory)
of the cache.
Dumitru Ceara (6):
lflow: Refactor convert_match_to_expr() to explicitly consume
prereqs.
lflow-cache: Move the lflow cache to its own module.
lflow-cache: Add coverage counters.
lflow-cache: Reclaim heap memory after cache flush.
lflow-cache: Make maximum number of cache entries configurable.
lflow-cache: Make max cache memory usage configurable.
NEWS | 5 +
configure.ac | 1
controller/automake.mk | 2
controller/chassis.c | 44 +++++
controller/lflow-cache.c | 282
++++++++++++++++++++++++++++++++
controller/lflow-cache.h | 81 +++++++++
controller/lflow.c | 349
+++++++++++++--------------------------
controller/lflow.h | 6 -
controller/ovn-controller.8.xml | 23 +++
controller/ovn-controller.c | 65 ++++---
include/ovn/expr.h | 2
lib/expr.c | 46 +++++
12 files changed, 635 insertions(+), 271 deletions(-)
create mode 100644 controller/lflow-cache.c
create mode 100644 controller/lflow-cache.h
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
