On Wed, Oct 15, 2025 at 11:12:20AM +0200, Dumitru Ceara via dev wrote: > Hi Ales, Mark, > > On 10/15/25 10:23 AM, Ales Musil wrote: > > On Fri, Oct 10, 2025 at 9:58 PM Mark Michelson <[email protected]> wrote: > > > >> On Fri, Oct 10, 2025 at 3:43 AM Ales Musil <[email protected]> wrote: > >>> > >>> > >>> > >>> On Fri, Oct 10, 2025 at 2:51 AM Mark Michelson <[email protected]> > >> wrote: > >>>> > >>>> On Thu, Oct 9, 2025 at 5:25 PM Mark Michelson <[email protected]> > >> wrote: > >>>>> > >>>>> Hi Ales, > >>>>> > >>> > >>> > >>> Hi Mark, > >>> > >>> thank you for the review, but I'm probably missing something here. > >>> > >>> > >>>> > >>>>> I've been thinking through this change, and I think the idea might > >> have some flaws. Let's consider an arbitrary engine node B that takes > >> engine node A as input. In this scenario, engine node B writes to the SB DB > >> and engine node A does not. With this change, we mark engine node B as > >> SB_WRITE, but we do not do the same with node A. > >>>>> > >>> > >>> > >>> Why would we need to? A state doesn't depend on SB being writable. > >>> > >>>> > >>>>> We perform an engine run where recomputation is not allowed because > >> the SB DB is read only. When engine node A runs, since it does not have > >> SB_WRITE specified, it is able to recompute its data. However, when engine > >> node B runs, it cannot recompute its data. > >>>>> > >>>>> Now, what happens during the next engine run? > >>>>> * Let's say engine node A has no new input data. Engine node A's > >> state will be EN_UNCHANGED. In this case, engine node B sees that engine > >> node A is unchanged, so engine node B does not run. However, B really > >> should run because in the previous engine node run, A recomputed. In this > >> case, B's data is incorrect. > >>>>> * Let's say engine node A has new data and can process it > >> incrementally. Engine node A's state will be EN_UPDATED. > >>>>> * Engine node B may also try to incrementally handle A's tracked > >> data. However, the only tracked data that B will see is from the current > >> engine run. The data that A recomputed in the previous engine run will be > >> unknown to B. In this case, B's data is incorrect. > >>>>> * Engine node B may not have an input handler for A. In this > >> case, B will recompute and all will be well. > >>>>> * Let's say engine node A has new data and cannot process it > >> incrementally. In this case, A will recompute, and then B will also likely > >> recompute. The data integrity is fine in this case. However, A has had to > >> recompute twice. Before this change, A would only have recomputed on the > >> second engine run. > >>>>> > >>>>> In 2 of the 4 cases, B ends up with incorrect data. In 1 of the 4 > >> cases, the data is correct in engine node B, but A has to recompute twice. > >> And in 1 of the 4 cases, the data is correct in engine node B and we > >> successfully avoided an unnecessary recomputation of A. The only time this > >> change has a positive effect is if the SB_WRITE node has no input handlers > >> for its input nodes, and the input nodes are able to incrementally process > >> their data. > >>>> > >>>> I've realized that in the scenario I outlined, a couple of those > >>>> outcomes can't happen. When the first engine run happens, B will fail > >>>> to recompute, and the engine run will be canceled. In ovn-controller, > >>>> if an engine run is canceled, then we call > >>>> engine_set_force_recompute_immediate(). This means that the next run > >>>> will force all nodes to recompute. This means that the second and > >>>> third outcomes (both based on A being able to incrementally process > >>>> its data) cannot happen. Also, when engine_force_recompute is true, > >>>> the state of input nodes is not considered. This means that if A is > >>>> unchanged, it does not matter. B will still recompute and will > >>>> therefore have all of A's data. So my concerns about data integrity > >>>> are not correct. > >>> > >>> > >>> > >>> So none of the outcomes highlighted cannot happen, if B is cancelled > >>> we will immediately run force recompute loop that will set everything > >>> straight. > >>> > >>>> > >>>> > >>>> However, the fact that the engine forces all nodes to recompute means > >>>> that A will recompute on both engine runs. This means that with the > >>>> patch, A will recompute two times across the two engine runs. Without > >>>> the patch, A only recomputes once. So I still think the patch is > >>>> hurting more than it is helping. > >>> > >>> > >>> That's the case only in the worst scenario. Keep in mind that this > >>> doesn't always happen. So it's like all of the sudden we will do two > >>> recmputes for certain nodes back to back. In fact I see the opposite > >>> happening. Only 3 nodes in ovn-controller are marked as SB_WRITE any > >>> of those nodes might cancel but the rest is free to recompute however > >>> they like. We saw that there has been cancels for nodes do not > >>> require SB write and this was harming performance a good example are > >>> nodes pflow_output and lflow_output. Those are very heavy CPU time > >>> consumers, now they are free to recompute even if SB is read only > >>> during the current run. > >> > >> Thanks for the explanation Ales. I was coming at this change with much > >> more familiarity with the northd incremental engine than the > >> ovn-controller incremental engine. In northd, a lot of nodes are > >> eventual inputs for en_northd or en_lflow, both of which write to the > >> SB DB. So in most cases, even if a node doesn't write to the SB DB, > >> then it eventually will feed into a node that does write to the SB DB > >> [1]. In my head, it felt like the "common" case was for this to > >> happen. However, in ovn-controller, a good majority of the nodes do > >> not write to the SB DB and several of those nodes are never inputs > >> (directly or indirectly) to nodes that write to the SB DB. As you > >> pointed out, pflow_output and lflow_output are good examples of this. > >> > >> The situation I've outlined is still possible in ovn-controller. Many > >> nodes are inputs for en_runtime_data. Letting those nodes recompute > >> when en_runtime_data may eventually be denied the right to do so could > >> be a waste of CPU resources. > >> > >> What if the change were altered a bit: if an engine node is marked as > >> SB_WRITE, then any nodes that are upstream from that node (i.e. any > >> node that is a direct or indirect input to that node) is also marked > >> as SB_WRITE by the incremental engine. Meanwhile, any nodes that are > >> downstream from SB_WRITE nodes are not. This way, nodes like > >> pflow_output and lflow_output can recompute even when the SB database > >> is read-only. But nodes that input into runtime_data, route_exchange, > >> and garp_rarp can avoid unnecessary recomputes. > >> > >> What do you think? > >> > > > > Your suggestion will unfortunately cause the change to not have any > > benefit, in other words the state would be the same as on main right > > now. For example the runtime_data node has several inputs a lot of > > them are DB nodes e.g. sb_chassis. By marking them also as SB write > > or "canceled if recompte is not allowed" if you will, would basically > > mean it would mostly cancel as without the change. The leaf nodes, > > usually the DB ones are always recomputing. They cannot have any > > handlers. So I'm afraid that this change would then have the only > > benefit of documenting which node can write into SB DB. > > > > But to check if my theory is correct I did run ovn-heater with a > > baseline from main collecting the engine stats. Below you can see > > the results: > > > > --------------------------- > > main-250-density-heavy-ipv4 > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 1472 ms > > avg : 129 ms > > 95% : 537 ms > > total : 13750 > > failures: 0 > > > > IP stats: > > Recompute: 328312207, Compute: 62608243, Cancel: 2067 > > (0.0006295836572412308%) > > > > --------------------------- > > sb-ro-250-density-heavy-ipv4 > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 1343 ms > > avg : 130 ms > > 95% : 577 ms > > total : 13750 > > failures: 0 > > > > IP stats: > > Recompute: 325154128, Compute: 62002527, Cancel: 2138 > > (0.0006575343247679758%) > > > > --------------------------- > > main-250-density-heavy-ipv4-ic > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 119 ms > > avg : 26 ms > > 95% : 48 ms > > total : 13750 > > failures: 0 > > > > IP stats: > > Recompute: 77772935, Compute: 4420305, Cancel: 627 (0.0008061930541775233%) > > > > > > --------------------------- > > sb-ro-250-density-heavy-ipv4-ic > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 131 ms > > avg : 26 ms > > 95% : 47 ms > > total : 13750 > > failures: 0 > > > > IP stats: > > Recompute: 77918353, Compute: 4434319, Cancel: 533 (0.0006840493663925366%) > > > > --------------------------- > > main-250-density-heavy-ipv6 > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 1330 ms > > avg : 134 ms > > 95% : 611 ms > > total : 13750 > > failures: 0 > > > > IP stats: > > Recompute: 325845839, Compute: 62012707, Cancel: 1724 > > (0.0005290845527722083%) > > > > --------------------------- > > sb-ro-250-density-heavy-ipv6 > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 1376 ms > > avg : 133 ms > > 95% : 569 ms > > total : 13750 > > failures: 0 > > > > IP stats: > > Recompute: 324545521, Compute: 61835316, Cancel: 2680 > > (0.0008257701390369827%) > > > > --------------------------- > > main-250-density-heavy-ipv6-ic > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 174 ms > > avg : 26 ms > > 95% : 48 ms > > total : 13750 > > failures: 0 > > > > IP stats: > > Recompute: 76480809, Compute: 4401000, Cancel: 686 (0.0008969570392488919%) > > > > --------------------------- > > sb-ro-250-density-heavy-ipv6-ic > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 172 ms > > avg : 26 ms > > 95% : 48 ms > > total : 13750 > > failures: 0 > > > > IP stats: > > Recompute: 78371328, Compute: 4493119, Cancel: 615 (0.0007847257609313447%) > > > > --------------------------- > > main-250-density-light-ipv4 > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 1991 ms > > avg : 79 ms > > 95% : 124 ms > > total : 63750 > > failures: 0 > > > > IP stats: > > Recompute: 1787284835, Compute: 339628410, Cancel: 3089 > > (0.0001728319929486785%) > > > > --------------------------- > > sb-ro-250-density-light-ipv4 > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 2465 ms > > avg : 82 ms > > 95% : 135 ms > > total : 63750 > > failures: 0 > > > > IP stats: > > Recompute: 1744830863, Compute: 334913315, Cancel: 2439 > > (0.00013978432246472707%) > > > > --------------------------- > > main-250-density-light-ipv4-ic > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 123 ms > > avg : 24 ms > > 95% : 34 ms > > total : 63750 > > failures: 0 > > > > IP stats: > > Recompute: 47999536, Compute: 5072743, Cancel: 630 (0.001312512687622647%) > > > > --------------------------- > > sb-ro-250-density-light-ipv4-ic > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 129 ms > > avg : 24 ms > > 95% : 34 ms > > total : 63750 > > failures: 0 > > > > IP stats: > > Recompute: 48054016, Compute: 5090679, Cancel: 522 (0.0010862775756348855%) > > > > --------------------------- > > main-250-density-light-ipv6 > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 2181 ms > > avg : 79 ms > > 95% : 141 ms > > total : 63750 > > failures: 0 > > > > IP stats: > > Recompute: 1778682312, Compute: 337995667, Cancel: 1309 > > (0.00007359380543499778%) > > > > --------------------------- > > sb-ro-250-density-light-ipv6 > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 1993 ms > > avg : 76 ms > > 95% : 128 ms > > total : 63750 > > failures: 0 > > > > IP stats: > > Recompute: 1775718929, Compute: 337788420, Cancel: 1527 > > (0.0000859933390956154%) > > > > --------------------------- > > main-250-density-light-ipv6-ic > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 179 ms > > avg : 27 ms > > 95% : 41 ms > > total : 63750 > > failures: 0 > > > > IP stats: > > Recompute: 47954143, Compute: 4608282, Cancel: 728 (0.0015181170060739068%) > > > > > > --------------------------- > > sb-ro-250-density-light-ipv6-ic > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 169 ms > > avg : 27 ms > > 95% : 41 ms > > total : 63750 > > failures: 0 > > > > IP stats: > > Recompute: 46764060, Compute: 4505181, Cancel: 597 (0.0012766214054126181%) > > > > --------------------------- > > main-500-density-heavy-ipv4 > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 3291 ms > > avg : 483 ms > > 95% : 1570 ms > > total : 27500 > > failures: 0 > > > > IP stats: > > Recompute: 845825451, Compute: 166132940, Cancel: 2524 > > (0.00029840672174334936%) > > > > --------------------------- > > sb-ro-500-density-heavy-ipv4 > > --------------------------- > > ovn-installed: > > min : 7 ms > > max : 3223 ms > > avg : 447 ms > > 95% : 1504 ms > > total : 27500 > > failures: 0 > > > > IP stats: > > Recompute: 847419493, Compute: 166570179, Cancel: 2072 > > (0.0002445070023896653%) > > > > --------------------------- > > main-500-density-heavy-ipv6 > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 11875 ms > > avg : 784 ms > > 95% : 2267 ms > > total : 27500 > > failures: 0 > > > > IP stats: > > Recompute: 857064532, Compute: 167714262, Cancel: 2521 > > (0.0002941435453077412%) > > > > > > --------------------------- > > sb-ro-500-density-heavy-ipv6 > > --------------------------- > > ovn-installed: > > min : 7 ms > > max : 7056 ms > > avg : 598 ms > > 95% : 2147 ms > > total : 27500 > > failures: 0 > > > > IP stats: > > Recompute: 865415057, Compute: 169182676, Cancel: 2166 > > (0.0002502845290800158%) > > > > --------------------------- > > main-500-density-light-ipv4 > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 6962 ms > > avg : 306 ms > > 95% : 1389 ms > > total : 127500 > > failures: 0 > > > > IP stats: > > Recompute: 5688174577, Compute: 1186841715, Cancel: 1468 > > (0.00002580792801148937%) > > > > --------------------------- > > sb-ro-500-density-light-ipv4 > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 5508 ms > > avg : 301 ms > > 95% : 1325 ms > > total : 127500 > > failures: 0 > > > > IP stats: > > Recompute: 5676845569, Compute: 1184598070, Cancel: 2659 > > (0.00004683939289312734%) > > > > --------------------------- > > main-500-density-light-ipv6 > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 11675 ms > > avg : 276 ms > > 95% : 797 ms > > total : 127500 > > failures: 0 > > > > IP stats: > > Recompute: 5591661452, Compute: 1162323896, Cancel: 2665 > > (0.00004766025309073021%) > > > > --------------------------- > > sb-ro-500-density-light-ipv6 > > --------------------------- > > ovn-installed: > > min : 6 ms > > max : 6673 ms > > avg : 242 ms > > 95% : 688 ms > > total : 127500 > > failures: 0 > > > > IP stats: > > Recompute: 5779381065, Compute: 1201598403, Cancel: 1728 > > (0.0000298993954640712%) > > > > The numbers caught me a little by surprise, but the important thing > > is that the percentage of cancels is pretty small in general. So having > > 2 recomputes the input nodes for SB write nodes won't cause any > > significant issue. Also on the other side there doesn't seem to be any > > measurable improvement. However, unless you see a different reason > > I would still like to get this patch in for the documentation purpose. > > > > > > Thanks, Ales, for testing this! I agree, even if we don't get a > significant improvement on the ovn-heater benchmarks it's probably still > good to get this in for the additional semantic check it implements. > > From my perspective: > > Acked-by: Dumitru Ceara <[email protected]> > > But I'd love to hear from Mark too before this is merged.
Hi everyone, i think with runtime_data we the additional interesting behaviour that binding_run makes a lot of effort handle cases where southbound is unavailable (or we do not have a transaction for other reasons). It might be the case that it could actually recompute without southbound. I am not sure if this would actually work. But maybe it improves the above statistics. Thanks a lot, Felix > > Thanks, > Dumitru > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
