You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Brian Pane <bp...@pacbell.net> on 2001/11/15 07:05:18 UTC
[PATCH] mmap_setaside (with apr_mmap_dup)
Based on Cliff's suggestion, this patch introduces an apr_mmap_dup()
function that's used to setaside mmap buckets without having to
memcpy the content.
I gave up trying to do full reference counting semantics for
duplicates of apr_mmap_t, because I couldn't find a suitable
pool to own the locks that would be required in a threaded
implementation. Instead, apr_mmap_dup() lets the caller
specify which of the two apr_mmap_t structs--the original one
or the new one--is the owner of the mmap (and is responsible
for destroying it). The patch includes a change to mod_file_cache
to create a non-owner copy of the mmap for the bucket brigade to
use, so that the mmap can't get unmapped by any code other
than mod_file_cache itself
--Brian
Re: [PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Ian Holsman <ia...@cnet.com>.
Thanks Brian.
It's commited.
On Thu, 2001-11-15 at 20:35, Brian Pane wrote:
> Greg Stein wrote:
> [...]
>
> >>I gave up trying to do full reference counting semantics for
> >>duplicates of apr_mmap_t, because I couldn't find a suitable
> >>pool to own the locks that would be required in a threaded
> >>implementation.
> >>
> >
> >Not sure what you mean here. I don't quite understand how threading comes
> >into play. apr_file_t and apr_mmap_t can't be used by multiple threads,
> >AFAIK. And I don't think they should either...
> >
>
> When I tried to implement a traditional reference-counting solution
> (all apr_mmap_t's duplicated from the same original share a reference
> count, and the mmap'ed segment is destroyed only when the refcount
> reaches zero), I ran into threading problems with mod_file_cache.
> It's possible for two threads to be doing a dup of the same
> cached apr_mmap_t at the same time--if they're both delivering the
> same statically cached file, and they both end up having to call
> mmap_setaside. To avoid the race condition here, we'd need a lock
> around the reference count. The lock API requires that we create the
> lock from a pool, but there isn't a really suitable pool from which
> to allocate the lock, given that its lifetime isn't necessarily tied
> to that of a request or connection.
>
> Fortunately, this whole problem went away when I switched from the
> reference-counted model to the transfer-of-ownership model. The latter
> doesn't require any locking.
>
> >>...
> >>--- srclib/apr/mmap/win32/mmap.c 2001/06/27 22:07:24 1.6
> >>+++ srclib/apr/mmap/win32/mmap.c 2001/11/15 05:45:58
> >>@@ -62,10 +62,15 @@
> >>
> >> #if APR_HAS_MMAP
> >>
> >>-static apr_status_t mmap_cleanup(void *themmap)
> >>+APR_DECLARE(apr_status_t) apr_mmap_cleanup(void *themmap)
> >> {
> >>
> >
> >Why is this APR_DECLARE'd now? And why the function rename?
> >
> This is an error left over from an earlier design approach that I tried
> and abandoned. Thanks for catching that. I've attached a new version
> of the patch that fixes it.
>
> --Brian
>
>
>
> ----
>
> Index: srclib/apr/include/apr_mmap.h
> ===================================================================
> RCS file: /home/cvspublic/apr/include/apr_mmap.h,v
> retrieving revision 1.25
> diff -u -r1.25 apr_mmap.h
> --- srclib/apr/include/apr_mmap.h 2001/08/18 15:15:46 1.25
> +++ srclib/apr/include/apr_mmap.h 2001/11/16 04:22:14
> @@ -111,6 +111,8 @@
> void *mm;
> /** The amount of data in the mmap */
> apr_size_t size;
> + /** Whether this object is reponsible for doing the munmap */
> + int is_owner;
> };
>
> #if APR_HAS_MMAP || defined(DOXYGEN)
> @@ -134,6 +136,19 @@
> apr_file_t *file, apr_off_t offset,
> apr_size_t size, apr_int32_t flag,
> apr_pool_t *cntxt);
> +
> +/**
> + * Duplicate the specified MMAP.
> + * @param new_mmap The structure to duplicate into.
> + * @param old_mmap The file to duplicate.
> + * @param p The pool to use for the new file.
> + * @param transfer_ownership Whether responsibility for destroying
> + * the memory-mapped segment is transferred from old_mmap to new_mmap
> + */
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership);
>
> /**
> * Remove a mmap'ed.
> Index: srclib/apr/mmap/unix/mmap.c
> ===================================================================
> RCS file: /home/cvspublic/apr/mmap/unix/mmap.c,v
> retrieving revision 1.36
> diff -u -r1.36 mmap.c
> --- srclib/apr/mmap/unix/mmap.c 2001/08/02 04:26:53 1.36
> +++ srclib/apr/mmap/unix/mmap.c 2001/11/16 04:22:14
> @@ -83,6 +83,11 @@
> {
> apr_mmap_t *mm = themmap;
> int rv;
> +
> + if (!mm->is_owner) {
> + return APR_SUCCESS;
> + }
> +
> #ifdef BEOS
> rv = delete_area(mm->area);
>
> @@ -159,10 +164,37 @@
> (*new)->mm = mm;
> (*new)->size = size;
> (*new)->cntxt = cont;
> + (*new)->is_owner = 1;
>
> /* register the cleanup... */
> apr_pool_cleanup_register((*new)->cntxt, (void*)(*new), mmap_cleanup,
> apr_pool_cleanup_null);
> + return APR_SUCCESS;
> +}
> +
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership)
> +{
> + *new_mmap = (apr_mmap_t *)apr_pmemdup(p, old_mmap, sizeof(apr_mmap_t));
> + (*new_mmap)->cntxt = p;
> +
> + /* The old_mmap can transfer ownership only if the old_mmap itself
> + * is an owner of the mmap'ed segment.
> + */
> + if (old_mmap->is_owner) {
> + if (transfer_ownership) {
> + (*new_mmap)->is_owner = 1;
> + old_mmap->is_owner = 0;
> + apr_pool_cleanup_kill(old_mmap->cntxt, old_mmap, mmap_cleanup);
> + }
> + else {
> + (*new_mmap)->is_owner = 0;
> + }
> + apr_pool_cleanup_register(p, *new_mmap, mmap_cleanup,
> + apr_pool_cleanup_null);
> + }
> return APR_SUCCESS;
> }
>
> Index: srclib/apr/mmap/win32/mmap.c
> ===================================================================
> RCS file: /home/cvspublic/apr/mmap/win32/mmap.c,v
> retrieving revision 1.6
> diff -u -r1.6 mmap.c
> --- srclib/apr/mmap/win32/mmap.c 2001/06/27 22:07:24 1.6
> +++ srclib/apr/mmap/win32/mmap.c 2001/11/16 04:22:14
> @@ -66,6 +66,11 @@
> {
> apr_mmap_t *mm = themmap;
> apr_status_t rv = 0;
> +
> + if (!mm->is_owner) {
> + return APR_SUCCESS;
> + }
> +
> if (mm->mv) {
> if (!UnmapViewOfFile(mm->mv))
> {
> @@ -155,10 +160,37 @@
> (*new)->mm = (char*)((*new)->mv) + offset;
> (*new)->size = size;
> (*new)->cntxt = cont;
> + (*new)->is_owner = 1;
>
> /* register the cleanup... */
> apr_pool_cleanup_register((*new)->cntxt, (void*)(*new), mmap_cleanup,
> apr_pool_cleanup_null);
> + return APR_SUCCESS;
> +}
> +
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership)
> +{
> + *new_mmap = (apr_mmap_t *)apr_pmemdup(p, old_mmap, sizeof(apr_mmap_t));
> + (*new_mmap)->cntxt = p;
> +
> + /* The old_mmap can transfer ownership only if the old_mmap itself
> + * is an owner of the mmap'ed segment.
> + */
> + if (old_mmap->is_owner) {
> + if (transfer_ownership) {
> + (*new_mmap)->is_owner = 1;
> + old_mmap->is_owner = 0;
> + apr_pool_cleanup_kill(old_mmap->cntxt, old_mmap, mmap_cleanup);
> + }
> + else {
> + (*new_mmap)->is_owner = 0;
> + }
> + apr_pool_cleanup_register(p, *new_mmap, mmap_cleanup,
> + apr_pool_cleanup_null);
> + }
> return APR_SUCCESS;
> }
>
> Index: srclib/apr-util/buckets/apr_buckets_mmap.c
> ===================================================================
> RCS file: /home/cvspublic/apr-util/buckets/apr_buckets_mmap.c,v
> retrieving revision 1.43
> diff -u -r1.43 apr_buckets_mmap.c
> --- srclib/apr-util/buckets/apr_buckets_mmap.c 2001/09/22 22:36:07 1.43
> +++ srclib/apr-util/buckets/apr_buckets_mmap.c 2001/11/16 04:22:14
> @@ -121,24 +121,18 @@
> {
> apr_bucket_mmap *m = data->data;
> apr_mmap_t *mm = m->mmap;
> - char *base;
> - void *addr;
> + apr_mmap_t *new_mm;
> apr_status_t ok;
>
> if (apr_pool_is_ancestor(mm->cntxt, p)) {
> return APR_SUCCESS;
> }
>
> - ok = apr_mmap_offset(&addr, m->mmap, data->start);
> + ok = apr_mmap_dup(&new_mm, mm, p, 1);
> if (ok != APR_SUCCESS) {
> return ok;
> }
> -
> - base = apr_palloc(p, data->length);
> - memcpy(base, addr, data->length);
> - data = apr_bucket_pool_make(data, base, data->length, p);
> - mmap_destroy(m);
> -
> + m->mmap = new_mm;
> return APR_SUCCESS;
> }
>
> Index: modules/cache/mod_file_cache.c
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/modules/cache/mod_file_cache.c,v
> retrieving revision 1.62
> diff -u -r1.62 mod_file_cache.c
> --- modules/cache/mod_file_cache.c 2001/08/11 04:04:12 1.62
> +++ modules/cache/mod_file_cache.c 2001/11/16 04:22:15
> @@ -333,8 +333,20 @@
> #if APR_HAS_MMAP
> apr_bucket *b;
> apr_bucket_brigade *bb = apr_brigade_create(r->pool);
> + apr_mmap_t *mm;
>
> - b = apr_bucket_mmap_create(file->mm, 0, (apr_size_t)file->finfo.size);
> + /* Create a copy of the apr_mmap_t that's marked as
> + * a "non-owner." The reason for this is that the
> + * mmap bucket setaside function might later try to
> + * transfer ownership of the mmap from a request
> + * pool to a parent pool. For this particular mmap,
> + * though, we want the cache to retain ownership so
> + * that it never gets munmapped. Thus we give the
> + * bucket code a non-owner copy to work with.
> + */
> + apr_mmap_dup(&mm, file->mm, r->pool, 0);
> +
> + b = apr_bucket_mmap_create(mm, 0, (apr_size_t)file->finfo.size);
> APR_BRIGADE_INSERT_TAIL(bb, b);
> b = apr_bucket_eos_create();
> APR_BRIGADE_INSERT_TAIL(bb, b);
--
Ian Holsman IanH@cnet.com
Performance Measurement & Analysis
CNET Networks - (415) 344-2608
Re: [PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Ian Holsman <ia...@cnet.com>.
Thanks Brian.
It's commited.
On Thu, 2001-11-15 at 20:35, Brian Pane wrote:
> Greg Stein wrote:
> [...]
>
> >>I gave up trying to do full reference counting semantics for
> >>duplicates of apr_mmap_t, because I couldn't find a suitable
> >>pool to own the locks that would be required in a threaded
> >>implementation.
> >>
> >
> >Not sure what you mean here. I don't quite understand how threading comes
> >into play. apr_file_t and apr_mmap_t can't be used by multiple threads,
> >AFAIK. And I don't think they should either...
> >
>
> When I tried to implement a traditional reference-counting solution
> (all apr_mmap_t's duplicated from the same original share a reference
> count, and the mmap'ed segment is destroyed only when the refcount
> reaches zero), I ran into threading problems with mod_file_cache.
> It's possible for two threads to be doing a dup of the same
> cached apr_mmap_t at the same time--if they're both delivering the
> same statically cached file, and they both end up having to call
> mmap_setaside. To avoid the race condition here, we'd need a lock
> around the reference count. The lock API requires that we create the
> lock from a pool, but there isn't a really suitable pool from which
> to allocate the lock, given that its lifetime isn't necessarily tied
> to that of a request or connection.
>
> Fortunately, this whole problem went away when I switched from the
> reference-counted model to the transfer-of-ownership model. The latter
> doesn't require any locking.
>
> >>...
> >>--- srclib/apr/mmap/win32/mmap.c 2001/06/27 22:07:24 1.6
> >>+++ srclib/apr/mmap/win32/mmap.c 2001/11/15 05:45:58
> >>@@ -62,10 +62,15 @@
> >>
> >> #if APR_HAS_MMAP
> >>
> >>-static apr_status_t mmap_cleanup(void *themmap)
> >>+APR_DECLARE(apr_status_t) apr_mmap_cleanup(void *themmap)
> >> {
> >>
> >
> >Why is this APR_DECLARE'd now? And why the function rename?
> >
> This is an error left over from an earlier design approach that I tried
> and abandoned. Thanks for catching that. I've attached a new version
> of the patch that fixes it.
>
> --Brian
>
>
>
> ----
>
> Index: srclib/apr/include/apr_mmap.h
> ===================================================================
> RCS file: /home/cvspublic/apr/include/apr_mmap.h,v
> retrieving revision 1.25
> diff -u -r1.25 apr_mmap.h
> --- srclib/apr/include/apr_mmap.h 2001/08/18 15:15:46 1.25
> +++ srclib/apr/include/apr_mmap.h 2001/11/16 04:22:14
> @@ -111,6 +111,8 @@
> void *mm;
> /** The amount of data in the mmap */
> apr_size_t size;
> + /** Whether this object is reponsible for doing the munmap */
> + int is_owner;
> };
>
> #if APR_HAS_MMAP || defined(DOXYGEN)
> @@ -134,6 +136,19 @@
> apr_file_t *file, apr_off_t offset,
> apr_size_t size, apr_int32_t flag,
> apr_pool_t *cntxt);
> +
> +/**
> + * Duplicate the specified MMAP.
> + * @param new_mmap The structure to duplicate into.
> + * @param old_mmap The file to duplicate.
> + * @param p The pool to use for the new file.
> + * @param transfer_ownership Whether responsibility for destroying
> + * the memory-mapped segment is transferred from old_mmap to new_mmap
> + */
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership);
>
> /**
> * Remove a mmap'ed.
> Index: srclib/apr/mmap/unix/mmap.c
> ===================================================================
> RCS file: /home/cvspublic/apr/mmap/unix/mmap.c,v
> retrieving revision 1.36
> diff -u -r1.36 mmap.c
> --- srclib/apr/mmap/unix/mmap.c 2001/08/02 04:26:53 1.36
> +++ srclib/apr/mmap/unix/mmap.c 2001/11/16 04:22:14
> @@ -83,6 +83,11 @@
> {
> apr_mmap_t *mm = themmap;
> int rv;
> +
> + if (!mm->is_owner) {
> + return APR_SUCCESS;
> + }
> +
> #ifdef BEOS
> rv = delete_area(mm->area);
>
> @@ -159,10 +164,37 @@
> (*new)->mm = mm;
> (*new)->size = size;
> (*new)->cntxt = cont;
> + (*new)->is_owner = 1;
>
> /* register the cleanup... */
> apr_pool_cleanup_register((*new)->cntxt, (void*)(*new), mmap_cleanup,
> apr_pool_cleanup_null);
> + return APR_SUCCESS;
> +}
> +
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership)
> +{
> + *new_mmap = (apr_mmap_t *)apr_pmemdup(p, old_mmap, sizeof(apr_mmap_t));
> + (*new_mmap)->cntxt = p;
> +
> + /* The old_mmap can transfer ownership only if the old_mmap itself
> + * is an owner of the mmap'ed segment.
> + */
> + if (old_mmap->is_owner) {
> + if (transfer_ownership) {
> + (*new_mmap)->is_owner = 1;
> + old_mmap->is_owner = 0;
> + apr_pool_cleanup_kill(old_mmap->cntxt, old_mmap, mmap_cleanup);
> + }
> + else {
> + (*new_mmap)->is_owner = 0;
> + }
> + apr_pool_cleanup_register(p, *new_mmap, mmap_cleanup,
> + apr_pool_cleanup_null);
> + }
> return APR_SUCCESS;
> }
>
> Index: srclib/apr/mmap/win32/mmap.c
> ===================================================================
> RCS file: /home/cvspublic/apr/mmap/win32/mmap.c,v
> retrieving revision 1.6
> diff -u -r1.6 mmap.c
> --- srclib/apr/mmap/win32/mmap.c 2001/06/27 22:07:24 1.6
> +++ srclib/apr/mmap/win32/mmap.c 2001/11/16 04:22:14
> @@ -66,6 +66,11 @@
> {
> apr_mmap_t *mm = themmap;
> apr_status_t rv = 0;
> +
> + if (!mm->is_owner) {
> + return APR_SUCCESS;
> + }
> +
> if (mm->mv) {
> if (!UnmapViewOfFile(mm->mv))
> {
> @@ -155,10 +160,37 @@
> (*new)->mm = (char*)((*new)->mv) + offset;
> (*new)->size = size;
> (*new)->cntxt = cont;
> + (*new)->is_owner = 1;
>
> /* register the cleanup... */
> apr_pool_cleanup_register((*new)->cntxt, (void*)(*new), mmap_cleanup,
> apr_pool_cleanup_null);
> + return APR_SUCCESS;
> +}
> +
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership)
> +{
> + *new_mmap = (apr_mmap_t *)apr_pmemdup(p, old_mmap, sizeof(apr_mmap_t));
> + (*new_mmap)->cntxt = p;
> +
> + /* The old_mmap can transfer ownership only if the old_mmap itself
> + * is an owner of the mmap'ed segment.
> + */
> + if (old_mmap->is_owner) {
> + if (transfer_ownership) {
> + (*new_mmap)->is_owner = 1;
> + old_mmap->is_owner = 0;
> + apr_pool_cleanup_kill(old_mmap->cntxt, old_mmap, mmap_cleanup);
> + }
> + else {
> + (*new_mmap)->is_owner = 0;
> + }
> + apr_pool_cleanup_register(p, *new_mmap, mmap_cleanup,
> + apr_pool_cleanup_null);
> + }
> return APR_SUCCESS;
> }
>
> Index: srclib/apr-util/buckets/apr_buckets_mmap.c
> ===================================================================
> RCS file: /home/cvspublic/apr-util/buckets/apr_buckets_mmap.c,v
> retrieving revision 1.43
> diff -u -r1.43 apr_buckets_mmap.c
> --- srclib/apr-util/buckets/apr_buckets_mmap.c 2001/09/22 22:36:07 1.43
> +++ srclib/apr-util/buckets/apr_buckets_mmap.c 2001/11/16 04:22:14
> @@ -121,24 +121,18 @@
> {
> apr_bucket_mmap *m = data->data;
> apr_mmap_t *mm = m->mmap;
> - char *base;
> - void *addr;
> + apr_mmap_t *new_mm;
> apr_status_t ok;
>
> if (apr_pool_is_ancestor(mm->cntxt, p)) {
> return APR_SUCCESS;
> }
>
> - ok = apr_mmap_offset(&addr, m->mmap, data->start);
> + ok = apr_mmap_dup(&new_mm, mm, p, 1);
> if (ok != APR_SUCCESS) {
> return ok;
> }
> -
> - base = apr_palloc(p, data->length);
> - memcpy(base, addr, data->length);
> - data = apr_bucket_pool_make(data, base, data->length, p);
> - mmap_destroy(m);
> -
> + m->mmap = new_mm;
> return APR_SUCCESS;
> }
>
> Index: modules/cache/mod_file_cache.c
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/modules/cache/mod_file_cache.c,v
> retrieving revision 1.62
> diff -u -r1.62 mod_file_cache.c
> --- modules/cache/mod_file_cache.c 2001/08/11 04:04:12 1.62
> +++ modules/cache/mod_file_cache.c 2001/11/16 04:22:15
> @@ -333,8 +333,20 @@
> #if APR_HAS_MMAP
> apr_bucket *b;
> apr_bucket_brigade *bb = apr_brigade_create(r->pool);
> + apr_mmap_t *mm;
>
> - b = apr_bucket_mmap_create(file->mm, 0, (apr_size_t)file->finfo.size);
> + /* Create a copy of the apr_mmap_t that's marked as
> + * a "non-owner." The reason for this is that the
> + * mmap bucket setaside function might later try to
> + * transfer ownership of the mmap from a request
> + * pool to a parent pool. For this particular mmap,
> + * though, we want the cache to retain ownership so
> + * that it never gets munmapped. Thus we give the
> + * bucket code a non-owner copy to work with.
> + */
> + apr_mmap_dup(&mm, file->mm, r->pool, 0);
> +
> + b = apr_bucket_mmap_create(mm, 0, (apr_size_t)file->finfo.size);
> APR_BRIGADE_INSERT_TAIL(bb, b);
> b = apr_bucket_eos_create();
> APR_BRIGADE_INSERT_TAIL(bb, b);
--
Ian Holsman IanH@cnet.com
Performance Measurement & Analysis
CNET Networks - (415) 344-2608
Re: [PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Cliff Woolley <cl...@yahoo.com>.
On Fri, 16 Nov 2001, Greg Stein wrote:
> > around the reference count. The lock API requires that we create the
> > lock from a pool, but there isn't a really suitable pool from which
> > to allocate the lock, given that its lifetime isn't necessarily tied
> > to that of a request or connection.
>
> The "suitable pool" is that of the apr_mmap_t itself. Note that the file
> cache would be putting that lock into its own long-lived pool.
>
> Not so hard, or I'm missing something :-)
You're missing something. :-)
Let's say you have a pool hierarchy like this:
a
/ \
b c
/
d
You have an apr_mmap_t allocated out of c. You request to dup the mmap
into b. Will b live longer than c? No way to know. Will c live longer
than b? No way to know. So if you've allocated your refcount or the lock
on that refcount out of c, and c goes away before b does, then b has a
reference to no-longer-existing memory.
This is not an odd situation: it's almost exactly what's happening in
mod_file_cache right now. c is the cmd->pool. b is the c->pool. d is
the r->pool. (There might be some pools between a and b for the diagram
to be exactly right, but the hierarchy is essentially correct I believe.)
Granted, in that particular example, c lives much, much longer than b, but
in general that's not at all guaranteed to be the case.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: [PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Cliff Woolley <cl...@yahoo.com>.
On Fri, 16 Nov 2001, Greg Stein wrote:
> > around the reference count. The lock API requires that we create the
> > lock from a pool, but there isn't a really suitable pool from which
> > to allocate the lock, given that its lifetime isn't necessarily tied
> > to that of a request or connection.
>
> The "suitable pool" is that of the apr_mmap_t itself. Note that the file
> cache would be putting that lock into its own long-lived pool.
>
> Not so hard, or I'm missing something :-)
You're missing something. :-)
Let's say you have a pool hierarchy like this:
a
/ \
b c
/
d
You have an apr_mmap_t allocated out of c. You request to dup the mmap
into b. Will b live longer than c? No way to know. Will c live longer
than b? No way to know. So if you've allocated your refcount or the lock
on that refcount out of c, and c goes away before b does, then b has a
reference to no-longer-existing memory.
This is not an odd situation: it's almost exactly what's happening in
mod_file_cache right now. c is the cmd->pool. b is the c->pool. d is
the r->pool. (There might be some pools between a and b for the diagram
to be exactly right, but the hierarchy is essentially correct I believe.)
Granted, in that particular example, c lives much, much longer than b, but
in general that's not at all guaranteed to be the case.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: [PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Nov 15, 2001 at 08:35:47PM -0800, Brian Pane wrote:
>...
> When I tried to implement a traditional reference-counting solution
> (all apr_mmap_t's duplicated from the same original share a reference
> count, and the mmap'ed segment is destroyed only when the refcount
> reaches zero), I ran into threading problems with mod_file_cache.
> It's possible for two threads to be doing a dup of the same
> cached apr_mmap_t at the same time--if they're both delivering the
> same statically cached file, and they both end up having to call
> mmap_setaside. To avoid the race condition here, we'd need a lock
> around the reference count. The lock API requires that we create the
> lock from a pool, but there isn't a really suitable pool from which
> to allocate the lock, given that its lifetime isn't necessarily tied
> to that of a request or connection.
The "suitable pool" is that of the apr_mmap_t itself. Note that the file
cache would be putting that lock into its own long-lived pool.
Not so hard, or I'm missing something :-)
> Fortunately, this whole problem went away when I switched from the
> reference-counted model to the transfer-of-ownership model. The latter
> doesn't require any locking.
Yah.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Nov 15, 2001 at 08:35:47PM -0800, Brian Pane wrote:
>...
> When I tried to implement a traditional reference-counting solution
> (all apr_mmap_t's duplicated from the same original share a reference
> count, and the mmap'ed segment is destroyed only when the refcount
> reaches zero), I ran into threading problems with mod_file_cache.
> It's possible for two threads to be doing a dup of the same
> cached apr_mmap_t at the same time--if they're both delivering the
> same statically cached file, and they both end up having to call
> mmap_setaside. To avoid the race condition here, we'd need a lock
> around the reference count. The lock API requires that we create the
> lock from a pool, but there isn't a really suitable pool from which
> to allocate the lock, given that its lifetime isn't necessarily tied
> to that of a request or connection.
The "suitable pool" is that of the apr_mmap_t itself. Note that the file
cache would be putting that lock into its own long-lived pool.
Not so hard, or I'm missing something :-)
> Fortunately, this whole problem went away when I switched from the
> reference-counted model to the transfer-of-ownership model. The latter
> doesn't require any locking.
Yah.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
[PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Brian Pane <bp...@pacbell.net>.
Greg Stein wrote:
[...]
>>I gave up trying to do full reference counting semantics for
>>duplicates of apr_mmap_t, because I couldn't find a suitable
>>pool to own the locks that would be required in a threaded
>>implementation.
>>
>
>Not sure what you mean here. I don't quite understand how threading comes
>into play. apr_file_t and apr_mmap_t can't be used by multiple threads,
>AFAIK. And I don't think they should either...
>
When I tried to implement a traditional reference-counting solution
(all apr_mmap_t's duplicated from the same original share a reference
count, and the mmap'ed segment is destroyed only when the refcount
reaches zero), I ran into threading problems with mod_file_cache.
It's possible for two threads to be doing a dup of the same
cached apr_mmap_t at the same time--if they're both delivering the
same statically cached file, and they both end up having to call
mmap_setaside. To avoid the race condition here, we'd need a lock
around the reference count. The lock API requires that we create the
lock from a pool, but there isn't a really suitable pool from which
to allocate the lock, given that its lifetime isn't necessarily tied
to that of a request or connection.
Fortunately, this whole problem went away when I switched from the
reference-counted model to the transfer-of-ownership model. The latter
doesn't require any locking.
>>...
>>--- srclib/apr/mmap/win32/mmap.c 2001/06/27 22:07:24 1.6
>>+++ srclib/apr/mmap/win32/mmap.c 2001/11/15 05:45:58
>>@@ -62,10 +62,15 @@
>>
>> #if APR_HAS_MMAP
>>
>>-static apr_status_t mmap_cleanup(void *themmap)
>>+APR_DECLARE(apr_status_t) apr_mmap_cleanup(void *themmap)
>> {
>>
>
>Why is this APR_DECLARE'd now? And why the function rename?
>
This is an error left over from an earlier design approach that I tried
and abandoned. Thanks for catching that. I've attached a new version
of the patch that fixes it.
--Brian
[PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Brian Pane <bp...@pacbell.net>.
Greg Stein wrote:
[...]
>>I gave up trying to do full reference counting semantics for
>>duplicates of apr_mmap_t, because I couldn't find a suitable
>>pool to own the locks that would be required in a threaded
>>implementation.
>>
>
>Not sure what you mean here. I don't quite understand how threading comes
>into play. apr_file_t and apr_mmap_t can't be used by multiple threads,
>AFAIK. And I don't think they should either...
>
When I tried to implement a traditional reference-counting solution
(all apr_mmap_t's duplicated from the same original share a reference
count, and the mmap'ed segment is destroyed only when the refcount
reaches zero), I ran into threading problems with mod_file_cache.
It's possible for two threads to be doing a dup of the same
cached apr_mmap_t at the same time--if they're both delivering the
same statically cached file, and they both end up having to call
mmap_setaside. To avoid the race condition here, we'd need a lock
around the reference count. The lock API requires that we create the
lock from a pool, but there isn't a really suitable pool from which
to allocate the lock, given that its lifetime isn't necessarily tied
to that of a request or connection.
Fortunately, this whole problem went away when I switched from the
reference-counted model to the transfer-of-ownership model. The latter
doesn't require any locking.
>>...
>>--- srclib/apr/mmap/win32/mmap.c 2001/06/27 22:07:24 1.6
>>+++ srclib/apr/mmap/win32/mmap.c 2001/11/15 05:45:58
>>@@ -62,10 +62,15 @@
>>
>> #if APR_HAS_MMAP
>>
>>-static apr_status_t mmap_cleanup(void *themmap)
>>+APR_DECLARE(apr_status_t) apr_mmap_cleanup(void *themmap)
>> {
>>
>
>Why is this APR_DECLARE'd now? And why the function rename?
>
This is an error left over from an earlier design approach that I tried
and abandoned. Thanks for catching that. I've attached a new version
of the patch that fixes it.
--Brian
Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Cliff Woolley <cl...@yahoo.com>.
On Thu, 15 Nov 2001, Greg Stein wrote:
> > I gave up trying to do full reference counting semantics for
> > duplicates of apr_mmap_t, because I couldn't find a suitable
> > pool to own the locks that would be required in a threaded
> > implementation.
>
> Not sure what you mean here. I don't quite understand how threading comes
> into play. apr_file_t and apr_mmap_t can't be used by multiple threads,
> AFAIK. And I don't think they should either...
Sure they can. apr_file_t has an APR_XTHREAD flag explicitly for this
purpose (I think apr_mmap_t ought to have one, too, but it doesn't).
*Buckets* can't be shared across threads, but apr_file_t's and
apr_mmap_t's most certainly can and are. See mod_file_cache.
> > + *new_mmap = (apr_mmap_t *)apr_palloc(p, sizeof(apr_mmap_t));
> > + memcpy(*new_mmap, old_mmap, sizeof(apr_mmap_t));
> > + (*new_mmap)->cntxt = p;
>
> For the two dup() functions, you can use apr_pmemdup() to simplify this.
I think he probably did this as an optimization because the compiler can
recognize a memcpy with a constant length parameter and make it very fast,
whereas the generalized memcpy() routine is a big ol' bugger. <shrug>
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Nov 14, 2001 at 10:05:18PM -0800, Brian Pane wrote:
> Based on Cliff's suggestion, this patch introduces an apr_mmap_dup()
> function that's used to setaside mmap buckets without having to
> memcpy the content.
Cool.
> I gave up trying to do full reference counting semantics for
> duplicates of apr_mmap_t, because I couldn't find a suitable
> pool to own the locks that would be required in a threaded
> implementation.
Not sure what you mean here. I don't quite understand how threading comes
into play. apr_file_t and apr_mmap_t can't be used by multiple threads,
AFAIK. And I don't think they should either...
> Instead, apr_mmap_dup() lets the caller
> specify which of the two apr_mmap_t structs--the original one
> or the new one--is the owner of the mmap (and is responsible
> for destroying it).
Hrm. It would have been really nice to use apr_pool_is_ancestor(), but I
could see a case where you have disjoint pools:
A
/ \
B C
And you're trying to transfer ownership from B to C.
>...
> --- srclib/apr/mmap/win32/mmap.c 2001/06/27 22:07:24 1.6
> +++ srclib/apr/mmap/win32/mmap.c 2001/11/15 05:45:58
> @@ -62,10 +62,15 @@
>
> #if APR_HAS_MMAP
>
> -static apr_status_t mmap_cleanup(void *themmap)
> +APR_DECLARE(apr_status_t) apr_mmap_cleanup(void *themmap)
> {
Why is this APR_DECLARE'd now? And why the function rename?
In fact, doesn't this change the cleanup to a pascal-style function, and
thus not able to be used as a cleanup callback?
>...
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership)
> +{
> + *new_mmap = (apr_mmap_t *)apr_palloc(p, sizeof(apr_mmap_t));
> + memcpy(*new_mmap, old_mmap, sizeof(apr_mmap_t));
> + (*new_mmap)->cntxt = p;
For the two dup() functions, you can use apr_pmemdup() to simplify this.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Nov 14, 2001 at 10:05:18PM -0800, Brian Pane wrote:
> Based on Cliff's suggestion, this patch introduces an apr_mmap_dup()
> function that's used to setaside mmap buckets without having to
> memcpy the content.
Cool.
> I gave up trying to do full reference counting semantics for
> duplicates of apr_mmap_t, because I couldn't find a suitable
> pool to own the locks that would be required in a threaded
> implementation.
Not sure what you mean here. I don't quite understand how threading comes
into play. apr_file_t and apr_mmap_t can't be used by multiple threads,
AFAIK. And I don't think they should either...
> Instead, apr_mmap_dup() lets the caller
> specify which of the two apr_mmap_t structs--the original one
> or the new one--is the owner of the mmap (and is responsible
> for destroying it).
Hrm. It would have been really nice to use apr_pool_is_ancestor(), but I
could see a case where you have disjoint pools:
A
/ \
B C
And you're trying to transfer ownership from B to C.
>...
> --- srclib/apr/mmap/win32/mmap.c 2001/06/27 22:07:24 1.6
> +++ srclib/apr/mmap/win32/mmap.c 2001/11/15 05:45:58
> @@ -62,10 +62,15 @@
>
> #if APR_HAS_MMAP
>
> -static apr_status_t mmap_cleanup(void *themmap)
> +APR_DECLARE(apr_status_t) apr_mmap_cleanup(void *themmap)
> {
Why is this APR_DECLARE'd now? And why the function rename?
In fact, doesn't this change the cleanup to a pascal-style function, and
thus not able to be used as a cleanup callback?
>...
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership)
> +{
> + *new_mmap = (apr_mmap_t *)apr_palloc(p, sizeof(apr_mmap_t));
> + memcpy(*new_mmap, old_mmap, sizeof(apr_mmap_t));
> + (*new_mmap)->cntxt = p;
For the two dup() functions, you can use apr_pmemdup() to simplify this.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Brian Pane <bp...@pacbell.net>.
Cliff Woolley wrote:
[...]
>>The patch includes a change to mod_file_cache to create a non-owner
>>copy of the mmap for the bucket brigade to use, so that the mmap can't
>>get unmapped by any code other than mod_file_cache itself
>>
>
>Why is that necessary? The original mmap from mod_file_cache is owned by
>mod_file_cache and is allocated out of cmd->pool. cmd->pool definitely
>lives longer than r->pool. I guess the problem is that cmd->pool is not
>an ancestor of r->pool, right? It's disjoint (kind of a great-uncle or
>something ;). FEH. Is there some clean way we can allocate the mmap out
>of a better pool than the cmd->pool [like pchild or something]? The less
>work we do at request time, the better. I guess this wouldn't be the end
>of the world, though.
>
Right, it's because the original mmap comes from a disjoint pool that
we need to make the non-owner copy at request time. Allocating the cached
mmap structs from pchild probably would eliminate the need for this. But
it's most likely not worth the effort, because the copy is a relatively
lightweight operation.
--Brian
Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Brian Pane <bp...@pacbell.net>.
Cliff Woolley wrote:
[...]
>>The patch includes a change to mod_file_cache to create a non-owner
>>copy of the mmap for the bucket brigade to use, so that the mmap can't
>>get unmapped by any code other than mod_file_cache itself
>>
>
>Why is that necessary? The original mmap from mod_file_cache is owned by
>mod_file_cache and is allocated out of cmd->pool. cmd->pool definitely
>lives longer than r->pool. I guess the problem is that cmd->pool is not
>an ancestor of r->pool, right? It's disjoint (kind of a great-uncle or
>something ;). FEH. Is there some clean way we can allocate the mmap out
>of a better pool than the cmd->pool [like pchild or something]? The less
>work we do at request time, the better. I guess this wouldn't be the end
>of the world, though.
>
Right, it's because the original mmap comes from a disjoint pool that
we need to make the non-owner copy at request time. Allocating the cached
mmap structs from pchild probably would eliminate the need for this. But
it's most likely not worth the effort, because the copy is a relatively
lightweight operation.
--Brian
Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Cliff Woolley <cl...@yahoo.com>.
On Wed, 14 Nov 2001, Brian Pane wrote:
> I gave up trying to do full reference counting semantics for
> duplicates of apr_mmap_t, because I couldn't find a suitable
> pool to own the locks that would be required in a threaded
> implementation. Instead, apr_mmap_dup() lets the caller
> specify which of the two apr_mmap_t structs--the original one
> or the new one--is the owner of the mmap (and is responsible
> for destroying it).
Yeah, that could work. I'l have to think on it a bit to see if I can come
up with any cases where it won't work, but it seems okay so far in
concept [haven't looked closely at the implementation yet].
> The patch includes a change to mod_file_cache to create a non-owner
> copy of the mmap for the bucket brigade to use, so that the mmap can't
> get unmapped by any code other than mod_file_cache itself
Why is that necessary? The original mmap from mod_file_cache is owned by
mod_file_cache and is allocated out of cmd->pool. cmd->pool definitely
lives longer than r->pool. I guess the problem is that cmd->pool is not
an ancestor of r->pool, right? It's disjoint (kind of a great-uncle or
something ;). FEH. Is there some clean way we can allocate the mmap out
of a better pool than the cmd->pool [like pchild or something]? The less
work we do at request time, the better. I guess this wouldn't be the end
of the world, though.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Cliff Woolley <cl...@yahoo.com>.
On Wed, 14 Nov 2001, Brian Pane wrote:
> I gave up trying to do full reference counting semantics for
> duplicates of apr_mmap_t, because I couldn't find a suitable
> pool to own the locks that would be required in a threaded
> implementation. Instead, apr_mmap_dup() lets the caller
> specify which of the two apr_mmap_t structs--the original one
> or the new one--is the owner of the mmap (and is responsible
> for destroying it).
Yeah, that could work. I'l have to think on it a bit to see if I can come
up with any cases where it won't work, but it seems okay so far in
concept [haven't looked closely at the implementation yet].
> The patch includes a change to mod_file_cache to create a non-owner
> copy of the mmap for the bucket brigade to use, so that the mmap can't
> get unmapped by any code other than mod_file_cache itself
Why is that necessary? The original mmap from mod_file_cache is owned by
mod_file_cache and is allocated out of cmd->pool. cmd->pool definitely
lives longer than r->pool. I guess the problem is that cmd->pool is not
an ancestor of r->pool, right? It's disjoint (kind of a great-uncle or
something ;). FEH. Is there some clean way we can allocate the mmap out
of a better pool than the cmd->pool [like pchild or something]? The less
work we do at request time, the better. I guess this wouldn't be the end
of the world, though.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA