httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jean-frederic clere <jfcl...@gmail.com>
Subject Re: slotmem API notes
Date Thu, 14 May 2009 07:36:45 GMT
Chris Darroch wrote:
> 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?

In mod_cluster I am not using it (yet).

> 
>   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().

My bad... I am bad in giving name to things, but using eclipse that is 
just a click.

> 
> 
>   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.

Looks like a bug :-/

> 
>   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.

Well yes want to use it as a flag (for example to make the grab/return 
etc logic switchable.

> 
>   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.

I am not sure... Because the type of lock depends on the type of memory 
you are using simple mutex in plainmem and file_mutex in sharemem and 
something else if the memory is in a cluster.

> 
>   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?

The do operates on all slots put/get only in one... May be a flag in the 
create. Something like ALLOW_LAZY_LOCKING?

>  (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.)

The idea is to remove segments "forgotten" by dead processes.

> 
>   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 am -1 for passing a server_rec* because I think we should be able to 
use the provider (via a wrapper) in something else that httpd.

> 
>   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.

+1

> 
>   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.

Well if we want to use it for the scoreboard mem() prevents memcpy().

> 
>   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.

Yep a flag to attach to get the create behaviour?

> 
>   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.)

Ok for the apr_pool_t.

> 
>   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 plan to add ap_slotmem_get_used() which belongs to the same kind of 
methods.

> 
>   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.

Well destroy() is complex to do: you have to wait until everyone stops 
using the stuff, no?

> 
>   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.

we have:
grab/return
put/get
you want:
put/set/get/reset.
I do see a big difference, except I prefer the first one.

> 
> 
>   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;
>        }

Oops my bad.

> 
>   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.

-0

Cheers

Jean-Frederic

Mime
View raw message