Ben Pfaff <b...@ovn.org> wrote on 12/04/2016 12:40:00 AM: > On Fri, Apr 01, 2016 at 02:14:27PM +0200, Liran Schour wrote: > > Ben Pfaff <b...@ovn.org> wrote on 30/03/2016 07:56:01 PM: > > > > > On Tue, Mar 22, 2016 at 11:24:14PM +0200, Liran Schour wrote: > > > > Ben Pfaff <b...@ovn.org> wrote on 22/03/2016 07:23:33 PM: > > > > > > > > > On Fri, Mar 04, 2016 at 08:08:59AM +0000, Liran Schour wrote: > > > > > > Hold session's conditions in ovsdb_monitor_session_condition. Pass > > it > > > > > > to ovsdb_monitor for generating "update2" notifications. > > > > > > Add functions that can generate "update2" notification for a > > > > > > "monitor_cond" session. > > > > > > JSON cache is enabled only for session's with true condition only. > > > > > > "monitor_cond" and "monitor_cond_change" are RFC 7047 extensions > > > > > > described by ovsdb-server(1) manpage. > > > > > > > > > > > > Signed-off-by: Liran Schour <lir...@il.ibm.com> > > > > > > > > > > I think that ovsdb_monitor_table_condition_set() has a memory leak > > in > > > > > the error case; it does not free the 'error' returned to it. > > > > > > > > > > > > > Right. Will fix that. > > > > > > > > > I'm a little confused about ovsdb_monitor_session_condition. Why is > > > > > this a separate data structure? That is, why is there a separate > > shash > > > > > of conditions rather than incorporating a condition into > > > > > ovsdb_monitor_table? > > > > > > > > > > > > > A single ovsdb_monitor instance can be used by many jsonrpc sessions > > (as > > > > long as they share the same set of tables and columns). > > > > Ovsdb_monitor_condition_session is holding the condition state per
> > each > > > > jsonrpc session. Each session, in ovsdb_monitor, can have a different > > set > > > > of conditions and it is being represented by > > > > ovsdb_monitor_session_condition with ovsdb_monitor_table_condition per > > > > > > table. > > > > The trivial option was to divide ovsdb_monitor into 2 different > > instances > > > > if we have 2 sessions with different sets of conditions. But, > > condition is > > > > changing during the lifetime of monitor session. And by holding > > sessions > > > > with different conditions under the same ovsdb_monitor we can reduce > > the > > > > computation needed to insert row changes to ovsdb_monitor. (insert > > once > > > > instead of twice) > > > > > > Those downsides of having a monitor per (monitored columns, condition) > > > seem pretty minor. It also seems to me like the "obvious" way to > > > implement this. It seems like, for example, the problem of caching with > > > conditions is much easier if the data structures are organized this > > > way. Did you actually try implementing it that way and determine that > > > there was some insuperable difficulty? > > > > No, I did not try implementing it in the 'trivial' way. The reason is > > twofold: first, to gain the ability to share the computed updates between > > sessions, also as per-session conditions change; second, to gain the > > ability to 'fall back' to using a Json cache for condition-less tables. (A > > typical monitor session includes conditioned tables and condition-less > > tables). > > > > My implementation choice achieves the above and has the complexity of > > processing a DB update in O(#conditioned columns). The 'trivial' > > alternative would not achieve the above in the general case and would have > > the complexity of processing a DB update in O(#jsonrpc sessions with > > matched conditions), in the best case. > > > > As I see it, the above stated benefits of the more complex design worth > > the complexity increase in the code, given also that the memory complexity > > is similar. > > As I understand it, the implementation in this patch series cannot share > a cache of changes across sessions that include a condition, even if the > condition is the same. Is that correct? Correct, json-cache is not shared across sessions in general, only across sessions with empty or true conditions on all tables, or per table with empty or true condition. What shared across sessions is the changes list computed for updates matching shared condition clauses, as follows: update arrives --> update is added to all the matched condition clauses (clauses are shared between the sessions) so that changes list is computed per clause --> per session update messages are created using the already computed changes lists. Changes list for all sessions sharing a common ovsdb_monitor is computed in O(#conditioned columns), once per update. To introduce the feature incrementally, the patch series starts with evaluating conditions and computing update messages by going over the raw changes list; then allows re-using the json-cache for empty or true conditioned tables (patch #5); and only then introduces the clause-based changes tracking described above (patch #13-17). This patch series has been under test for several weeks now (both in OpenStack cloud and OVN sandbox); several bugs were revealed and fixed; performance evaluation is in progress. Initial results show significant CPU load reduction on ovn-controllers. There are additional insights. I am ready to share details and discuss. I will be at the OpenStack summit, can we arrange a discussion session on this topic there? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
