[ovs-dev] [PATCH] ovsdb: Weak references performance fix

[Ben Pfaff]

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:blp at ovn.org]
> > Sent: sábado, 2 de julio de 2016 14:06
> > To: Rodriguez Betancourt, Esteban <estebarb at hpe.com>
> > Cc: dev at 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 <estebarb at 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.