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

Dumitru Ceara dceara at redhat.com
Fri Jan 29 19:22:30 UTC 2021


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?

> 
> 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.

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



More information about the dev mailing list