Re: [PATCH] Add faster HTM fastpath for libitm TSX v2

[Torvald Riegel <triegel@redhat.com>]

On Tue, 2013-01-29 at 20:19 +0100, Andi Kleen wrote:
> > next time please reply to the points raised in a review if you disagree
> > with them, and don't just ignore them.  That speeds up the review.
> 
> It was all in the previous email on the topic.

This v2 patch did not incorporate all the changes that I requested, nor
did you explicitly object to those you didn't incorporate.  Maybe you
don't care what's in libitm's comments, but I do.

> > >  // See gtm_thread::begin_transaction.
> > > -uint32_t GTM::htm_fastpath = 0;
> > > +uint32_t GTM::htm_fastpath asm("__gtm_htm_fastpath") = 0;
> > > +
> > > +uint32_t *GTM::global_lock asm("__gtm_global_lock");
> > 
> > Rearrange the serial lock's fields (see below).
> 
> To my knowledge C++ classes have no guaranteed layout,
> so that's not safe because there is no guarantee where
> the vtable pointers are. it would be only with plain old structs.

And gtm_rwlock doesn't have any virtual members, right?  So it's like a
plain old struct (ie, it's a POD type).

> +  // pr_tryHTM can be set by an assembler fast path when it already
> tried
> +  // a hardware transaction once. In this case we do one retry less.
> 
> pr_tryHTM is already documented.

And I asked you to simply reference this in a comment.  What's the
problem with that?  I do not want people working on libitm to have to
grep through the code for uses of pr_tryHTM and look for comments that
might be related to it just because you don't want to put a simple
reference into the comment at the declaration of pr_tryHTM.  Can't you
see that adding the reference is just plain useful for everybody else?


Torvald