From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id F20E33858CDB for ; Fri, 8 Dec 2023 10:19:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org F20E33858CDB Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org F20E33858CDB Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702030784; cv=none; b=qH7Zp58e4g/7j030CMhLQRb6t+6a6Je9ikuB94XEEvEhg3571f3NG7atscCzr5zqwVatP5kmtxue3JnWRgxCsSYlEoyxCH1d+NbUC99UXPH+kRyN6i8hdNvijo3YuNbYxJ+FQhH/sV1/6cC4e1GL0Z/9cYdrWEyJf4Lpm+CZkPA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702030784; c=relaxed/simple; bh=zme/8eYiwhTa9C2jGx+1dwAvOSemIssmTqspskwN7gM=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=yBkveXYZxXfOiW23pldUNzZVMZOCNgTys194Eot/jKJtpTJlkVFU7aWe6EaNjOJiZJDqiz+ggm/JB+pRgrP7mB/QpmG1VZb8jpRDn+uas2fzbzWsMl5+OMoBl9TXl49+be9Gg2idWnQfVrIYI0RM8Yvk4bn8kA7upqjXIUzBl/U= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1702030782; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references; bh=+O14kuSmCEyb4cu2yDjKn95kmq+sjIsVrUG4YslvQ1k=; b=Kg2FVmGGP4+Aq2AIJYroaq9r0Gmh83tBWTuK2hyrR8cBgyX8TSywPZXRsGvgtzrgw2mEtr In3OQs2tkEffNH90H7UX2+aQZR0N+1VQllNw87PqisgL8oO6H2vEGeLJGQbyu6w6PcX0jy N2s/aIWT1u4x01IcjmM0cDyV2RPUNBc= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-439-6UjZ-fvuOa-lV0bS8QLrFQ-1; Fri, 08 Dec 2023 05:19:37 -0500 X-MC-Unique: 6UjZ-fvuOa-lV0bS8QLrFQ-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id DCCBA848525; Fri, 8 Dec 2023 10:19:36 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.195.157]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 83A4A492BE6; Fri, 8 Dec 2023 10:19:36 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 3B8AJXsk2911825 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Fri, 8 Dec 2023 11:19:33 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 3B8AJW4N2911824; Fri, 8 Dec 2023 11:19:32 +0100 Date: Fri, 8 Dec 2023 11:19:31 +0100 From: Jakub Jelinek To: "Zhu, Lipeng" Cc: tkoenig@netcologne.de, rep.dot.nop@gmail.com, fortran@gcc.gnu.org, gcc-patches@gcc.gnu.org, hongjiu.lu@intel.com, tianyou.li@intel.com, pan.deng@intel.com, wangyang.guo@intel.com, Zhu@imap.gmail.com Subject: Re: [PATCH v6] libgfortran: Replace mutex with rwlock Message-ID: Reply-To: Jakub Jelinek References: <93b9e2d5-4355-136a-a961-da1ae9c1468f@netcologne.de> <20230818031818.2161842-1-lipeng.zhu@intel.com> MIME-Version: 1.0 In-Reply-To: <20230818031818.2161842-1-lipeng.zhu@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Fri, Aug 18, 2023 at 11:18:19AM +0800, Zhu, Lipeng wrote: > From: Lipeng Zhu > > This patch try to introduce the rwlock and split the read/write to > unit_root tree and unit_cache with rwlock instead of the mutex to > increase CPU efficiency. In the get_gfc_unit function, the percentage > to step into the insert_unit function is around 30%, in most instances, > we can get the unit in the phase of reading the unit_cache or unit_root > tree. So split the read/write phase by rwlock would be an approach to > make it more parallel. > > BTW, the IPC metrics can gain around 9x in our test > server with 220 cores. The benchmark we used is > https://github.com/rwesson/NEAT > > libgcc/ChangeLog: > > * gthr-posix.h (__GTHREAD_RWLOCK_INIT): New macro > (__gthrw): New function > (__gthread_rwlock_rdlock): New function > (__gthread_rwlock_tryrdlock): New function > (__gthread_rwlock_wrlock): New function > (__gthread_rwlock_trywrlock): New function > (__gthread_rwlock_unlock): New function > > libgfortran/ChangeLog: > > * io/async.c (DEBUG_LINE): New > * io/async.h (RWLOCK_DEBUG_ADD): New macro > (CHECK_RDLOCK): New macro > (CHECK_WRLOCK): New macro > (TAIL_RWLOCK_DEBUG_QUEUE): New macro > (IN_RWLOCK_DEBUG_QUEUE): New macro > (RDLOCK): New macro > (WRLOCK): New macro > (RWUNLOCK): New macro > (RD_TO_WRLOCK): New macro > (INTERN_RDLOCK): New macro > (INTERN_WRLOCK): New macro > (INTERN_RWUNLOCK): New macro > * io/io.h (internal_proto): Define unit_rwlock > * io/transfer.c (st_read_done_worker): Relace unit_lock with unit_rwlock > (st_write_done_worker): Relace unit_lock with unit_rwlock > * io/unit.c (get_gfc_unit): Relace unit_lock with unit_rwlock > (if): Relace unit_lock with unit_rwlock > (close_unit_1): Relace unit_lock with unit_rwlock > (close_units): Relace unit_lock with unit_rwlock > (newunit_alloc): Relace unit_lock with unit_rwlock > * io/unix.c (flush_all_units): Relace unit_lock with unit_rwlock The changeLog entries are all incorrect. 1) they should be indented by a tab, not 4 spaces, and should end with a dot 2) when several consecutive descriptions have the same text, especially when it is long, it should use Likewise. for the 2nd and following 3) (internal_proto) is certainly not what you've changed, from what I can see in io.h you've done: * io/io.h (struct gfc_unit): Change UNIT_LOCK to UNIT_RWLOCK in a comment. (unit_lock): Remove including associated internal_proto. (unit_rwlock): New declarations including associated internal_proto. (dec_waiting_unlocked): Use WRLOCK and RWUNLOCK on unit_rwlock instead of __gthread_mutex_lock and __gthread_mutex_unlock on unit_lock. (if) is certainly not what you've changed either, always find what function or macro the change was in, or if you remove something, state it, if you add something, state it. 4) all the Replace unit_lock with unit_rwlock. descriptions only partially match reality, you've also changed the operations on those variables. > --- a/libgfortran/io/async.h > +++ b/libgfortran/io/async.h > @@ -207,9 +207,132 @@ > INTERN_LOCK (&debug_queue_lock); \ > MUTEX_DEBUG_ADD (mutex); \ > INTERN_UNLOCK (&debug_queue_lock); \ > - DEBUG_PRINTF ("%s" DEBUG_RED "ACQ:" DEBUG_NORM " %-30s %78p\n", aio_prefix, #mutex, mutex); \ > + DEBUG_PRINTF ("%s" DEBUG_RED "ACQ:" DEBUG_NORM " %-30s %78p\n", aio_prefix, #mutex, \ > + mutex); \ Why are you changing this at all? > +#define RD_TO_WRLOCK(rwlock) \ > + RWUNLOCK (rwlock);\ At least a space before \ (or better tab > +#define RD_TO_WRLOCK(rwlock) \ > + RWUNLOCK (rwlock);\ Likewise. > + WRLOCK (rwlock); > +#endif > +#endif > + > +#ifndef __GTHREAD_RWLOCK_INIT > +#define RDLOCK(rwlock) LOCK (rwlock) > +#define WRLOCK(rwlock) LOCK (rwlock) > +#define RWUNLOCK(rwlock) UNLOCK (rwlock) > +#define RD_TO_WRLOCK(rwlock) {} do {} while (0) instead of {} ? > #endif > > #define INTERN_LOCK(mutex) T_ERROR (__gthread_mutex_lock, mutex); > > #define INTERN_UNLOCK(mutex) T_ERROR (__gthread_mutex_unlock, mutex); > > +#define INTERN_RDLOCK(rwlock) T_ERROR (__gthread_rwlock_rdlock, rwlock); > +#define INTERN_WRLOCK(rwlock) T_ERROR (__gthread_rwlock_wrlock, rwlock); > +#define INTERN_RWUNLOCK(rwlock) T_ERROR (__gthread_rwlock_unlock, rwlock); Admittedly preexisting issue, but I wonder why the ; at the end, all the uses of RDLOCK etc. I've seen were followed by ; > --- a/libgfortran/io/unit.c > +++ b/libgfortran/io/unit.c > @@ -33,34 +33,36 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see > > > /* IO locking rules: > - UNIT_LOCK is a master lock, protecting UNIT_ROOT tree and UNIT_CACHE. > + UNIT_RWLOCK is a master lock, protecting UNIT_ROOT tree and UNIT_CACHE. master rw lock ? > +/* get_gfc_unit_from_root()-- Given an integer, return a pointer > + to the unit structure. Returns NULL if the unit does not exist, > + otherwise returns a locked unit. */ Formatting, to is indented differently from otherwise, both should be 3 spaces, and there should be 2 spaces after ., rather than just one (in both spots). > + > +static inline gfc_unit * get_gfc_unit_from_unit_root (int n) Formatting, there shouldn't be space after *, but also function name should be at the beginning of line, so in this case it should be static inline gfc_unit * get_gfc_unit_from_unit_root (int n) > +{ > + gfc_unit *p; > + int c = 0; > + p = unit_root; This should be indented by 2 spaces rather than 4. > + while (p != NULL) > + { > + c = compare (n, p->unit_number); > + if (c < 0) > + p = p->left; > + if (c > 0) > + p = p->right; > + if (c == 0) > + break; > + } > + return p; And the above return p; line as well. Jakub