[ovs-dev] RFC DEMO of using parallel processing in OVN

[Anton Ivanov]

On 15/07/2020 11:04, Dumitru Ceara wrote:
> On 7/15/20 11:54 AM, Anton Ivanov wrote:
>> On 15/07/2020 09:57, Dumitru Ceara wrote:
>>> On 7/14/20 6:06 PM, Anton Ivanov wrote:
>>>> On 14/07/2020 16:27, Dumitru Ceara wrote:
>>>>> On 7/10/20 10:39 AM, anton.ivanov at cambridgegreys.com wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> This is a series of patches to demo the use in OVN of the parallel
>>>>>> processing of hashes patchset which I submitted to OVS.
>>>>>>
>>>>>> The OVS patch series required for this patch can be found here:
>>>>>>
>>>>>> https://patchwork.ozlabs.org/project/openvswitch/patch/20200706083650.29443-2-anton.ivanov@cambridgegreys.com/
>>>>>>
>>>>>>
>>>>>> https://patchwork.ozlabs.org/project/openvswitch/patch/20200706083650.29443-3-anton.ivanov@cambridgegreys.com/
>>>>>>
>>>>>>
>>>>>>
>>>>>> or here:
>>>>>>
>>>>>> https://github.com/kot-begemot-uk/ovs/tree/parallel-submit-final
>>>>>>
>>>>>> They improve the performance and stability of ovn in a scale test.
>>>>>> I have used 64+ fake nodes in my scale testing. Your mileage may
>>>>>> vary depending on the performance of the systems used to run the
>>>>>> tests.
>>>>>>
>>>>>> The patchset covers most parts of ovn-northd which look amenable to
>>>>>> parallel processing. It may be possible to expand it to also cover
>>>>>> initial datapath hash generation and sb_only/nb_only lists, but that
>>>>>> does not look like it is worth it except for extremely large datasets.
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> dev mailing list
>>>>>> dev at openvswitch.org
>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>>>
>>>>> Hi Anton,
>>>>>
>>>>> Would you mind rebasing and sending a v2 of this RFC? For some reason
>>>>> the emails for the first two patches don't show up in patchwork:
>>>> I think I know what it is. I will try to find it and fix it in the next
>>>> revision. There is something somewhere which is not ASCII clean and I
>>>> had UTF8 warnings for these.
>>>>
>>> Hi Anton,
>>>
>>> Ok. There might be more issues though. If I patch your first patch
>>> manually, the second patch doesn't apply cleanly. Patch 3 applies ok
>>> afterwards but #4 fails.
>>>
>>>>> https://patchwork.ozlabs.org/project/openvswitch/list/?series=188809
>>>>>
>>>>> Also, there were changes in OVN master in the meantime and trying to
>>>>> apply your changes manually also fails due to conflicts in quite a few
>>>>> places.
>>>> That unfortunately will be the case with the next version after a few
>>>> commits to master.
>>>>
>>>> As long as the code follows the current layout, any version which tries
>>>> to re-organize processing will be very painful to rebase.
>>>>
>>> I agree, unfortunately I don't know what we can do to avoid that right
>>> now.
>>>
>>>> It may be a good idea to apply something like patches 1 from this series
>>>> (the one that got lost). It splits out the sequential code in the
>>>> lswitch and lrouter lflow build into a sequence of procedures.
>>> Sounds good, let's try that. To make that happen it's better to send the
>>> series as non-rfc and mention explicitly in the cover letter that the
>>> first patch doesn't depend on any unapplied OVS patches. Like that, once
>>> acked, patch 1 could be applied on its own.
>>>
>>>> This makes any future changes much easier to merge.
>>>>
>>> To save a round of reviews, I have some comments on patch #1. It might
>>> be good to address them before sending a new version of the series.
>>>
>>> Also I think it might make review easier if patch #1 is split in
>>> multiple smaller patches (that would get applied together), one per
>>> function. Right now it's quite hard to compare the old version with the
>>> new one.
>>>
>>> Regarding patch #4, I think it should be split in (at least) two parts:
>>> - refactoring of lflow generation for logical switches: this should
>>> happen in the beginning of the series like for logical router flows.
>>> - parallelization of the lflow generation.
>> I am going to split the whole sequence in much smaller chunks and I will
>> deliberately preserve the existing indent.
>>
>> One of the reasons why git cannot figure out what is what and merge
>> anything is that moving code to per-stage functions will change the
>> indent one step to the left. This marks the whole block as a change for
>> git.
>>
>> If the original indent is kept temporarily "as is" (despite this being
>> an obvious style violation), the diffs should be become possible to rebase.
>>
>> So I apologize in advance for the next series being "ugly". If we decide
>> to use this and it is merged we can always tidy up the indents at that
>> point.
>>
> I'll let other people comment on this too. From my perspective though,
> if the commit is relatively small, e.g., introducing only one refactored
> function at a time, it's desired to have the correct indentation from
> the beginning.

The full patch sequence is on the list.

It should be significantly faster than the original one too. I will try to run some performance tests on it overnight.

A.

>
> Regards,
> Dumitru
>
>
-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/