On 27/04/2021 17:53, Han Zhou wrote: > On Tue, Apr 27, 2021 at 8:39 AM Mark Gray <[email protected]> wrote: >> >> Hi Han, >> >> Thanks for fixing this. I reviewed this series but I am not an expert on >> the code. Please have a look at my suggestions but I suggest also >> waiting for an ack from Girish or Krzystof as they will probably test it. >> > > Thanks Mark for the review. > > + Winson who verified for the same environment where Girish was reporting > the issue. (Both of them are now my colleagues :) )
Haha, that is handy. Hopefully you will still be working on OVN! > Winson would you add your Tested-by to this series? > >> Mark >> >> On 22/04/2021 21:14, Han Zhou wrote: >>> Cleanup particially tracked data due to some of the change handler >> s/particially/partially? > > Ack > >>> executions before falling back to recompute. This is done already >>> in the en_runtime_data_run() implementation, but this patch makes >>> it a generic behavior of the I-P engine. >>> >>> Signed-off-by: Han Zhou <[email protected]> >>> --- >>> v1->v2: no change >>> >>> controller/ovn-controller.c | 17 ----------------- >>> lib/inc-proc-eng.c | 5 +++++ >>> 2 files changed, 5 insertions(+), 17 deletions(-) >>> >>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>> index 6f7c9ea61..13c03131c 100644 >>> --- a/controller/ovn-controller.c >>> +++ b/controller/ovn-controller.c >>> @@ -1412,23 +1412,6 @@ en_runtime_data_run(struct engine_node *node, > void *data) >>> struct sset *local_lport_ids = &rt_data->local_lport_ids; >>> struct sset *active_tunnels = &rt_data->active_tunnels; >>> >>> - /* Clear the (stale) tracked data if any. Even though the tracked > data >>> - * gets cleared in the beginning of engine_init_run(), >>> - * any of the runtime data handler might have set some tracked >>> - * data and later another runtime data handler might return false >>> - * resulting in full recompute of runtime engine and rendering the > tracked >>> - * data stale. >>> - * >>> - * It's possible that engine framework can be enhanced to indicate >>> - * the node handlers (in this case > flow_output_runtime_data_handler) >>> - * that its input node had a full recompute. However we would still >>> - * need to clear the tracked data, because we don't want the >>> - * stale tracked data to be accessed outside of the engine, since > the >>> - * tracked data is cleared in the engine_init_run() and not at the >>> - * end of the engine run. >>> - * */ >>> - en_runtime_data_clear_tracked_data(data); >>> - >>> static bool first_run = true; >>> if (first_run) { >>> /* don't cleanup since there is no data yet */ >>> diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c >>> index a6337a1d9..161327404 100644 >>> --- a/lib/inc-proc-eng.c >>> +++ b/lib/inc-proc-eng.c >>> @@ -327,6 +327,11 @@ engine_recompute(struct engine_node *node, bool > forced, bool allowed) >>> } >>> >>> /* Run the node handler which might change state. */ >> Can you move this^ comment down to above the run function as I think it >> is relevant to that code? > > Well, I added it this way because the major step here is to "run()", i.e. > recompute, and I added a minor/sub step which is clearing tracked data > first. Is this reasonable? I can change it if you think the other way is > better. > >>> + /* Clear tracked data before calling run() so that partially > tracked data >>> + * from some of the change handler executions are cleared. */ >>> + if (node->clear_tracked_data) { >>> + node->clear_tracked_data(node->data); >>> + } >>> node->run(node, node->data); >>> node->stats.recompute++; >>> } >>> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
