On 2/6/24 22:22, Ilya Maximets wrote:
>>>
>>> I did some testing with my patch and these are the findings
>>>
>>>                                      | Avg. Poll Intervals | Total
>>> test time |  northd RSS
>>> ------------------------------ +------------------------+---------------
>>> Last week                     |      1.5 seconds     |  1005 seconds
>>> |    2.5 GB
>>> This week                     |      6  seconds       |  2246 seconds
>>>  |    8.5 GB
>>> Ilya's Patch                   |     2.5 seconds      |  1170 seconds
>>>  |    3.1 GB
>>> Numan's patch (run1)   |     1.6 seconds      |  992 seconds    |    2.43 GB
>>> Numan's patch (run2)   |     1.7 seconds      |  1022 seconds   |    2.43 GB
>>>
>>> Seems like removing dp ref cnt all together is more efficient and very 
>>> close to
>>> last week's results.
>>>
>>> I'll submit the patch tomorrow after some more testing.
>>>
>>> Feel free to provide any review comments or feedback if you've any.
>>>
>>> Thanks
>>> Numan
>>>
>>>>>
>>>>> Regarding the cluster density 500 node test with Ilya's fix,  I think
>>>>> this seems o.k to me since
>>>>> the test falls back to recompute a lot and there is some cost now with
>>>>> the lflow I-P patches.
>>>>> This can be improved by adding port group I-P.   I can start looking
>>>>> into the port-group I-P if you
>>>>> think it would be worth it.  Let me know.
>>>>>
>>>>
>>>> Adding more I-P would be helpful, but it still doesn't explain the
>>>> observation from Ilya regarding the 66% increase in recompute time and 25%
>>>> increase in memory. My test cases couldn't reproduce such significant
>>>> increases (it is ~10-20% in my test). I think we'd better figure out what
>>>> caused such big increases - is it still related to ref count or is it
>>>> something else. I think the lflow_ref might have contributed to it,
>>>> especially the use of hmap instead of list. It is totally fine if the
>>>> lflow_ref adds some cost, which is required for the I-P to work, but it is
>>>> better to understand where the cost comes from and justify it (or optimize
>>>> it).
>>>
>>> Looks like from my patch results,  dp_ref_cnt is contributing to this 
>>> increase.
>>> Is it possible for you to trigger another test with my patch -
>>> https://github.com/numansiddique/ovn/commit/92f6563d9e7b1e6c8f2b924dea7a220781e10a05
>>>  
>>> <https://github.com/numansiddique/ovn/commit/92f6563d9e7b1e6c8f2b924dea7a220781e10a05>
>>>
>> It is great to see that removing ref count resolved the performance gap. The 
>> test result is actually very interesting. I also had a test with more LBs 
>> (10k instead of the earlier 1k) in the large LBG, trying to make the cost of 
>> bitmap scan more obvious, but still, the difference between your patch and 
>> Ilya's patch is not significant in my test. The latency difference is within 
>> 5%, and the memory <1%. So I wonder why in the ovn-heater test it is so 
>> different. Is it possible that in the ovn-heater test it does create a lot 
>> of duplicated lflows so ref count hmap operations are actually triggered for 
>> a significant amount of times, which might have contributed to the major 
>> cost even with Ilya's patch, and with your patch it means it fall back to 
>> recompute much more often?
> 
> FWIW, in my testing with the cluster-density 500node database there
> are 12 M refcounts allocated.  With 32 bytes each, it's about 370 MB
> of RAM.  We should also add a fair share of allocation overhead since
> we're allocating a huge number of very small objects.
> 
> Numan also gets rid of all the space allocated for hash maps that hold
> all these refcounts, and these were taking ~25% of all allocations.
> 
> Getting rid of these allocations likely saves a lot of CPU cycles as
> well, since they are very heavy.
> 
> P.S.
> Almost all of these 12M refcounts are for:
>   build_lrouter_defrag_flows_for_lb():ovn_lflow_add_with_dp_group()
> All of these have refcount == 3.
> 
> There is one with refcount 502:
>   build_egress_delivery_flows_for_lrouter_port():ovn_lflow_add_default_drop()
> 
> And there are 500-ish with refcount of 2:
>   build_lswitch_rport_arp_req_flow():ovn_lflow_add_with_hint()

On the current main we allocate ~120 M refcounts.

> 
> Best regards, Ilya Maximets.

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to