From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27327 invoked by alias); 30 Jan 2013 11:28:49 -0000 Received: (qmail 27207 invoked by uid 22791); 30 Jan 2013 11:28:47 -0000 X-SWARE-Spam-Status: No, hits=-7.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_RW X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 30 Jan 2013 11:28:38 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r0UBSYZ8025052 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 30 Jan 2013 06:28:34 -0500 Received: from [10.36.7.186] (vpn1-7-186.ams2.redhat.com [10.36.7.186]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r0UBSWtS009665; Wed, 30 Jan 2013 06:28:33 -0500 Subject: Re: [PATCH] Add faster HTM fastpath for libitm TSX v2 From: Torvald Riegel To: Andi Kleen Cc: gcc-patches@gcc.gnu.org, rth@redhat.com, Andi Kleen In-Reply-To: <20130129191942.GB30577@one.firstfloor.org> References: <1359084657-3498-1-git-send-email-andi@firstfloor.org> <1359465434.3101.12796.camel@triegel.csb> <20130129191942.GB30577@one.firstfloor.org> Content-Type: text/plain; charset="UTF-8" Date: Wed, 30 Jan 2013 11:28:00 -0000 Message-ID: <1359545310.3101.14728.camel@triegel.csb> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2013-01/txt/msg01425.txt.bz2 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