On Tue, Feb 18, 2020 at 11:56 AM Numan Siddique <[email protected]> wrote: > > On Tue, Feb 18, 2020 at 11:50 PM Han Zhou <[email protected]> wrote: > > > > On Tue, Feb 18, 2020 at 9:27 AM Numan Siddique <[email protected]> wrote: > > > > > On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <[email protected]> wrote: > > > > > > > > On Tue, Feb 18, 2020 at 7:32 AM <[email protected]> wrote: > > > > > > > > > > From: Numan Siddique <[email protected]> > > > > > > > > > > After the patch [1], which added caching of lflow expr, the > > > > lflow_resource_ref > > > > > is not rebuilt properly when lflow_run() is called. If a lflow is > > > already > > > > cached > > > > > in lflow expr cache, then the lflow_resource_ref is not updated. > > > > > But flow_output_run() clears the lflow_resource_ref cache before > > > calling > > > > lflow_run(). > > > > > > > > > > This results in incorrect OF flows present in the switch even if the > > > > > address set/port group references have changed. > > > > > > > > > > This patch fixes this issue by not clearing the lflow_resource_ref > > > cache. > > > > > This cache gets cleaned up during lflow change handler or in > > > lflow_run(). > > > > > > > > Hi Numan, > > > > > > > > This approach looks dangerous to me, since the lflow_resource_ref is not > > > a > > > > cache but part of the engine data. Originally, the life cycle of it > > > follows > > > > the same principle like other data of I-P engine, now if we change the > > > > principle we need to be very careful. > > > > At least one scenario would have problem. E.g. when there is a pending > > > > transaction, we cannot run engine in that iteration, and we will trigger > > > a > > > > complete recompute next time, because the tracking data would be lost in > > > > the next iteration. So it is not possible to call > > > > lflow_resource_destroy_lflow() for deleted rows in that case because > > > there > > > > won't be any deleted flows tracked. It seems the commit 672508f6 > > > > (ovn-controller: Fix memory issues due to lflow expr caching.) would have > > > > similar problem, too, in that case. > > > > > > > > I am not sure if there would be other corner cases that would have issue > > > > with this approach. Probably we can think more of it for making the data > > > > required to build the lflow_resource_ref part of the expr cache. > > > > > > Hi Han, > > > > > > I'll take a closer look at your comments and come back tomorrow. > > > Is it wise to disable or revert lflow expr caching commit ? And then > > > address all these > > > issues properly ? SInce we are close to 20.03 release . > > > > > > > Yes, reverting for 20.03 sounds good to me. We can always back port later > > when it’s mature enough. > > Hi Han, > > I've submitted v2 by taking a different approach as suggested by you. v2 will > now store the addr set and port group name sets as part of lflow expr cache > and add those to the lflow resource ref. > > Kindly take a look. I think this should work now without any issues :). > > Thanks > Numan >
Hi Numan, Thanks for the revision. I acked. For the change-tracking problem I mentioned for commit 672508f6 (ovn-controller: Fix memory issues due to lflow expr caching.), do you plan to address that as well? It would still have memory leak in some corner cases, but the impact may be minor. Thanks, Han _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
