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.iva...@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.iva...@cambridgegreys.com/ >>>>> >>>>> >>>>> https://patchwork.ozlabs.org/project/openvswitch/patch/20200706083650.29443-3-anton.iva...@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 >>>>> d...@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. Regards, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
