httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Darroch <chr...@pearsoncmg.com>
Subject slotmem API notes
Date Thu, 14 May 2009 04:37:25 GMT
Hi --

   Well, I should be working on mod_fcgid but I wanted to first
follow up on a long-ago promise to review the slotmem API by developing
a "test harness" framework and a slotmem provider that worked with
ZooKeeper as an experiment.

   To that end, mod_shmap now offers a simple "RESTful" interface to the
slotmem API, and mod_slotmem_zk and mod_zk together provide data storage
using ZooKeeper.  See http://people.apache.org/~chrisd/projects/shared_map/
and http://people.apache.org/~chrisd/projects/zookeeper/ for the
gory details.  (Note that the current version of mod_sharedmem in
trunk won't fully work with mod_shmap because of the work-in-progress
on the inuse flag array.  A slightly older version such as r773977
should work OK.)


   I have a few thoughts and concerns about the slotmem API as it
stands.  I'll try to divvy them up into sections below.  First, some
ideas about how to make the existing interface cleaner.

   Remove mod_slotmem (server/slotmem.c) and the associated ap_slotmem_*()
"wrapper" functions.  They do nothing that I can see except add an extra
layer of complexity.  I had no problems using the standard provider
interface to the slotmem providers.  Why would anyone not just do this?

   The wrappers also add confusion because the meanings of common
terms are changed.  For example, providers become "methods" (i.e.,
"storage methods"); the familiar ap_list_provider_names() becomes
ap_slotmem_methods().  But "methods" are what I expect the provider
to provide.  I suggest removing or renaming away any references
to "storage methods" in favour of conventional providers, instances, etc.

   So, define AP_SLOTMEM_PROVIDER_GROUP as "slotmem" and
AP_SLOTMEM_PROVIDER_VERSION as "0".  Remove AP_SLOTMEM_STORAGE.

   Rename mod_sharedmem to mod_slotmem_shm and mod_plainmem to
mod_slotmem_plain.

   Rename ap_slotmem_storage_method to ap_slotmem_provider_t.

   Rename ap_slotmem_t to ap_slotmem_instance_t.

   Remove the "slotmem_" prefix from method names (i.e., the names
of the functions provided by a provider).  If I've got a slotmem
provider, it's just extra typing to have to call
slotmem->slotmem_create() instead of slotmem->create().


   Next, there are several problems I see with the apslotmem_type
enum at the moment.  For one thing, there's only one enumerated value,
SLOTMEM_PERSIST, so there's no way to specify "don't persist".
I hacked around that by passing 1 in mod_shmap.

   Also, if we retain this type, I'd suggest renaming it to a
slightly more conventional looking ap_slotmem_type_t.

   But my personal preference would be to remove it and instead
add an unsigned int flags field to ap_slotmem_provider_t, and
define some flag values, such as AP_SLOTMEM_FLAG_NOTMPSAFE = 0x0001
(read "not MP-safe") and AP_SLOTMEM_FLAG_PERSIST = 0x0002.

   While enumerated types are clean if you've got a long list of
mutually exclusive values, a flags field follows the existing socache
framework, and allows for a number of mutually non-exclusive flags to
be set.  This is actually closer to what I suspect future development
might bring.


   The mention of the socache API brings me to my next set of
points, which proceed from the experience of programming with both
APIs and from a strong feeling that the two have a great deal in
common.  That's where I'm starting from -- specifically a sense
that the two APIs should mirror each other closely.  (Obviously
not everyone might agree with that.)

   To that end, having a AP_SLOTMEM_FLAG_NOTMPSAFE flag would
make the two APIs align on the issue of determining when a caller
needs to take special care in a multi-processing context.

   It would also allow us to remove the global mutex from
mod_sharedmem.  That would simplify maintenance a good deal.

   It would also clear up some things I find odd at the moment;
for instance, why is there locking in the do() method but not
the put() method?  (I'm also not 100% sure the locking in the
create() method will be very valuable.  For example, when
APR_USE_SHMGET is true, apr_shm_remove() will call shmctl()
to mark the segment for destruction once all processes have
detacted from it.  So locking around apr_shm_remove() and
apr_shm_create() doesn't help the case where another process,
which has already attached to the old segment, just keeps on
using that old segment after the lock is released.)

   At any rate, moving responsibility for locking up to the
caller level, as the socache API does, I think makes a lot of
sense.  It means that a caller running in a single-process,
single-threaded context can simply choose not to add the overhead
of a global lock.  Other callers can make their own decisions
about what kind of locking they need.


   Getting to the end now ... next up, a few comments on the
provider methods and their arguments.

   It would be great to pass server_rec* and apr_pool_t* arguments
to all the methods (except maybe num_slots() and slot_size()).
Some providers may need to report error messages, and to do that
they really need a server_rec*.  They may also need to allocate
data from a temporary pool.  The most obvious use case is that
r->pool would be passed to a method like get() or put().

   I'd also love to see create() split into two methods, create()
and init().  The create() method is then responsible for allocating
the ap_slotmem_instance_t structure and filling in the arguments;
it's also assumed that it will do any argument checks that are
required.  To this end, the typical use case is to call it in
the *first* invocation of the post_config hook, and pass the
pconf memory pool.  Note that this implies that create() takes
two apr_pool_t arguments, one for allocating "permanent" structures
which will last the lifetime of the instance, and one for temporary
allocations.  One would normally pass pconf and ptemp from the
post_config hook.

   The init() method then does all the work of actually setting
up the provider's data storage.  The typical use case would
be to call this from the *second* or subsequent invocations of
the post_config hook.  This is how the mod_socache_shmcb provider
functions and like mod_sharedmem is allocates shared memory segments.

   I'd suggest removing both the mem() and attach() methods.

   The mem() method assumes that all providers will return a
slab of memory, and that writes to this memory are equivalent to
a put().  That's not going to be possible unless all providers
deal with memory -- in which case, the only two possible providers
that I can see are the "plain" and shared memory providers we have,
and then the purpose of a whole API and provider framework is lost
on me.  The mod_slotmem_zk provider I wrote, for example, just has
to return APR_ENOTIMPL for mem() because there's really nothing else
it can do.  Similarly any provider which uses a DB file or similar
backing store isn't going to be able to make mem() available.
Better, I think, to drop the method since it's so deeply tied to
an in-memory provider model.

   The attach() method is similarly tied to an in-memory provider
model, and in fact, even more tied to the notion of shared memory,
I think.  From the code in mod_plainmem and mod_sharedmem, moreover,
it looks to me like just calling create() will have virtually the
same effect as attach() -- if a existing segment is available with
the same name, it'll be found and used.  If we really need the
ability to call create() but have it stop short of actually creating
a new segment if no pre-existing one is found, let's add an argument
to create() that specifies this behaviour.  But my guess would be
that there will be few uses for attach() that aren't satisfied by
create() as it stands now.

   The ap_slotmem_callback_fn_t function type should, I think, be
modified not to take a void *mem argument in its signature, but
instead a slot id (i.e., an unsigned int).  Either that or we should
remove the do() method.  This is again because the existing design
assumes that all providers can provide a slab of memory which
the function can operate on.  (I suppose there is a third option
here, in which non-in-memory providers implement do() by allocating
a temporary slab of memory and then, for each in-use slot, performing
a get(), the callback function, and a put().  This obviously would
require an apr_pool_t to be passed to do(), but I'd recommend that
anyway.)

   I'd also suggest putting num_items and item_size elements
into the ap_slotmem_provider_t structure, and then we can remove
the num_items() and item_size() "getter" methods.  That seems
equally clean and slightly simpler to me.

   I'd add a destroy() method that tears down an instance, and
expect callers to use it appropriately.  That means the caller
likely needs to register a cleanup function whenever a successful
create() is performed on a slotmem provider.  This cleanup function
then should call destroy() on the returned slotmem instance.  The
advantage here is that the providers don't need to register anything,
which simplifies their code, and it gives the caller full control
over when the slotmem instance should be destroyed.

   Last but not least on the method front, I'd suggest adding
set() and reset() methods.  The set() method would work roughly like
memset() -- that is, it would accept a byte value and write that
into every byte in the slot.

   The reset() method would do the same as set() with a zero
byte value (i.e., blank the memory) but would also "release"
the slot.  The proposed grab() and return() methods would be
unnecessary.  A provider which wanted to track which slots were
"in use" would simply set the in-use flag when put(), set(), or
get() was called, and reset the flag when reset() was called.


   Finally, just in terms of code simplicity, I noticed that
the mod_plainmem and mod_sharedmem modules both have loops like this:

        if (next) {
            for (;;) {
                ...
                if (!next->next)
                    break;
                next = next->next;
            }
        }

which I think might be reduced to:

        while (next) {
            ...
            next = next->next;
        }

   I'd also be inclined to remove the sharedmem_getstorage(),
sharedmem_initgpool(), and sharedmem_initialize_cleanup() functions,
and just in-line their contents in the places they are used.
All are used just one time each that I can see.

Chris.

-- 
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263  E4DE C8E3 FA36 366A 375B









Mime
View raw message