Re: [VOTE] accept mod_macro as standard module in httpd
+1 On 03-01-2013 03:06, Eric Covener wrote: I was preparing the IP clearance forms and noticed our original vote thread was more of a discussion. I wanted to record a formal vote here so I can link to it. Pending IP clearance... [+1] accept mod_macro as a standard module and responsibility for its maintenance [ +/- 0] don't care won't help [ -1] don't accept mod_macro as a standard module My +1 -- Eric Covener cove...@gmail.com
Re: mod_lbmethod_byrequests required to define a BalancerMember
Resolved in r1428916 On Jan 3, 2013, at 4:12 PM, Jim Jagielski j...@jagunet.com wrote: I'll work on it tomorrow... a quick look indicates its a simple patch. On Jan 3, 2013, at 4:05 PM, Eric Covener cove...@gmail.com wrote: On Thu, Jan 3, 2013 at 3:39 PM, Jim Jagielski j...@jagunet.com wrote: We should be able to defer that until we actually *need* to start, as we do with other settings in trunk. I can work on that in trunk and the propose a backport to 2.4... That would be great, I probably won't bother with any interim doc unless someone else reports it or follows up.
Re: mod_lbmethod_byrequests required to define a BalancerMember
On Fri, Jan 4, 2013 at 10:14 AM, Jim Jagielski j...@jagunet.com wrote: Resolved in r1428916 thanks, that did the trick for me w/ the ProxySet $othermethod scenario.
Re: mod_lbmethod_byrequests required to define a BalancerMember
great! Thx for the quick verification. On Jan 4, 2013, at 10:37 AM, Eric Covener cove...@gmail.com wrote: On Fri, Jan 4, 2013 at 10:14 AM, Jim Jagielski j...@jagunet.com wrote: Resolved in r1428916 thanks, that did the trick for me w/ the ProxySet $othermethod scenario.
Re: [Discuss] mod_lua and database connectivity
On Thu, Jan 3, 2013 at 11:05 PM, Daniel Gruno rum...@cord.dk wrote: On 01/04/2013 12:57 AM, Brian McCallister wrote: ... Supporting luasql would be a big bonus, though I understand if goal is to provide a quick and dirty api which is backed by mod_dbd Quick and dirty makes it sound so...dirty. But yes, essentially, the purpose is to provide very basic database features for those just getting started or those who either cannot be bothered (we know who you lot are!) or can't figure out how to use luasql or other external libraries. I'd want mod_lua to be something you can sit down and get started on without having to learn how to compile lua or install libraries, BUT if you wanted/needed the extra capabilities that fx luasql provides, you could simply switch. It is not in any way intended as a full replacement, rather as a 'starter kit' so people load the module and go 'okay, what can this mod_lua puppy do for me? and not have to think about the sometimes cumbersome process of extending Lua's capabilities. I suppose I can boil my reasoning down to two major points: 1) We have apr_dbd capabilities in httpd, so why not support it in Lua? This would mean a much smaller foot print when using databases in mod_lua compared to loading an external library into every VM. 2) We'd want something that can connect through mod_dbd, which is unlikely to be supported by third party database libraries. Shouldn't this be a method on the server representation, not the request representation? There is no server handle, only a request handle given to mod_lua scripts, you should know that ;). The server handle is obtained through the request handle, so all is well and good here. It is still a method on the server, not the request, even if you get passed a request. ... Hmm, if db here represents a handle, it should prolly be paired with acquire not open. Semantics ;) but sure, we could call it 'acquire' instead of 'open'. Implied semantics matter a LOT in API design. Check your errors :-) I don't need to check errors, I just need to check whether 'rows' is a table or a nil value in the case of an error. I could've checked if 'err' was anything, but the result is the same. An API which returns (foo, err) should be error checked on the error, not the foo. This style of API will sometimes return a foo in a bad state, and an err to let you know. In your example this is fine, I am just being a pedant because if an API is designed a certain way, we shoul duse it that way :-) Also, be careful what you return, you don't want to the API to force you to realize all results from a query eagerly. Currently, it works that way - it fetches everything at once, which I grant can be something you may or may not want. I am considering adding a metatable to the 'rows' object with an __index hook that fetches new rows only when needed. Good call! I will hold off on this for a while though, as there are some memory pool concerns. Imagine if one runs a query and uses the request pool for fetching data, then waits till another request comes through before iterating through the rows, that would cause an error, unless the query was stored in the global server pool, in which case it could not get released (and then I'd have to, instead, use another pool for allocating memory, which would have to be tied to the db object - so much to do!). The global pool option is what lua-apr does with everything, which is not something I'd want mod_lua to do. -- Run a prepared statement and inject values into it: local rows, err = db:inject(r, SELECT `name` FROM `tbl` WHERE `id` = %u, 1234) Hmm, I would expect an API like local pstmt, err = h:prepare(...) ... = pstmt:execute(hello, 7) -- or ... = pstmt:query(hello, 7) or such style api. Injecting into implicit prepared statement is a strange api. Not necessarily. If the injection function stores a key/value pair of injected statements and prepares them beforehand, then a subsequent call using the same statement would just look up whether that statement has already been prepared, and there would be no need to recompile. Though, I do see the benefits of instead associating a prepared statement with a tag of your own, so I will definitely consider this as well :) Thanks for your feedback, it's very much appreciated! With regards, Daniel.
Balancer persist and inherit stuff in trunk
Have people had a chance to test, review and try the balancer persist and inheritance stuff in trunk? I want to make sure that we have some level of verification and agreement there before I work on the backports for 2.4 ;)
Re: Balancer persist and inherit stuff in trunk
On 04.01.2013 13:48, Jim Jagielski wrote: Have people had a chance to test, review and try the balancer persist and inheritance stuff in trunk? I want to make sure that we have some level of verification and agreement there before I work on the backports for 2.4;) Could you give the link to the patch/issue? I'd be happy to test it with a custom-built 2.4.3 here. Thanks! -mi
Re: Balancer persist and inherit stuff in trunk
Sorry -- I meant to send the below just to Jim, but messed-up the headers in Thunderbird -- and it ended up looking, as if Jim wrote it :( On 04.01.2013 14:06, j...@jagunet.com wrote: On 04.01.2013 13:48, Jim Jagielski wrote: Have people had a chance to test, review and try the balancer persist and inheritance stuff in trunk? I want to make sure that we have some level of verification and agreement there before I work on the backports for 2.4;) Could you give the link to the patch/issue? I'd be happy to test it with a custom-built 2.4.3 here. Thanks! -mi
Re: svn commit: r1428184 - /httpd/httpd/trunk/acinclude.m4
On Thursday 03 January 2013, Joe Orton wrote:
Maybe the wording of pkg-config's --static option is somewhat
misleading. It doesn't force linking against the static libs
(i.e. lib{ssl,crypto}.a), but outputs the libraries from
openssl.pc's Libs.private line instead.
Yup. Having httpd (mod_ssl/ab) link against whatever is listed in
Libs.private is the wrong default, it should be a special case for
the handful of people who build custom OpenSSL libraries.
Yes. Linking mod_ssl/ab with libz.so introduces spurious dependencies.
Linux distributions would have to undo that change.
thread-safety of time conversion caches in util_time.c and mod_log_config.c
I entered a bug on the thread-safety of the time conversion caches in server/util_time.c and modules/loggers/mod_log_config.c. In brief, they're not thread-safe because: 1. The algorithm depends on total memory ordering, and both the volatile qualifier and memory barriers are not used. 2. The algorithm is subject to the ABA problem. The details of the problem are here: https://issues.apache.org/bugzilla/show_bug.cgi?id=54363 I included in the bug not-yet-tested patches with a different algorithm. Do other people agree that the existing algorithm have the problems explained in detail in the bug? Thanks, Daniel Lescohier
Re: event mpm and mod_status
I just saw this from last month from Stefan Fritsch and Niklas Edmundsson: The fact that the client ip shows up on all threads points to some potential optimization: Recently active threads should be preferred, because their memory is more likely to be in the cpu caches. Right now, the thread that has been idle the longest time will do the work. Ah, virtually guaranteeing that the thread with the coldest cache gets to do the work... I definitely agree on the potential for improvement here, you would most likely want to select either the thread that processed this request last time, or the most recently active idle thread. These two conditions kinda collides though, so the challenge is probably to come up with some rather cheap selection algorithm that is good enough. Which CPU memory caches are you referring to? 1. For stack, on a new request, you're writing a new call stack, the prior request's stack was unwound. 2. For heap, you're creating a new request pool, so you will be popping an apr memnode from the head of the allocator's free list. It may even be the same memnode that was pushed onto the free list when the prior request's apr pool was freed. 3. On a new request, the next code/instructions it will execute will be the functions that reads the http request and populates the request_rec. That's different code than happened at the end of the prior request (serving the request, logging the request, etc.). So, I'm not convinced a thread selection algorithm is needed.
Re: event mpm and mod_status
On Friday 04 January 2013, Daniel Lescohier wrote: I just saw this from last month from Stefan Fritsch and Niklas Edmundsson: The fact that the client ip shows up on all threads points to some potential optimization: Recently active threads should be preferred, because their memory is more likely to be in the cpu caches. Right now, the thread that has been idle the longest time will do the work. Ah, virtually guaranteeing that the thread with the coldest cache gets to do the work... I definitely agree on the potential for improvement here, you would most likely want to select either the thread that processed this request last time, or the most recently active idle thread. These two conditions kinda collides though, so the challenge is probably to come up with some rather cheap selection algorithm that is good enough. Which CPU memory caches are you referring to? 1. For stack, on a new request, you're writing a new call stack, the prior request's stack was unwound. Even if the previous request's stack memory is freed, the new request will use the same memory which will likely be in the cpu cache. And since the stack is written to in portions smaller than a cache line, the cpu would have to read the stack from memory otherwise. 2. For heap, you're creating a new request pool, so you will be popping an apr memnode from the head of the allocator's free list. It may even be the same memnode that was pushed onto the free list when the prior request's apr pool was freed. Mpm event uses per-connection allocators. The most recently freed allocator will be used first. If this is done by the most recently active thread, it may be more likely that the allocator memory is in the cache of the correct cpu. But this may be more complex in practice. Besides, it is quite possible that the most recently used memnode is not at the top of the allocator. 3. On a new request, the next code/instructions it will execute will be the functions that reads the http request and populates the request_rec. That's different code than happened at the end of the prior request (serving the request, logging the request, etc.). If a request is handled by a previously inactive thread, it may be more likely that that thread is scheduled on a cpu core that did not previously run httpd at all. This would then cause a cold instruction cache and require a context switch. However, this depends very much on the OS and the details of the workload. So, I'm not convinced a thread selection algorithm is needed. For 1., a better thread selection would definitely be a win. For 2. and 3., it is less obvious.
Re: thread-safety of time conversion caches in util_time.c and mod_log_config.c
On Friday 04 January 2013, Daniel Lescohier wrote: I entered a bug on the thread-safety of the time conversion caches in server/util_time.c and modules/loggers/mod_log_config.c. In brief, they're not thread-safe because: 1. The algorithm depends on total memory ordering, and both the volatile qualifier and memory barriers are not used. 2. The algorithm is subject to the ABA problem. I agree that the lack of memory barriers is a problem. And it ABA problem would not exist if callers of ap_recent_* would actually check that the time *is* recent (as written in the docs in util_time.h). This is not a problem for the error log, because it always uses apr_time_now(). But the request time in mod_log_config may be further in the past. Out of interest, have you seen the function give wrong results in practice? The details of the problem are here: https://issues.apache.org/bugzilla/show_bug.cgi?id=54363 I included in the bug not-yet-tested patches with a different algorithm. Do other people agree that the existing algorithm have the problems explained in detail in the bug? I haven't reviewed your patches in detail, yet. One thing: apr_atomic_read should be enough, you don't need apr_atomic_cas with the same value. Cheers, Stefan
Re: thread-safety of time conversion caches in util_time.c and mod_log_config.c
apr_atomic_read32 is not implemented with a memory barrier. The
implementation of apr_atomic_read32 in the APR code base is just a read
from a pointer marked volatile. E.g., here is the atomic/unix/builtins.c
implementation:
APR_DECLARE(apr_uint32_t) apr_atomic_read32(volatile apr_uint32_t *mem)
{
return *mem;
}
The atomic/unix/builtins.c implementation would have to change to this to
have a memory barrier:
APR_DECLARE(apr_uint32_t) apr_atomic_read32(volatile apr_uint32_t *mem)
{
__sync_synchronize();
return *mem;
}
All the other apr atomic implementations would have to change if
apr_atomic_read32 is supposed to mean with-a-memory-barrier. All the
current implementations don't implement a memory barrier (all the
implementations use the C definition shown at the beginning of this email),
so I assume apr_atomic_read32 (and apr_atomic_set32) means
without-a-memory-barrier.
The Apache Portable Runtime API has no memory barrier function in the API
(like gcc's builtin __sync_synchronize()), so I had to choose among the
other functions in the API. I decided to use Compare-And-Swap for the
memory barrier, because the generic Compare-And-Swap concept implies the
use of memory barriers, and also the APR implementation actually implements
a memory barrier in apr_atomic_cas32. I also could've used
apr_atomic_add32(cache_element-key, 0)==seconds, but Compare-And-Swap is
the more generic concept, so I used that instead. I agree, though, for the
read-the-cache_element portion of the function, it'd be better to do just
an atomic_read, instead of a compare-and-swap, if only the APR Atomic API
had a memory barrier function that I could call before the read.
After copying from the time cache_element to this thread's memory allocated
from the request pool, the thread needs a memory barrier when checking the
cache_element-key, in order to make sure another cpu thread/core didn't
start modifying the cache_element while this thread was copying it. But
note, before doing the copy to this thread's memory, the function doesn't
use a memory barrier, it checks the cache_element-key without a memory
barrier, because it's speculatively doing the copy (because almost all the
time there won't be a conflict), and then doing the validation with a
memory barrier after the copy is done (and in the rare case that the
validation fails, the thread just calculates the time value itself).
For the old code, I haven't seen bad results in practice (not that I
probably would've noticed it), I only noticed that the code is not portably
thread-safe because I was working on a project to upgrade our web servers
to 2.4, and I was updating our own internal custom Apache modules, and I
noticed this when looking at the source of mod_log_config.c. So, I brought
it up because the algorithm does not look correct. The problem was that
the old code was supposedly a lock-free and wait-free algorithm, but it
didn't use the atomic processor instructions you need to use in order to
implement lock-free and wait-free algorithms.
On Fri, Jan 4, 2013 at 7:58 PM, Stefan Fritsch s...@sfritsch.de wrote:
On Friday 04 January 2013, Daniel Lescohier wrote:
I entered a bug on the thread-safety of the time conversion caches
in server/util_time.c and modules/loggers/mod_log_config.c.
In brief, they're not thread-safe because:
1. The algorithm depends on total memory ordering, and both the
volatile qualifier and memory barriers are not used.
2. The algorithm is subject to the ABA problem.
I agree that the lack of memory barriers is a problem.
And it ABA problem would not exist if callers of ap_recent_* would
actually check that the time *is* recent (as written in the docs in
util_time.h). This is not a problem for the error log, because it
always uses apr_time_now(). But the request time in mod_log_config may
be further in the past.
Out of interest, have you seen the function give wrong results in
practice?
The details of the problem are here:
https://issues.apache.org/bugzilla/show_bug.cgi?id=54363
I included in the bug not-yet-tested patches with a different
algorithm.
Do other people agree that the existing algorithm have the problems
explained in detail in the bug?
I haven't reviewed your patches in detail, yet. One thing:
apr_atomic_read should be enough, you don't need apr_atomic_cas with
the same value.
Cheers,
Stefan
Re: svn commit: r1428184 - /httpd/httpd/trunk/acinclude.m4
On 04.01.2013 22:40, Stefan Fritsch wrote:
On Thursday 03 January 2013, Joe Orton wrote:
Maybe the wording of pkg-config's --static option is somewhat
misleading. It doesn't force linking against the static libs
(i.e. lib{ssl,crypto}.a), but outputs the libraries from
openssl.pc's Libs.private line instead.
Yup. Having httpd (mod_ssl/ab) link against whatever is listed in
Libs.private is the wrong default, it should be a special case for
the handful of people who build custom OpenSSL libraries.
Yes. Linking mod_ssl/ab with libz.so introduces spurious dependencies.
Linux distributions would have to undo that change.
Ok, seems like I underestimated the effect of occasionally having one
or two not directly needed DT_NEEDED tags in mod_ssl.so... I was more
concerned about having an additional, clumsy (and hard-to-explain) option
in configure, but here we go:
--enable-ssl-staticlib-deps
link mod_ssl with dependencies of OpenSSL's static
libraries (as indicated by pkg-config --static).
Must be specified in addition to --enable-ssl.
Added in r1429228, 2.4.x backport proposal adapted.
Kaspar
