Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Content-Type: text/plain; charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 10.2 \(3259\))
Subject: Re: svn commit: r1783755 -
 /httpd/httpd/trunk/server/mpm/event/event.c
From: Stefan Eissing <stefan.eissing@greenbytes.de>
In-Reply-To: <BD2D4CCC-4501-44FB-8464-F7627354AE5A@jaguNET.com>
Date: Mon, 20 Feb 2017 15:51:41 +0100
Cc: APR Developer List <dev@apr.apache.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <30337B28-34C6-4C65-A60F-2BB2C6BBA63F@greenbytes.de>
References: <20170220133804.A904B3A00A3@svn01-us-west.apache.org>
 <BD2D4CCC-4501-44FB-8464-F7627354AE5A@jaguNET.com>
To: dev@httpd.apache.org
archived-at: Mon, 20 Feb 2017 14:51:22 -0000


> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <jim@jaguNET.com>:
>=20
> The below got me thinking...
>=20
> Right now, the pool allocator mutex is only used when, well,
> allocator_alloc() is called, which means that sometimes
> apr_palloc(), for example, can be thread-safeish and sometimes
> not, depending on whether or not the active node has enough
> space.
>=20
> For 1.6 and later, it might be nice to actually protect the
> adjustment of the active node, et.al. to, if a mutex is present,
> always be thread-safe... that is, always when we "alloc" memory,
> even when/if we do/don't called allocator_alloc().
>=20
> Thoughts?

So, apr_p*alloc() calls would be thread-safe if a mutex is set in
the underlying allocator? Hmm, at what cost? would be my question.

In regard to thread safety of apr_allocator, there is still something
I do not understand. Observations:

1. When I remove the own allocator in h2_mplx, so it inherits the
   now protected one from the master connection, all runs fine.
2. When I remove the own allocator from the slave connections also,
   in h2_conn, so that slave conns also use the protected allocator
   from the master conn, things break. E.g. strange behaviour up
   to crashes under load.

Which indicates that I have not fully understood the contract of these
things, or there is a hidden assumptions in regard to conn_rec->pool.
hidden to me, at least.

Can someone shed light on this?

>=20
>> On Feb 20, 2017, at 8:38 AM, ylavic@apache.org wrote:
>>=20
>> Author: ylavic
>> Date: Mon Feb 20 13:38:03 2017
>> New Revision: 1783755
>>=20
>> URL: http://svn.apache.org/viewvc?rev=3D1783755&view=3Drev
>> Log:
>> mpm_event: use a mutex for ptrans' allocator to be safe with =
concurrent
>> creation and destruction of its subpools, like with mod_http2.
>>=20
>>=20
>> Modified:
>>   httpd/httpd/trunk/server/mpm/event/event.c
>>=20
>> Modified: httpd/httpd/trunk/server/mpm/event/event.c
>> URL: =
http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?re=
v=3D1783755&r1=3D1783754&r2=3D1783755&view=3Ddiff
>> =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D
>> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
>> +++ httpd/httpd/trunk/server/mpm/event/event.c Mon Feb 20 13:38:03 =
2017
>> @@ -2096,6 +2096,7 @@ static void * APR_THREAD_FUNC listener_t
>>                }
>>                if (!listeners_disabled) {
>>                    void *csd =3D NULL;
>> +                    apr_thread_mutex_t *mutex;
>>                    ap_listen_rec *lr =3D (ap_listen_rec *) pt->baton;
>>                    apr_pool_t *ptrans;         /* Pool for =
per-transaction stuff */
>>                    ap_pop_pool(&ptrans, worker_queue_info);
>> @@ -2105,20 +2106,44 @@ static void * APR_THREAD_FUNC listener_t
>>                        apr_allocator_t *allocator;
>>=20
>>                        apr_allocator_create(&allocator);
>> -                        apr_allocator_max_free_set(allocator,
>> -                                                   ap_max_mem_free);
>> +                        apr_allocator_max_free_set(allocator, =
ap_max_mem_free);
>>                        apr_pool_create_ex(&ptrans, pconf, NULL, =
allocator);
>> -                        apr_allocator_owner_set(allocator, ptrans);
>>                        if (ptrans =3D=3D NULL) {
>>                            ap_log_error(APLOG_MARK, APLOG_CRIT, rc,
>>                                         ap_server_conf, =
APLOGNO(03097)
>>                                         "Failed to create transaction =
pool");
>> +                            apr_allocator_destroy(allocator);
>>                            signal_threads(ST_GRACEFUL);
>>                            return NULL;
>>                        }
>> +                        apr_allocator_owner_set(allocator, ptrans);
>>                    }
>>                    apr_pool_tag(ptrans, "transaction");
>>=20
>> +                    /* We need a mutex in the allocator to =
synchronize ptrans'
>> +                     * children creations/destructions, but this =
mutex ought to
>> +                     * live in ptrans itself to avoid leaks, hence =
it's cleared
>> +                     * in ap_push_pool(). We could recycle some =
pconf's mutexes
>> +                     * like we do for ptrans subpools, but that'd =
need another
>> +                     * synchronization mechanism, whereas creating a =
pthread
>> +                     * mutex (unix here!) is really as simple/fast =
as a static
>> +                     * PTHREAD_MUTEX_INIT assignment, so let's not =
bother and
>> +                     * create the mutex for each ptrans (recycled or =
not).
>> +                     */
>> +                    rc =3D apr_thread_mutex_create(&mutex,
>> +                                                 =
APR_THREAD_MUTEX_DEFAULT,
>> +                                                 ptrans);
>> +                    if (rc !=3D APR_SUCCESS) {
>> +                        ap_log_error(APLOG_MARK, APLOG_CRIT, rc,
>> +                                     ap_server_conf, APLOGNO()
>> +                                     "Failed to create transaction =
pool mutex");
>> +                        ap_push_pool(worker_queue_info, ptrans);
>> +                        signal_threads(ST_GRACEFUL);
>> +                        return NULL;
>> +                    }
>> +                    =
apr_allocator_mutex_set(apr_pool_allocator_get(ptrans),
>> +                                            mutex);
>> +
>>                    get_worker(&have_idle_worker, 1, =
&workers_were_busy);
>>                    rc =3D lr->accept_func(&csd, lr, ptrans);
>>=20
>>=20
>>=20
>=20

Stefan Eissing

<green/>bytes GmbH
Hafenstrasse 16
48155 M=C3=BCnster
www.greenbytes.de