You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Yann Ylavic <yl...@gmail.com> on 2014/10/01 16:24:42 UTC
Re: apr_os_proc_mutex_get/set[_ex] for supported mechanisms
Here are v2 of the previous patch(es) with the following addons :
1. Really address the showstopper from
http://svn.apache.org/viewvc?view=revision&revision=1587066
The previous patch only addressed the impossibility to specify the
mechanism when set()ting an APR proc mutex from a OS proc mutex
(disambiguation on unixes with multiple mechanisms using the fd
available).
This time the patch addresses the get() part of the issue, by introducing :
APR_DECLARE(apr_status_t) apr_os_proc_mutex_get_ex(apr_os_proc_mutex_t
*ospmutex,
apr_proc_mutex_t *pmutex,
apr_lockmech_e *mech);
so that the caller can also get the mechanism used by the mutex.
Again, like with apr_os_proc_mutex_set_ex(), I choose to not add the
apr_lockmech_e to the apr_os_proc_mutex_t but instead use a new _ex()
function. This allows to be compatible with the existing codes which
would not have been aware of the new apr_os_proc_mutex_t field, with
an uninitialized value for the mech that we could not have relied on.
The new function is implemented on all platforms, and APR_LOCK_DEFAULT
is used when only one mech is available.
For unixes, the mech has been added to the
apr_proc_mutex_unix_lock_methods_t struct (where the mech's name is
already defined), so that it can be accessed immediatly with
mutex->meth->mech (like mutex->meth->name).
2. Since apr_[global/proc]_mutex_name() are already available to get
the name of the mech, I also added apr_[global/proc]_mutex_mech().
Hence the caller can get the enum easily from the mutex (eg. without
strcmp(apr_proc_mutex_name(mutex), "sysvsem") for every possible name
like in httpd/os/unix/unixd.c).
3. Configure PROC_PTHREAD mutexes to be "global" (ie. they are also
thread locking) in configure.in
This matters for apr_global_mutex_t where APR_PROC_MUTEX_IS_GLOBAL is
checked to determine whether an associated thread mutex is required to
be thread-safe. When the (default) proc-mutex is "global", all the
apr_global_mutex_* functions and types are #defined to the
corresponding proc-mutex ones, otherwise apr_global_mutex_t is a
struct containing both a proc and thread mutex, and the 2 locks are
successively acquired/released upon apr_global_mutex_lock/unlock().
This is not needed for proc-pthread mutexes, but since this is now the
first (default) global proc-mutex mechanism on unixes, the case was
not handled in include/arch/unix/apr_arch_global_mutex.h nor in
locks/unix/global_mutex.c, and I had to use some #if
!APR_PROC_MUTEX_IS_GLOBAL there to disable the global-mutex specific
code.
I also added a missing #define apr_global_mutex_perms_set
apr_proc_mutex_perms_set when the proc-mutex is global.
All the rest remain as decribed below (previous message).
On Tue, Sep 30, 2014 at 10:35 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> the current apr_os_proc_mutex_get/set() implementation does
> not do the right thing nor allow very much about posix semaphores wrt
> these functions.
>
> The attached patch tries to address this by adding the native type
> field (sem_t*) in apr_os_proc_mutex_t so that it can be got or put by
> the caller explicitely (previously it was stored in a int/fd in the OS
> type whereas the sem_t* was used only by the apr_proc_mutex_*()
> functions).
> This is abviously an ABI change since apr_os_proc_mutex_t is defined
> in include/apr_portable.h, I don't think we can avoid this.
>
> Rather than also adding this new field to the unixes specific
> apr_proc_mutex_t struct, I also modified the latter to replace all the
> duplicated fields by a apr_os_proc_mutex_t member (called os). Since
> apr_proc_mutex_t is private (include/arch/unix/apr_arch_proc_mutex.h
> for unixes), this is not an API/ABI change.
> This change implies the use of mutex->os.[native] instead of
> mutex->[native] in the different mechanisms implementation, but it
> also greatly simplifies apr_os_proc_mutex_get() to something as simple
> as *osmutex = mutex->os.
>
> To help disambiguate the mechanism to be used when creating an
> apr_proc_mutex_t from a native type, I also added the new
> apr_os_proc_mutex_put_ex() function, which takes the lockmech as
> parameter, and allows then for example to create a APR_LOCK_FLOCK
> mutex from a fd when the default lockmech is SYSV (both using a fd as
> native type).
> This, I think, addresses the showstopper from
> http://svn.apache.org/viewvc?view=revision&revision=1587066.
> To initialize the apr_proc_mutex_t from the native type, I slightly
> modified the existing/private proc_mutex_choose_method() function to
> eventually take an os type as argument and do the right thing when not
> NULL.
> This function is of course also implemented in other platforms (where
> APR_LOCK_DEFAULT is to be used as lockmech since there is the only
> choice on non-unixes), so this is a another API addon (still in
> include/apr_portable.h).
>
> On OS2, I have made apr_os_proc_mutex_get() return APR_SUCCESS since
> the existing code looks correct (but the returned status), and
> apr_os_proc_mutex_set() return APR_ENOPOOL when a NULL pool is given,
> for consistency with other platforms.
>
> For consistency on all platforms still, in the unix code this time, I
> changed the behaviour of the proc_mutex_*_cleanup() functions so that
> they cleanup the native mutex even if the APR mutex was created by
> apr_os_proc_mutex_set(), taking care of not closing the fd twice (for
> proc mutexes using a fd, since the mutex is also mapped to an
> apr_file_t).
> This is however inconsistant with other APR types that can be created
> from a native type (eg. apr_os_file_t using apr_os_file_put(), quite
> logically since the object is not really owned by APR), so maybe I
> should have done the opposite on non-unix platforms, let me know...
>
> Finally, netware, where the native mutex type is NXMutex_t (and not a
> *pointer to* NXMutex_t).
> AFAICT (from netware sources in nks/synch.h), the NXMutex_t struct is
> defined as :
>
> typedef struct
> {
> uint64_t reserved1;
> void *reserved2[10];
> } NXMutex_t;
>
> This is hence impossible to get/set it via the
> apr_os_proc_mutex_get/set() interface (would require a copy of the
> struct, which is obviously not suitable).
> The proposed change is to typedef NXMutex_t *apr_os_proc_mutex_t
> instead (with the *pointer to*), and do the right thing in the get/set
> functions.
> That's another API/ABI change though, but I doubt anyone has already
> used netware native mutex type successfully from/to
> apr_proc_mutex_t...
> Since I can't test it, this change is also attached here but in a
> distinct patch, and it will probably require a review from someone who
> knows netware and can test it (I don't, for both).
Re: apr_os_proc_mutex_get/set[_ex] for supported mechanisms
Posted by Yann Ylavic <yl...@gmail.com>.
Actually APR_PROC_MUTEX_IS_GLOBAL is not meant to disable thread mutex
on systems with multiple mechanism available.
We can't change configure.in this way since apr_global_mutex_create()
takes the mech as parameter (which may or not be global)...
However Windows mutexes are global (and there is one single choice for
the mech), so in apr.h.in we can #define APR_PROC_MUTEX_IS_GLOBAL 1
for this platform.
New patch (v4) attached, hence without point 2 below (the others
remain), but with the change in apr.h.in described above.
On Tue, Mar 24, 2015 at 10:41 PM, Yann Ylavic <yl...@gmail.com> wrote:
> Here is a new patch regarding this, with the following changes wrt to
> the previous one:
>
> 1. applies to latest trunk,
> 2. declares both POSIXSEM and PROC_PTHREAD as global in configure.in
> (ie. they don't need a thread mutex when used as global mutex),
> 3. takes into account the new APR_LOCK_DEFAULT_TIMED from r1667900,
> and return it in
> apr_{proc,global}_mutex_mech()/apr_os_proc_mutex_get_ex() for
> BeOS/OS2/Win32 (which have native timed mutexes),
> 4. gets rid of (unused) pthread_mutex_t *intraproc in apr_os_proc_mutex_t,
> 5. includes Netware changes (no more in a separate patch).
>
> Still I could only test this on linux (where all unixes mechanisms are
> available though), so review and (at least compile-)testing on other
> patforms much appreciated.
>
> Thanks,
> Yann.
>
>
> On Wed, Oct 1, 2014 at 4:24 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> Here are v2 of the previous patch(es) with the following addons :
>>
>> 1. Really address the showstopper from
>> http://svn.apache.org/viewvc?view=revision&revision=1587066
>>
>> The previous patch only addressed the impossibility to specify the
>> mechanism when set()ting an APR proc mutex from a OS proc mutex
>> (disambiguation on unixes with multiple mechanisms using the fd
>> available).
>> This time the patch addresses the get() part of the issue, by introducing :
>>
>> APR_DECLARE(apr_status_t) apr_os_proc_mutex_get_ex(apr_os_proc_mutex_t
>> *ospmutex,
>> apr_proc_mutex_t *pmutex,
>> apr_lockmech_e *mech);
>>
>> so that the caller can also get the mechanism used by the mutex.
>>
>> Again, like with apr_os_proc_mutex_set_ex(), I choose to not add the
>> apr_lockmech_e to the apr_os_proc_mutex_t but instead use a new _ex()
>> function. This allows to be compatible with the existing codes which
>> would not have been aware of the new apr_os_proc_mutex_t field, with
>> an uninitialized value for the mech that we could not have relied on.
>>
>> The new function is implemented on all platforms, and APR_LOCK_DEFAULT
>> is used when only one mech is available.
>>
>> For unixes, the mech has been added to the
>> apr_proc_mutex_unix_lock_methods_t struct (where the mech's name is
>> already defined), so that it can be accessed immediatly with
>> mutex->meth->mech (like mutex->meth->name).
>>
>>
>> 2. Since apr_[global/proc]_mutex_name() are already available to get
>> the name of the mech, I also added apr_[global/proc]_mutex_mech().
>>
>> Hence the caller can get the enum easily from the mutex (eg. without
>> strcmp(apr_proc_mutex_name(mutex), "sysvsem") for every possible name
>> like in httpd/os/unix/unixd.c).
>>
>>
>> 3. Configure PROC_PTHREAD mutexes to be "global" (ie. they are also
>> thread locking) in configure.in
>>
>> This matters for apr_global_mutex_t where APR_PROC_MUTEX_IS_GLOBAL is
>> checked to determine whether an associated thread mutex is required to
>> be thread-safe. When the (default) proc-mutex is "global", all the
>> apr_global_mutex_* functions and types are #defined to the
>> corresponding proc-mutex ones, otherwise apr_global_mutex_t is a
>> struct containing both a proc and thread mutex, and the 2 locks are
>> successively acquired/released upon apr_global_mutex_lock/unlock().
>>
>> This is not needed for proc-pthread mutexes, but since this is now the
>> first (default) global proc-mutex mechanism on unixes, the case was
>> not handled in include/arch/unix/apr_arch_global_mutex.h nor in
>> locks/unix/global_mutex.c, and I had to use some #if
>> !APR_PROC_MUTEX_IS_GLOBAL there to disable the global-mutex specific
>> code.
>>
>> I also added a missing #define apr_global_mutex_perms_set
>> apr_proc_mutex_perms_set when the proc-mutex is global.
>>
>>
>> All the rest remain as decribed below (previous message).
>>
>> On Tue, Sep 30, 2014 at 10:35 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>>
>>> the current apr_os_proc_mutex_get/set() implementation does
>>> not do the right thing nor allow very much about posix semaphores wrt
>>> these functions.
>>>
>>> The attached patch tries to address this by adding the native type
>>> field (sem_t*) in apr_os_proc_mutex_t so that it can be got or put by
>>> the caller explicitely (previously it was stored in a int/fd in the OS
>>> type whereas the sem_t* was used only by the apr_proc_mutex_*()
>>> functions).
>>> This is abviously an ABI change since apr_os_proc_mutex_t is defined
>>> in include/apr_portable.h, I don't think we can avoid this.
>>>
>>> Rather than also adding this new field to the unixes specific
>>> apr_proc_mutex_t struct, I also modified the latter to replace all the
>>> duplicated fields by a apr_os_proc_mutex_t member (called os). Since
>>> apr_proc_mutex_t is private (include/arch/unix/apr_arch_proc_mutex.h
>>> for unixes), this is not an API/ABI change.
>>> This change implies the use of mutex->os.[native] instead of
>>> mutex->[native] in the different mechanisms implementation, but it
>>> also greatly simplifies apr_os_proc_mutex_get() to something as simple
>>> as *osmutex = mutex->os.
>>>
>>> To help disambiguate the mechanism to be used when creating an
>>> apr_proc_mutex_t from a native type, I also added the new
>>> apr_os_proc_mutex_put_ex() function, which takes the lockmech as
>>> parameter, and allows then for example to create a APR_LOCK_FLOCK
>>> mutex from a fd when the default lockmech is SYSV (both using a fd as
>>> native type).
>>> This, I think, addresses the showstopper from
>>> http://svn.apache.org/viewvc?view=revision&revision=1587066.
>>> To initialize the apr_proc_mutex_t from the native type, I slightly
>>> modified the existing/private proc_mutex_choose_method() function to
>>> eventually take an os type as argument and do the right thing when not
>>> NULL.
>>> This function is of course also implemented in other platforms (where
>>> APR_LOCK_DEFAULT is to be used as lockmech since there is the only
>>> choice on non-unixes), so this is a another API addon (still in
>>> include/apr_portable.h).
>>>
>>> On OS2, I have made apr_os_proc_mutex_get() return APR_SUCCESS since
>>> the existing code looks correct (but the returned status), and
>>> apr_os_proc_mutex_set() return APR_ENOPOOL when a NULL pool is given,
>>> for consistency with other platforms.
>>>
>>> For consistency on all platforms still, in the unix code this time, I
>>> changed the behaviour of the proc_mutex_*_cleanup() functions so that
>>> they cleanup the native mutex even if the APR mutex was created by
>>> apr_os_proc_mutex_set(), taking care of not closing the fd twice (for
>>> proc mutexes using a fd, since the mutex is also mapped to an
>>> apr_file_t).
>>> This is however inconsistant with other APR types that can be created
>>> from a native type (eg. apr_os_file_t using apr_os_file_put(), quite
>>> logically since the object is not really owned by APR), so maybe I
>>> should have done the opposite on non-unix platforms, let me know...
>>>
>>> Finally, netware, where the native mutex type is NXMutex_t (and not a
>>> *pointer to* NXMutex_t).
>>> AFAICT (from netware sources in nks/synch.h), the NXMutex_t struct is
>>> defined as :
>>>
>>> typedef struct
>>> {
>>> uint64_t reserved1;
>>> void *reserved2[10];
>>> } NXMutex_t;
>>>
>>> This is hence impossible to get/set it via the
>>> apr_os_proc_mutex_get/set() interface (would require a copy of the
>>> struct, which is obviously not suitable).
>>> The proposed change is to typedef NXMutex_t *apr_os_proc_mutex_t
>>> instead (with the *pointer to*), and do the right thing in the get/set
>>> functions.
>>> That's another API/ABI change though, but I doubt anyone has already
>>> used netware native mutex type successfully from/to
>>> apr_proc_mutex_t...
>>> Since I can't test it, this change is also attached here but in a
>>> distinct patch, and it will probably require a review from someone who
>>> knows netware and can test it (I don't, for both).
Re: apr_os_proc_mutex_get/set[_ex] for supported mechanisms
Posted by Yann Ylavic <yl...@gmail.com>.
Here is a new patch regarding this, with the following changes wrt to
the previous one:
1. applies to latest trunk,
2. declares both POSIXSEM and PROC_PTHREAD as global in configure.in
(ie. they don't need a thread mutex when used as global mutex),
3. takes into account the new APR_LOCK_DEFAULT_TIMED from r1667900,
and return it in
apr_{proc,global}_mutex_mech()/apr_os_proc_mutex_get_ex() for
BeOS/OS2/Win32 (which have native timed mutexes),
4. gets rid of (unused) pthread_mutex_t *intraproc in apr_os_proc_mutex_t,
5. includes Netware changes (no more in a separate patch).
Still I could only test this on linux (where all unixes mechanisms are
available though), so review and (at least compile-)testing on other
patforms much appreciated.
Thanks,
Yann.
On Wed, Oct 1, 2014 at 4:24 PM, Yann Ylavic <yl...@gmail.com> wrote:
> Here are v2 of the previous patch(es) with the following addons :
>
> 1. Really address the showstopper from
> http://svn.apache.org/viewvc?view=revision&revision=1587066
>
> The previous patch only addressed the impossibility to specify the
> mechanism when set()ting an APR proc mutex from a OS proc mutex
> (disambiguation on unixes with multiple mechanisms using the fd
> available).
> This time the patch addresses the get() part of the issue, by introducing :
>
> APR_DECLARE(apr_status_t) apr_os_proc_mutex_get_ex(apr_os_proc_mutex_t
> *ospmutex,
> apr_proc_mutex_t *pmutex,
> apr_lockmech_e *mech);
>
> so that the caller can also get the mechanism used by the mutex.
>
> Again, like with apr_os_proc_mutex_set_ex(), I choose to not add the
> apr_lockmech_e to the apr_os_proc_mutex_t but instead use a new _ex()
> function. This allows to be compatible with the existing codes which
> would not have been aware of the new apr_os_proc_mutex_t field, with
> an uninitialized value for the mech that we could not have relied on.
>
> The new function is implemented on all platforms, and APR_LOCK_DEFAULT
> is used when only one mech is available.
>
> For unixes, the mech has been added to the
> apr_proc_mutex_unix_lock_methods_t struct (where the mech's name is
> already defined), so that it can be accessed immediatly with
> mutex->meth->mech (like mutex->meth->name).
>
>
> 2. Since apr_[global/proc]_mutex_name() are already available to get
> the name of the mech, I also added apr_[global/proc]_mutex_mech().
>
> Hence the caller can get the enum easily from the mutex (eg. without
> strcmp(apr_proc_mutex_name(mutex), "sysvsem") for every possible name
> like in httpd/os/unix/unixd.c).
>
>
> 3. Configure PROC_PTHREAD mutexes to be "global" (ie. they are also
> thread locking) in configure.in
>
> This matters for apr_global_mutex_t where APR_PROC_MUTEX_IS_GLOBAL is
> checked to determine whether an associated thread mutex is required to
> be thread-safe. When the (default) proc-mutex is "global", all the
> apr_global_mutex_* functions and types are #defined to the
> corresponding proc-mutex ones, otherwise apr_global_mutex_t is a
> struct containing both a proc and thread mutex, and the 2 locks are
> successively acquired/released upon apr_global_mutex_lock/unlock().
>
> This is not needed for proc-pthread mutexes, but since this is now the
> first (default) global proc-mutex mechanism on unixes, the case was
> not handled in include/arch/unix/apr_arch_global_mutex.h nor in
> locks/unix/global_mutex.c, and I had to use some #if
> !APR_PROC_MUTEX_IS_GLOBAL there to disable the global-mutex specific
> code.
>
> I also added a missing #define apr_global_mutex_perms_set
> apr_proc_mutex_perms_set when the proc-mutex is global.
>
>
> All the rest remain as decribed below (previous message).
>
> On Tue, Sep 30, 2014 at 10:35 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> the current apr_os_proc_mutex_get/set() implementation does
>> not do the right thing nor allow very much about posix semaphores wrt
>> these functions.
>>
>> The attached patch tries to address this by adding the native type
>> field (sem_t*) in apr_os_proc_mutex_t so that it can be got or put by
>> the caller explicitely (previously it was stored in a int/fd in the OS
>> type whereas the sem_t* was used only by the apr_proc_mutex_*()
>> functions).
>> This is abviously an ABI change since apr_os_proc_mutex_t is defined
>> in include/apr_portable.h, I don't think we can avoid this.
>>
>> Rather than also adding this new field to the unixes specific
>> apr_proc_mutex_t struct, I also modified the latter to replace all the
>> duplicated fields by a apr_os_proc_mutex_t member (called os). Since
>> apr_proc_mutex_t is private (include/arch/unix/apr_arch_proc_mutex.h
>> for unixes), this is not an API/ABI change.
>> This change implies the use of mutex->os.[native] instead of
>> mutex->[native] in the different mechanisms implementation, but it
>> also greatly simplifies apr_os_proc_mutex_get() to something as simple
>> as *osmutex = mutex->os.
>>
>> To help disambiguate the mechanism to be used when creating an
>> apr_proc_mutex_t from a native type, I also added the new
>> apr_os_proc_mutex_put_ex() function, which takes the lockmech as
>> parameter, and allows then for example to create a APR_LOCK_FLOCK
>> mutex from a fd when the default lockmech is SYSV (both using a fd as
>> native type).
>> This, I think, addresses the showstopper from
>> http://svn.apache.org/viewvc?view=revision&revision=1587066.
>> To initialize the apr_proc_mutex_t from the native type, I slightly
>> modified the existing/private proc_mutex_choose_method() function to
>> eventually take an os type as argument and do the right thing when not
>> NULL.
>> This function is of course also implemented in other platforms (where
>> APR_LOCK_DEFAULT is to be used as lockmech since there is the only
>> choice on non-unixes), so this is a another API addon (still in
>> include/apr_portable.h).
>>
>> On OS2, I have made apr_os_proc_mutex_get() return APR_SUCCESS since
>> the existing code looks correct (but the returned status), and
>> apr_os_proc_mutex_set() return APR_ENOPOOL when a NULL pool is given,
>> for consistency with other platforms.
>>
>> For consistency on all platforms still, in the unix code this time, I
>> changed the behaviour of the proc_mutex_*_cleanup() functions so that
>> they cleanup the native mutex even if the APR mutex was created by
>> apr_os_proc_mutex_set(), taking care of not closing the fd twice (for
>> proc mutexes using a fd, since the mutex is also mapped to an
>> apr_file_t).
>> This is however inconsistant with other APR types that can be created
>> from a native type (eg. apr_os_file_t using apr_os_file_put(), quite
>> logically since the object is not really owned by APR), so maybe I
>> should have done the opposite on non-unix platforms, let me know...
>>
>> Finally, netware, where the native mutex type is NXMutex_t (and not a
>> *pointer to* NXMutex_t).
>> AFAICT (from netware sources in nks/synch.h), the NXMutex_t struct is
>> defined as :
>>
>> typedef struct
>> {
>> uint64_t reserved1;
>> void *reserved2[10];
>> } NXMutex_t;
>>
>> This is hence impossible to get/set it via the
>> apr_os_proc_mutex_get/set() interface (would require a copy of the
>> struct, which is obviously not suitable).
>> The proposed change is to typedef NXMutex_t *apr_os_proc_mutex_t
>> instead (with the *pointer to*), and do the right thing in the get/set
>> functions.
>> That's another API/ABI change though, but I doubt anyone has already
>> used netware native mutex type successfully from/to
>> apr_proc_mutex_t...
>> Since I can't test it, this change is also attached here but in a
>> distinct patch, and it will probably require a review from someone who
>> knows netware and can test it (I don't, for both).