[ovs-dev] [PATCH ovn 0/6] ovn-controller: Make lflow cache size configurable.

[Mark Michelson]

Hi Dumitru,

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.

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?

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.

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?

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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>