Message ID | 1471436135.14544.96.camel@localhost.localdomain |
---|---|
State | New |
Headers | show |
On 08/17/2016 02:15 PM, Torvald Riegel wrote: > _res_hconf.initialized was not suitable for use in a multi-threaded > environment due to the lack of atomics and memory barriers. Use of it > was also unnecessary because _res_hconf_init did the right thing by > using __libc_once. This patch fixes the glibc-internal uses by just > calling _res_hconf_init unconditionally, and switches to a release MO > atomic store for _res_hconf.initialized to fix the glibc side of the > synchronization problem (which will maintain backward compatibility, but > cannot fix the lack of acquire MO on any glibc-external loads). > > [BZ #20477] > * resolv/res_hconf.c (do_init): Use atomic access. > * resolv/res_hconf.h: Add comments. > * nscd/aicache.c (addhstaiX): Call _res_hconf_init unconditionally. > * nss/getXXbyYY_r.c (REENTRANT_NAME): Likewise. > * sysdeps/posix/getaddrinfo.c (gaih_inet): Likewise. Looks good to me. Please commit. Thanks, Florian
On Aug 17 2016, Torvald Riegel <triegel@redhat.com> wrote: > diff --git a/resolv/res_hconf.h b/resolv/res_hconf.h > index b97734d..a3d23f3 100644 > --- a/resolv/res_hconf.h > +++ b/resolv/res_hconf.h > @@ -25,6 +25,15 @@ > > struct hconf > { > + /* We keep the INITIALIZED member only for backwards compatibility. New > + code should just call _res_hconf_init unconditionally. For this field > + to be used safely, users must ensure that either (1) a call to > + _res_hconf_init happens-before any load from INITIALIZED, or (2) an ^ > + assignment of zero to INITIALIZED happens-before any load from it, and ^ Those hyphens look wrong. Andreas.
On 08/17/2016 04:51 PM, Andreas Schwab wrote: > On Aug 17 2016, Torvald Riegel <triegel@redhat.com> wrote: > >> diff --git a/resolv/res_hconf.h b/resolv/res_hconf.h >> index b97734d..a3d23f3 100644 >> --- a/resolv/res_hconf.h >> +++ b/resolv/res_hconf.h >> @@ -25,6 +25,15 @@ >> >> struct hconf >> { >> + /* We keep the INITIALIZED member only for backwards compatibility. New >> + code should just call _res_hconf_init unconditionally. For this field >> + to be used safely, users must ensure that either (1) a call to >> + _res_hconf_init happens-before any load from INITIALIZED, or (2) an > ^ >> + assignment of zero to INITIALIZED happens-before any load from it, and > ^ > > Those hyphens look wrong. “happens-before” is third-person singular of the verb “happen-before”. It's a technical term widely used while discussing memory models. Here's an example: <http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.4.5> Florian
On Aug 17 2016, Florian Weimer <fweimer@redhat.com> wrote: > On 08/17/2016 04:51 PM, Andreas Schwab wrote: >> On Aug 17 2016, Torvald Riegel <triegel@redhat.com> wrote: >> >>> diff --git a/resolv/res_hconf.h b/resolv/res_hconf.h >>> index b97734d..a3d23f3 100644 >>> --- a/resolv/res_hconf.h >>> +++ b/resolv/res_hconf.h >>> @@ -25,6 +25,15 @@ >>> >>> struct hconf >>> { >>> + /* We keep the INITIALIZED member only for backwards compatibility. New >>> + code should just call _res_hconf_init unconditionally. For this field >>> + to be used safely, users must ensure that either (1) a call to >>> + _res_hconf_init happens-before any load from INITIALIZED, or (2) an >> ^ >>> + assignment of zero to INITIALIZED happens-before any load from it, and >> ^ >> >> Those hyphens look wrong. > > “happens-before” is third-person singular of the verb > “happen-before”. It's a technical term widely used while discussing memory > models. But writing it without hyphen is standard English and doesn't require special knowledge. Andreas.
On 08/18/2016 09:05 AM, Andreas Schwab wrote: > On Aug 17 2016, Florian Weimer <fweimer@redhat.com> wrote: > >> On 08/17/2016 04:51 PM, Andreas Schwab wrote: >>> On Aug 17 2016, Torvald Riegel <triegel@redhat.com> wrote: >>> >>>> diff --git a/resolv/res_hconf.h b/resolv/res_hconf.h >>>> index b97734d..a3d23f3 100644 >>>> --- a/resolv/res_hconf.h >>>> +++ b/resolv/res_hconf.h >>>> @@ -25,6 +25,15 @@ >>>> >>>> struct hconf >>>> { >>>> + /* We keep the INITIALIZED member only for backwards compatibility. New >>>> + code should just call _res_hconf_init unconditionally. For this field >>>> + to be used safely, users must ensure that either (1) a call to >>>> + _res_hconf_init happens-before any load from INITIALIZED, or (2) an >>> ^ >>>> + assignment of zero to INITIALIZED happens-before any load from it, and >>> ^ >>> >>> Those hyphens look wrong. >> >> “happens-before” is third-person singular of the verb >> “happen-before”. It's a technical term widely used while discussing memory >> models. > > But writing it without hyphen is standard English and doesn't require > special knowledge. But making sense of this comment requires special knowledge, and writing the verb as “happens-before” makes it clear that the memory relation is meant, and not some foggy concept of ordering. It's like MAY/SHOULD/RECOMMEND in IETF RFCs in this regard. Florian
On Thu, 2016-08-18 at 10:05 +0200, Florian Weimer wrote: > On 08/18/2016 09:05 AM, Andreas Schwab wrote: > > On Aug 17 2016, Florian Weimer <fweimer@redhat.com> wrote: > > > >> On 08/17/2016 04:51 PM, Andreas Schwab wrote: > >>> On Aug 17 2016, Torvald Riegel <triegel@redhat.com> wrote: > >>> > >>>> diff --git a/resolv/res_hconf.h b/resolv/res_hconf.h > >>>> index b97734d..a3d23f3 100644 > >>>> --- a/resolv/res_hconf.h > >>>> +++ b/resolv/res_hconf.h > >>>> @@ -25,6 +25,15 @@ > >>>> > >>>> struct hconf > >>>> { > >>>> + /* We keep the INITIALIZED member only for backwards compatibility. New > >>>> + code should just call _res_hconf_init unconditionally. For this field > >>>> + to be used safely, users must ensure that either (1) a call to > >>>> + _res_hconf_init happens-before any load from INITIALIZED, or (2) an > >>> ^ > >>>> + assignment of zero to INITIALIZED happens-before any load from it, and > >>> ^ > >>> > >>> Those hyphens look wrong. > >> > >> “happens-before” is third-person singular of the verb > >> “happen-before”. It's a technical term widely used while discussing memory > >> models. > > > > But writing it without hyphen is standard English and doesn't require > > special knowledge. > > But making sense of this comment requires special knowledge, and writing > the verb as “happens-before” makes it clear that the memory relation is > meant, and not some foggy concept of ordering. It's like > MAY/SHOULD/RECOMMEND in IETF RFCs in this regard. Yes. As Florian says, this is meant to refer to the happens-before relation in the C11 memory model, and not some other notion of ordering. The formalization of the C11 memory model uses "happens-before", so it seemed like a good choice. Nonetheless, I'm open to other ways of referring to it, as long as it's precise and clearly refers to the memory model; in the end, the glibc community needs to be happy with how we talk about concurrency. If anyone has suggestions, let me know.
commit 44d3ca295ade8cb11b1473a7009705f6ec99d9bf Author: Torvald Riegel <triegel@redhat.com> Date: Wed Aug 17 13:56:11 2016 +0200 Fix incorrect double-checked locking related to _res_hconf.initialized. _res_hconf.initialized was not suitable for use in a multi-threaded environment due to the lack of atomics and memory barriers. Use of it was also unnecessary because _res_hconf_init did the right thing by using __libc_once. This patch fixes the glibc-internal uses by just calling _res_hconf_init unconditionally, and switches to a release MO atomic store for _res_hconf.initialized to fix the glibc side of the synchronization problem (which will maintain backward compatibility, but cannot fix the lack of acquire MO on any glibc-external loads). [BZ #20477] * resolv/res_hconf.c (do_init): Use atomic access. * resolv/res_hconf.h: Add comments. * nscd/aicache.c (addhstaiX): Call _res_hconf_init unconditionally. * nss/getXXbyYY_r.c (REENTRANT_NAME): Likewise. * sysdeps/posix/getaddrinfo.c (gaih_inet): Likewise. diff --git a/nscd/aicache.c b/nscd/aicache.c index a2e6cf8..32c8f57 100644 --- a/nscd/aicache.c +++ b/nscd/aicache.c @@ -101,8 +101,7 @@ addhstaiX (struct database_dyn *db, int fd, request_header *req, nip = hosts_database; /* Initialize configurations. */ - if (__glibc_unlikely (!_res_hconf.initialized)) - _res_hconf_init (); + _res_hconf_init (); if (__res_maybe_init (&_res, 0) == -1) no_more = 1; diff --git a/nss/getXXbyYY_r.c b/nss/getXXbyYY_r.c index 93af253..18d3ad6 100644 --- a/nss/getXXbyYY_r.c +++ b/nss/getXXbyYY_r.c @@ -274,8 +274,7 @@ INTERNAL (REENTRANT_NAME) (ADD_PARAMS, LOOKUP_TYPE *resbuf, char *buffer, } #endif /* need _res */ #ifdef NEED__RES_HCONF - if (!_res_hconf.initialized) - _res_hconf_init (); + _res_hconf_init (); #endif /* need _res_hconf */ void *tmp_ptr = fct.l; diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c index 5cd1289..093c268 100644 --- a/resolv/res_hconf.c +++ b/resolv/res_hconf.c @@ -348,7 +348,8 @@ do_init (void) arg_trimdomain_list (ENV_TRIM_OVERR, 1, envval); } - _res_hconf.initialized = 1; + /* See comments on the declaration of _res_hconf. */ + atomic_store_release (&_res_hconf.initialized, 1); } diff --git a/resolv/res_hconf.h b/resolv/res_hconf.h index b97734d..a3d23f3 100644 --- a/resolv/res_hconf.h +++ b/resolv/res_hconf.h @@ -25,6 +25,15 @@ struct hconf { + /* We keep the INITIALIZED member only for backwards compatibility. New + code should just call _res_hconf_init unconditionally. For this field + to be used safely, users must ensure that either (1) a call to + _res_hconf_init happens-before any load from INITIALIZED, or (2) an + assignment of zero to INITIALIZED happens-before any load from it, and + these loads use acquire MO if the intent is to skip calling + _res_hconf_init if the load returns a nonzero value. Such acquire MO + loads will then synchronize with the release MO store to INITIALIZED + in do_init in res_hconf.c; see pthread_once for more detail. */ int initialized; int unused1; int unused2[4]; diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c index 574ce08..09fbc83 100644 --- a/sysdeps/posix/getaddrinfo.c +++ b/sysdeps/posix/getaddrinfo.c @@ -816,8 +816,7 @@ gaih_inet (const char *name, const struct gaih_service *service, nip = __nss_hosts_database; /* Initialize configurations. */ - if (__glibc_unlikely (!_res_hconf.initialized)) - _res_hconf_init (); + _res_hconf_init (); if (__res_maybe_init (&_res, 0) == -1) no_more = 1;