On Mon, Nov 18, 2019 at 1:27 PM Mark Michelson <mmich...@redhat.com> wrote:
>
> Hi Dumitru,
>
> I reviewed this changeset as a whole rather than trying to review each
> individual patch.
>
> Why does runtime_data_sb_port_binding_handler() in ovn-controller.c not
> update node state? Other change handlers update the node state based on
> whether data has changed. Is there some special reason why this
> particular change handler does not or cannot update node state? I'm
> guessing this is done on purpose, but because there is no comment it
> seems like an omission.
Hi Mark,
I can answer this. This function never changes the data, simply because it
didn't implement it, but if it is sure that the change has no impact, it
will return true, meaning the change has been handled properly (and there
is no impact to this node). Maybe I should have added some comment in the
beginning since this is not very obvious.
With the original implementation, the node->changed is always initialized
to false, so not setting it means keep the default "false".
With the new implementation, the node->state is always set to EN_VALID at
the end of engine_run_node().
Thanks,
Han
>
> On 11/18/19 9:06 AM, Dumitru Ceara wrote:
> > The incremental processing engine might stop a run before the
> > en_runtime_data node is processed. In such cases the ed_runtime_data
> > fields might contain pointers to already deleted SB records. For
> > example, if a port binding corresponding to a patch port is removed from
> > the SB database and the incremental processing engine aborts before the
> > en_runtime_data node is processed then the corresponding local_datapath
> > hashtable entry in ed_runtime_data is stale and will store a pointer to
> > the already freed sbrec_port_binding record.
> >
> > This will cause invalid memory accesses in various places (e.g.,
> > pinctrl_run() -> prepare_ipv6_ras()).
> >
> > This series fixes the issue (patch4) but to make the fix generic and
> > easier to debug it first refactors the incremental processing engine in
> > the following way:
> > - patch1: split engine_run() in smaller functional parts and simplify
the
> > logic of calling engine_run and engine_need_run in the main loop.
> > - patch2: remove recursion from the I-P engine code. Introduce node
states
> > to track validity of node data.
> > - patch3: move ct-zones to its own engine node in order to remove
dependencies
> > on other runtime data.
> >
> > CC: Han Zhou <hz...@ovn.org>
> > Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine
- quiet mode.")
> > Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> >
> > Dumitru Ceara (4):
> > ovn-controller: Refactor I-P engine_run() tracking.
> > ovn-controller: Add per node states to I-P engine.
> > ovn-controller: Add separate I-P engine node for processing
ct-zones.
> > ovn-controller: Fix use of dangling pointers in I-P runtime_data.
> >
> >
> > controller/ovn-controller.c | 418
++++++++++++++++++++++++-------------------
> > lib/inc-proc-eng.c | 314 +++++++++++++++++++++++++-------
> > lib/inc-proc-eng.h | 104 +++++++++--
> > 3 files changed, 564 insertions(+), 272 deletions(-)
> >
> > ---
> > v5:
> > - Rebase.
> > v4:
> > - Address Numan's comments:
> > - Fix engine_need_run().
> > v3:
> > - split the change in series.
> > - Address Han's comments:
> > - fix the data encapsulation issue.
> > - add is_valid method to nodes.
> > - add internal_data/data fields to nodes as it makes it easier to
write
> > the code instead of adding an "engine_get_data()" API.
> > v2: Address Han's comments:
> > - call engine_node_valid() in all the places where node local data is
> > used.
> > - move out "global" data outside the engine nodes. Make a clear
> > separation between data that can be safely used at any time and data
> > that can be used only when the engine run was successful.
> > - add a debug log for iterations when the engine didn't run.
> > - refactor a bit more the incremental engine code.
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
