That's a great explanation. I understand better now. Thank you.
On Fri, Jul 08, 2016 at 08:34:46PM +0000, Rodriguez Betancourt, Esteban wrote:
> Thanks for the comments, I'm going to send soon a new patch with the
> suggestions
> applied.
>
> About how the patch works:
> The first attempt to stop the cascade changes was something like:
>
> @@ -471,7 +512,7 @@ assess_weak_refs(struct ovsdb_txn *txn, struct
> ovsdb_txn_row *txn_row)
> struct ovsdb_table *table;
> struct shash_node *node;
>
> - if (txn_row->old) {
> + if (txn_row->old && !txn_row->new) {
>
> The rationale was: the weak references are linked by UUID, and the UUID never
> changes, unless
> if the current row is being deleted. So, we in fact didn't need to change the
> rows that weak-referenced
> the current row if the row wasn't deleted.
> It didn't works: we discover that the modified rows were lacking incoming
> weak references in
> the dst_refs list, so that gives us the idea of "moving" the references in
> old->dst_refs to new->dst_refs.
>
> In the original code (dst_refs is created from scratch):
>
> old->dst_refs = all the rows that weak referenced old
>
> new->dst_refs = all the rows that weak referenced old and are still weak
> referencing new + rows in the transaction that weak referenced new
>
>
>
> In the patch (dst_refs incrementally built):
> Old->dst_refs = all the rows that weak referenced old
>
> (Ideally, but expansive to calculate:)
> New->dst_refs = old->dst_refs - "weak references removed within this TXN" +
> "weak references created within this TXN"
>
> (What was implemented:)
> New->dst_refs = old->dst_refs - "weak references in old rows in TXN" + "weak
> references in new rows in TXN"
>
> The resulting sets should be equal in both cases.
>
>
> There we do some more optimizations:
> - If we know that the transactions must be successful at some point then,
> instead of cloning dst_refs we could just move the elements between
> the lists.
>
> - At that point we lost the rollback feature, but we aren't going to need
> it anyway (note that we didn't really touch the src_refs part).
>
> - The references in dst_refs must point to new instead than old.
> Previously we iterated over all the weak references in dst_refs
> to change that pointer, but using an UUID is easier, and prevents
> that iteration completely.
>
> Regards,
> Esteban
>
> > -----Original Message-----
> > From: Ben Pfaff [mailto:b...@ovn.org]
> > Sent: sábado, 2 de julio de 2016 14:06
> > To: Rodriguez Betancourt, Esteban <esteb...@hpe.com>
> > Cc: dev@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH] ovsdb: Weak references performance fix
> >
> > On Mon, Jun 27, 2016 at 08:07:02PM +0000, Rodriguez Betancourt, Esteban
> > wrote:
> > > Prevents the cloning of rows with outgoing or incoming weak references
> > > when those rows aren't being modified.
> > >
> > > It improves the OVSDB Server performance when many rows with weak
> > > references are involved in a transaction.
> > >
> > > Signed-off-by: Esteban Rodriguez Betancourt <esteb...@hpe.com>
> >
> > Great idea. Thanks for working on this!
> >
> > The new ovs_list_transplant() function takes two "const" pointers to lists
> > but
> > it modifies both lists. I don't think it makes sense for them to be const.
> >
> > The new ovs_list_transplant() function has a name that doesn't give much of
> > a idea of what it does. I think that really it appends everything in 'src'
> > to 'dst',
> > although those are not great names since both lists are modified. Maybe
> > ovs_list_push_back_all() would be a better name.
> >
> > I think that the new ovs_list_transplant() function can be implemented in
> > terms of ovs_list_splice(), as:
> > ovs_list_splice(dst, &src->next, src); I see a couple of existing
> > uses of
> > ovs_list_splice() to do that, so it's probably best to convert them to use
> > the
> > new function for consistency.
> >
> > Since add_weak_ref() doesn't use its first argument now, please delete the
> > parameter entirely instead of marking it OVS_UNUSED.
> >
> > In add_weak_ref(), instead of using memcpy() to copy a struct uuid, please
> > use an ordinary assignment with =.
> >
> > I don't think that this loop in ovsdb_txn_update_weak_refs() needs to be
> > the _SAFE variant: it appears to me that it iterates on ->src_refs and
> > ->src_node but only modifies ->dst_refs and ->dst_node:
> > > + LIST_FOR_EACH_SAFE (weak, next, src_node, &txn_row->new-
> > >src_refs) {
> > > + /* dst_row MUST exist */
> > > + dst_row = CONST_CAST(struct ovsdb_row *,
> > > + ovsdb_table_get_row(weak->dst_table, &weak->dst));
> > > + ovs_list_insert(&dst_row->dst_refs, &weak->dst_node);
> > > + }
> >
> > It's taking me some thought to convince myself that this new version is as
> > correct as the previous version. Do you have any arguments or explanations
> > to help me out?
> >
> > Thanks,
> >
> > Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
