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 [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id D501138708BD for ; Wed, 13 Jan 2021 12:00:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D501138708BD Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-219-OR2ZfwV0O8CBIrrZ4p4RKw-1; Wed, 13 Jan 2021 07:00:19 -0500 X-MC-Unique: OR2ZfwV0O8CBIrrZ4p4RKw-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D29A08066E2; Wed, 13 Jan 2021 12:00:18 +0000 (UTC) Received: from oldenburg2.str.redhat.com (ovpn-114-17.ams2.redhat.com [10.36.114.17]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9585718AAB; Wed, 13 Jan 2021 12:00:14 +0000 (UTC) From: Florian Weimer To: Chung-Lin Tang Cc: GNU C Library , Carlos O'Donell Subject: Re: [PATCH v4 2/2] BZ #17645, fix slow DSO sorting behavior in dynamic loader -- Algorithm changes References: <85ab188b-64c2-d1fd-33fa-d7d2e6fb2d97@codesourcery.com> Date: Wed, 13 Jan 2021 13:00:12 +0100 In-Reply-To: <85ab188b-64c2-d1fd-33fa-d7d2e6fb2d97@codesourcery.com> (Chung-Lin Tang's message of "Sun, 3 Jan 2021 19:22:48 +0800") Message-ID: <87h7nlcaib.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Jan 2021 12:00:26 -0000 Chung-Lin, I have not reviewed the actual sorting algorithm change yet. I will do so in a follow-up. This change should be quite safe as posted, given that the core behavioral change is hidden behind a tunable setting, and the default is the old behavior. But ideally, I think the new behavior should be the default, so that people can actually experience it. On the other, if the old behavior is preserved, we might still include this change in glibc 2.33. For glibc 2.34, I plan to work on system-wide tunable settings, without environment variables. So it would be possible to flip the default there, while still providing a way to system administrators to restore the old behavior. * Chung-Lin Tang: > diff --git a/elf/dl-close.c b/elf/dl-close.c > index c51becd06b..84d25a29d0 100644 > --- a/elf/dl-close.c > +++ b/elf/dl-close.c > @@ -164,8 +164,6 @@ _dl_close_worker (struct link_map *map, bool force) > =20 > bool any_tls =3D false; > const unsigned int nloaded =3D ns->_ns_nloaded; > - char used[nloaded]; > - char done[nloaded]; This change (moving those arrays into struct link_map) can go in separately if you want to split it out. There seems to be some further cleanup potential regarding the use of IDX_STILL_USED and l_map_done. > @@ -247,9 +242,9 @@ _dl_close_worker (struct link_map *map, bool force) > =09 { > =09=09assert (jmap->l_idx >=3D 0 && jmap->l_idx < nloaded); > =20 > -=09=09if (!used[jmap->l_idx]) > +=09=09if (!jmap->l_map_used) > =09=09 { > -=09=09 used[jmap->l_idx] =3D 1; > +=09=09 jmap->l_map_used =3D 1; > =09=09 if (jmap->l_idx - 1 < done_index) > =09=09 done_index =3D jmap->l_idx - 1; > =09=09 } The above comes under: /* And the same for relocation dependencies. */ if (l->l_reldeps !=3D NULL) for (unsigned int j =3D 0; j < l->l_reldeps->act; ++j) I have a conceptual concern here: I think it is a bug to let relocation dependencies affect finalizer order. In order to cut down the sorting algorithm variations, I would like to suggest not to take relocation dependencies into account for the new sorting mode. This way, there will be only one user-visible transition: from the old sorting to the new sorting. That's going to be much easier to test. > diff --git a/elf/dl-sort-maps.c b/elf/dl-sort-maps.c > index d21770267a..5aa96f4cc1 100644 > --- a/elf/dl-sort-maps.c > +++ b/elf/dl-sort-maps.c > @@ -16,16 +16,24 @@ > License along with the GNU C Library; if not, see > . */ > =20 > +#include > #include > +#include > =20 > +/* Note: this is the older, "original" sorting algorithm, being used as > + default up to 2.32. > =20 > -/* Sort array MAPS according to dependencies of the contained objects. > - Array USED, if non-NULL, is permutated along MAPS. If FOR_FINI this = is > - called for finishing an object. */ > -void > -_dl_sort_maps (struct link_map **maps, unsigned int nmaps, char *used, > -=09 bool for_fini) > + Sort array MAPS according to dependencies of the contained objects. > + If FOR_FINI is true, this is called for finishing an object. */ > +static void > +_dl_sort_maps_original (struct link_map **maps, unsigned int nmaps, > +=09=09=09unsigned int skip, bool for_fini) > { > + /* Allows caller to do the common optimization of skipping the first m= ap, > + usually the main binary. */ > + maps +=3D skip; > + nmaps -=3D skip; As I mentioned in the test case review, I do not think this is purely an optimization. If the main executable has a soname, it participates in the dependency sort like a shared object, so its constructor might not come last. I don't think this is was what programmers expect. > diff --git a/include/link.h b/include/link.h > index 4af16cb596..f2dbcbaf77 100644 > --- a/include/link.h > +++ b/include/link.h > @@ -181,6 +181,10 @@ struct link_map > unsigned int l_init_called:1; /* Nonzero if DT_INIT function called.= */ > unsigned int l_global:1;=09/* Nonzero if object in _dl_global_scope.= */ > unsigned int l_reserved:2;=09/* Reserved for internal use. */ > + unsigned int l_visited:1; /* Used internally for map dependency > +=09=09=09=09 graph traversal. */ > + unsigned int l_map_used:1; /* These two bits are used during traver= sal */ > + unsigned int l_map_done:1; /* of maps in _dl_close_worker(). */ > unsigned int l_phdr_allocated:1; /* Nonzero if the data structure po= inted > =09=09=09=09=09to by `l_phdr' is allocated. */ > unsigned int l_soname_added:1; /* Nonzero if the SONAME is for sure = in Some style nits: No () after function name, =E2=80=9D. */=E2=80=9D at the = end of a comment. Thanks, Florian --=20 Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'N= eill