> 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.
> > // 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.
+ // 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.
-Andi

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