Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
Am 27.04.2020 um 15:57 schrieb Joe Orton: On Sat, Apr 25, 2020 at 08:10:40PM +0200, Rainer Jung wrote: Patch available at home.apache.org/~rjung/patches/httpd-trunk-mon-snaps-v1_2.patch Very nice! +1 from me. Does the times_per_thread logic still make any sense? It's always been wrong for Linux AFAICT so maybe can just be dropped. I will do some code archeology to find out who added it and why. A minor nit, I think the snap_index should be read/written using apr_atomic_t since it's going to be accessed concurrently across threads? Yup. Since we only have concurrent reads, it should suffice to check the value with apr_atomic_read32 and then use apr_atomic_set32 to set it the 0 or 1. I wonder how we best make sure, that the new monitor hook is not called from modules? Currently it is no AP_DECLARE'd but contained in httpd.h. And the hook registration is in server/core.c, but the implementation in server/scoreboard.c. No idea (not tried) whether it would be better to have it in server/core.c as a static function or whether that's not needed and it would suffice to move the function declaration to a private header file. Thanks and regards, Rainer Some notes: - in order to use the data from mod_systemd (monitor hook), which runs in the maim process, and also from mod_status, so from child processes, it needs to sit in shared memory. Since most of the data is tightly coupled to the scoreboard, I added the new cached data to the global part of the scoreboard. - it was then IMHO a good fit to move the existing ap_get_sload(ap_sload_t *ld) from server/util.c to server/scoreboard.c. - ap_sload_t is used as a collection of data which can be used to generate averaged data from a pair of ap_sload_t structs. It was extended to also contain the total request duration plus total usr and sys CPU and the timestamp when the data was taken. So it should now contain all data from which mod-systemd and mod_status currently like to display derived averaged values. - busy and idle in ap_sload_t have been changed from int to float, because they were already only used a percentages. IMHO it doesn't make sense to express idle and busy as percentages based on a total of busy plus idle, because that sum is not a meaningful total. The server can grow by creating new processes and suddenly your busy percentage might shrink, although the absolute number of busy threads increases. That's counter intuitive. So I added the unused process slots to the total count, and we have three counters that sum up to this total, busy, idle and dead. We need a better name than "dead" for these unused process slots that might get used later. "unused" is to close to "idle" and I don't have a good name yet. - the new ap_mon_snap_t contains a pointer to an ap_sload_t, six averaged values generated from two ap_sload_t and a member that conatins the time delta between those two ap_sload_t. The ap_sload_t referenced by ap_mon_snap_t contains the data at time t1. During the next monitor run, new t1 data will be collected and the previous t1 data will be used as t0 data to generate the new averages. - the scoreboard contains two ap_mon_snap_t (plus the two ap_sload_t referenced by them) to be able to update one of them without breaking access by consumers to the other one. After the update the roles get switched. - both structs, ap_sload_t and ap_mon_snap_t are declared in httpd.h, because ap_sload_t already had been there. t might be a better fit to move them to scoreboard.h, but I'm not sure about that and I don't know if that would be allowed by the compatibility rules. - also in httpd.h there are now three new function declarations. Two only used by server/core.c as hook functions: int ap_scoreboard_monitor(apr_pool_t *p, server_rec *s); void ap_scoreboard_child_init(apr_pool_t *p, server_rec *s); and one for public consumption: AP_DECLARE(void) ap_get_mon_snap(ap_mon_snap_t *ms); - mod_systemd and mod_status now call ap_get_mon_snap() to retrieve the latest averaged data. mod_status still uses the scoreboard in addition to retrieve up-to-date current scalar metrics. Small adjustments to the mod_status output plus additions to mod_systemd notification messages. Tne STATUS in the notification of mod_systemd now looks liek this: STATUS=Pct Idle workers 28.4; Pct Busy workers 10.6; Pct Dead workers 60.9; Requests/sec: 5030; Bytes served/sec: 2.9MB/sec; Bytes served/request: 596 B/req; Avg Duration(ms): 5.78; Avg Concurrency: 29.1; Cpu Pct: 40.5 - scoreboard.c changes: - take over ap_get_sload(ap_sload_t *sl) from server/util.c. Added copied code from mod_status to that function to also add up the request duration and the usr and sys CPU times. - ap_scoreboard_monitor() runs in the monitor hook. It calls ap_get_sload() and then a static utility function calc_mon_data() to calculate the new averages. - Minor mmn change not yet part of the patch. It compiles fine (maintainer mode) on RHEL 7 x86_64
Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
Am 27.04.2020 um 17:28 schrieb Yann Ylavic: On Mon, Apr 27, 2020 at 5:18 PM Rainer Jung wrote: Hi Yann, Am 27.04.2020 um 16:40 schrieb Yann Ylavic: Hi Rainer, On Mon, Apr 27, 2020 at 3:17 PM Rainer Jung wrote: Thanks for this. Could you please create this as a PR on github as well? This ensures that all the Travis tests are run for your patch. Thanks Rüdiger. Done and indeed Travis found one not that I fixed but we need to discuss. Thanks Rainer, looks very nice. Could we avoid the "ap_sload_t *sload" pointer in ap_mon_snap_t somehow? I'm asking because snap0 and snap1 in global_score (SHM) end up containing a pointer now. My initial version had the fields of ap_sload_t directly inside ap_mon_snap_t instead of the pointer. I changed it, because - it allowed to more safely copy the whole ap_sload_t without relying on keeping two struct definitions in sync - it allowed to extend ap_sload_t and ap_mon_snap_t at their respective ends and stay compatible. You have a problem with the pointers because of possible alignment issues or what kind of trouble might we run into? Or is it just hard to understand (and more comments in the code will help)? My concern is more about security, e.g. CVE-2019-0211 kind of things, with an unprivileged user (child) playing with this pointer and making the main process (monitor?) at best crash, at worst do anything they want (root privilege escalation). I see. Although I don't see a way how we would execute code at the derefenced address, as a safety guard for future code changes we can easily avoid using the pointers in the scoreboard and instead use sload0 / snap0 and sload1 / snap1 when doing the calculations. Since the pointers (after init) would have been constant, we can hard code it into trivial logic instead. One could then use a separate type for snap0 and snap1 in the scoreboard, but I would find it better to stay with the ap_mon_snap_t type, even if we do not use the pointer there. For the struct passed in from the consumer, I would still use the pointer as-is. Will provide a change on top of the PR for further discussion. Thanks for the heads up. Didn't think about using the scoreboard and the monitor hook running in the parent havig security implications. One really needs to be careful about assumptions for the scoreboard data! Regards, Rainer
Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
On Mon, Apr 27, 2020 at 5:18 PM Rainer Jung wrote: > > Hi Yann, > > Am 27.04.2020 um 16:40 schrieb Yann Ylavic: > > Hi Rainer, > > > > On Mon, Apr 27, 2020 at 3:17 PM Rainer Jung wrote: > >>> > >>> Thanks for this. > >>> Could you please create this as a PR on github as well? This ensures that > >>> all the Travis tests are run for your patch. > >> > >> Thanks Rüdiger. Done and indeed Travis found one not that I fixed but we > >> need to discuss. > > > > Thanks Rainer, looks very nice. > > > > Could we avoid the "ap_sload_t *sload" pointer in ap_mon_snap_t somehow? > > I'm asking because snap0 and snap1 in global_score (SHM) end up > > containing a pointer now. > > My initial version had the fields of ap_sload_t directly inside > ap_mon_snap_t instead of the pointer. > > I changed it, because > > - it allowed to more safely copy the whole ap_sload_t without relying on > keeping two struct definitions in sync > > - it allowed to extend ap_sload_t and ap_mon_snap_t at their respective > ends and stay compatible. > > You have a problem with the pointers because of possible alignment > issues or what kind of trouble might we run into? Or is it just hard to > understand (and more comments in the code will help)? My concern is more about security, e.g. CVE-2019-0211 kind of things, with an unprivileged user (child) playing with this pointer and making the main process (monitor?) at best crash, at worst do anything they want (root privilege escalation). Regards, Yann.
Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
Hi Yann, Am 27.04.2020 um 16:40 schrieb Yann Ylavic: Hi Rainer, On Mon, Apr 27, 2020 at 3:17 PM Rainer Jung wrote: Thanks for this. Could you please create this as a PR on github as well? This ensures that all the Travis tests are run for your patch. Thanks Rüdiger. Done and indeed Travis found one not that I fixed but we need to discuss. Thanks Rainer, looks very nice. Could we avoid the "ap_sload_t *sload" pointer in ap_mon_snap_t somehow? I'm asking because snap0 and snap1 in global_score (SHM) end up containing a pointer now. My initial version had the fields of ap_sload_t directly inside ap_mon_snap_t instead of the pointer. I changed it, because - it allowed to more safely copy the whole ap_sload_t without relying on keeping two struct definitions in sync - it allowed to extend ap_sload_t and ap_mon_snap_t at their respective ends and stay compatible. You have a problem with the pointers because of possible alignment issues or what kind of trouble might we run into? Or is it just hard to understand (and more comments in the code will help)? Or because it is harder for consumers to use the API in a correct way (the pointer in ap_mon_snap_t might be uninitialized)? In the latter case: one could also see this as a feature instead of as a bug: as long as one is only interested in the averaged data and not in the sload data, you can pass in an ap_mon_snap_t with a null pointer to the ap_sload_t. Regards, Rainer
Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
Hi Rainer, On Mon, Apr 27, 2020 at 3:17 PM Rainer Jung wrote: > > > > Thanks for this. > > Could you please create this as a PR on github as well? This ensures that > > all the Travis tests are run for your patch. > > Thanks Rüdiger. Done and indeed Travis found one not that I fixed but we > need to discuss. Thanks Rainer, looks very nice. Could we avoid the "ap_sload_t *sload" pointer in ap_mon_snap_t somehow? I'm asking because snap0 and snap1 in global_score (SHM) end up containing a pointer now. Regards, Yann.
Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
On Sat, Apr 25, 2020 at 08:10:40PM +0200, Rainer Jung wrote: > Patch available at > > home.apache.org/~rjung/patches/httpd-trunk-mon-snaps-v1_2.patch Very nice! +1 from me. Does the times_per_thread logic still make any sense? It's always been wrong for Linux AFAICT so maybe can just be dropped. A minor nit, I think the snap_index should be read/written using apr_atomic_t since it's going to be accessed concurrently across threads? Regards, Joe > Some notes: > > - in order to use the data from mod_systemd (monitor hook), which runs in > the maim process, and also from mod_status, so from child processes, it > needs to sit in shared memory. Since most of the data is tightly coupled to > the scoreboard, I added the new cached data to the global part of the > scoreboard. > > - it was then IMHO a good fit to move the existing ap_get_sload(ap_sload_t > *ld) from server/util.c to server/scoreboard.c. > > - ap_sload_t is used as a collection of data which can be used to generate > averaged data from a pair of ap_sload_t structs. It was extended to also > contain the total request duration plus total usr and sys CPU and the > timestamp when the data was taken. So it should now contain all data from > which mod-systemd and mod_status currently like to display derived averaged > values. > > - busy and idle in ap_sload_t have been changed from int to float, because > they were already only used a percentages. IMHO it doesn't make sense to > express idle and busy as percentages based on a total of busy plus idle, > because that sum is not a meaningful total. The server can grow by creating > new processes and suddenly your busy percentage might shrink, although the > absolute number of busy threads increases. That's counter intuitive. So I > added the unused process slots to the total count, and we have three > counters that sum up to this total, busy, idle and dead. We need a better > name than "dead" for these unused process slots that might get used later. > "unused" is to close to "idle" and I don't have a good name yet. > > - the new ap_mon_snap_t contains a pointer to an ap_sload_t, six averaged > values generated from two ap_sload_t and a member that conatins the time > delta between those two ap_sload_t. The ap_sload_t referenced by > ap_mon_snap_t contains the data at time t1. During the next monitor run, new > t1 data will be collected and the previous t1 data will be used as t0 data > to generate the new averages. > > - the scoreboard contains two ap_mon_snap_t (plus the two ap_sload_t > referenced by them) to be able to update one of them without breaking access > by consumers to the other one. After the update the roles get switched. > > - both structs, ap_sload_t and ap_mon_snap_t are declared in httpd.h, > because ap_sload_t already had been there. t might be a better fit to move > them to scoreboard.h, but I'm not sure about that and I don't know if that > would be allowed by the compatibility rules. > > - also in httpd.h there are now three new function declarations. Two only > used by server/core.c as hook functions: > > int ap_scoreboard_monitor(apr_pool_t *p, server_rec *s); > void ap_scoreboard_child_init(apr_pool_t *p, server_rec *s); > > and one for public consumption: > > AP_DECLARE(void) ap_get_mon_snap(ap_mon_snap_t *ms); > > - mod_systemd and mod_status now call ap_get_mon_snap() to retrieve the > latest averaged data. mod_status still uses the scoreboard in addition to > retrieve up-to-date current scalar metrics. Small adjustments to the > mod_status output plus additions to mod_systemd notification messages. Tne > STATUS in the notification of mod_systemd now looks liek this: > > STATUS=Pct Idle workers 28.4; Pct Busy workers 10.6; Pct Dead workers 60.9; > Requests/sec: 5030; Bytes served/sec: 2.9MB/sec; Bytes served/request: 596 > B/req; Avg Duration(ms): 5.78; Avg Concurrency: 29.1; Cpu Pct: 40.5 > > - scoreboard.c changes: > > - take over ap_get_sload(ap_sload_t *sl) from server/util.c. > Added copied code from mod_status to that function to also add up the > request duration and the usr and sys CPU times. > > - ap_scoreboard_monitor() runs in the monitor hook. It calls > ap_get_sload() and then a static utility function calc_mon_data() to > calculate the new averages. > > - Minor mmn change not yet part of the patch. > > It compiles fine (maintainer mode) on RHEL 7 x86_64 and on Solaris 10 Sparc > and I did some tests with mod_status and mod_systemd. > > Regards, > > Rainer > > Am 24.04.2020 um 18:32 schrieb Rainer Jung: > > Am 24.04.2020 um 16:21 schrieb Joe Orton: > > > On Fri, Apr 24, 2020 at 12:17:19PM +0200, Rainer Jung wrote: > > > > Thinking further: I think it would make sense to have a module or core > > > > implement the monitor hook to generate that derived data (requests/sec, > > > > bytes/sec, durationMs/request, avgConcurrency) in the last > > > > monitor interval > > > > and to provide that data to consumers like
Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
Am 27.04.2020 um 08:57 schrieb Ruediger Pluem: On 4/25/20 8:10 PM, Rainer Jung wrote: Patch available at home.apache.org/~rjung/patches/httpd-trunk-mon-snaps-v1_2.patch Thanks for this. Could you please create this as a PR on github as well? This ensures that all the Travis tests are run for your patch. Thanks Rüdiger. Done and indeed Travis found one not that I fixed but we need to discuss. I changed the data type of the idle and busy fields in the sload struct from int to float, because they were used as floats. But that broke mod_headers which uses the data and will break other consumers as well. For compatibility reasons, we could keep the int type though. Regards, Rainer
Re: Providing near-time averaged monitoring data for mod_systemd and mod_status
On 4/25/20 8:10 PM, Rainer Jung wrote: > Patch available at > > home.apache.org/~rjung/patches/httpd-trunk-mon-snaps-v1_2.patch Thanks for this. Could you please create this as a PR on github as well? This ensures that all the Travis tests are run for your patch. Regards Rüdiger
Providing near-time averaged monitoring data for mod_systemd and mod_status
Patch available at home.apache.org/~rjung/patches/httpd-trunk-mon-snaps-v1_2.patch Some notes: - in order to use the data from mod_systemd (monitor hook), which runs in the maim process, and also from mod_status, so from child processes, it needs to sit in shared memory. Since most of the data is tightly coupled to the scoreboard, I added the new cached data to the global part of the scoreboard. - it was then IMHO a good fit to move the existing ap_get_sload(ap_sload_t *ld) from server/util.c to server/scoreboard.c. - ap_sload_t is used as a collection of data which can be used to generate averaged data from a pair of ap_sload_t structs. It was extended to also contain the total request duration plus total usr and sys CPU and the timestamp when the data was taken. So it should now contain all data from which mod-systemd and mod_status currently like to display derived averaged values. - busy and idle in ap_sload_t have been changed from int to float, because they were already only used a percentages. IMHO it doesn't make sense to express idle and busy as percentages based on a total of busy plus idle, because that sum is not a meaningful total. The server can grow by creating new processes and suddenly your busy percentage might shrink, although the absolute number of busy threads increases. That's counter intuitive. So I added the unused process slots to the total count, and we have three counters that sum up to this total, busy, idle and dead. We need a better name than "dead" for these unused process slots that might get used later. "unused" is to close to "idle" and I don't have a good name yet. - the new ap_mon_snap_t contains a pointer to an ap_sload_t, six averaged values generated from two ap_sload_t and a member that conatins the time delta between those two ap_sload_t. The ap_sload_t referenced by ap_mon_snap_t contains the data at time t1. During the next monitor run, new t1 data will be collected and the previous t1 data will be used as t0 data to generate the new averages. - the scoreboard contains two ap_mon_snap_t (plus the two ap_sload_t referenced by them) to be able to update one of them without breaking access by consumers to the other one. After the update the roles get switched. - both structs, ap_sload_t and ap_mon_snap_t are declared in httpd.h, because ap_sload_t already had been there. t might be a better fit to move them to scoreboard.h, but I'm not sure about that and I don't know if that would be allowed by the compatibility rules. - also in httpd.h there are now three new function declarations. Two only used by server/core.c as hook functions: int ap_scoreboard_monitor(apr_pool_t *p, server_rec *s); void ap_scoreboard_child_init(apr_pool_t *p, server_rec *s); and one for public consumption: AP_DECLARE(void) ap_get_mon_snap(ap_mon_snap_t *ms); - mod_systemd and mod_status now call ap_get_mon_snap() to retrieve the latest averaged data. mod_status still uses the scoreboard in addition to retrieve up-to-date current scalar metrics. Small adjustments to the mod_status output plus additions to mod_systemd notification messages. Tne STATUS in the notification of mod_systemd now looks liek this: STATUS=Pct Idle workers 28.4; Pct Busy workers 10.6; Pct Dead workers 60.9; Requests/sec: 5030; Bytes served/sec: 2.9MB/sec; Bytes served/request: 596 B/req; Avg Duration(ms): 5.78; Avg Concurrency: 29.1; Cpu Pct: 40.5 - scoreboard.c changes: - take over ap_get_sload(ap_sload_t *sl) from server/util.c. Added copied code from mod_status to that function to also add up the request duration and the usr and sys CPU times. - ap_scoreboard_monitor() runs in the monitor hook. It calls ap_get_sload() and then a static utility function calc_mon_data() to calculate the new averages. - Minor mmn change not yet part of the patch. It compiles fine (maintainer mode) on RHEL 7 x86_64 and on Solaris 10 Sparc and I did some tests with mod_status and mod_systemd. Regards, Rainer Am 24.04.2020 um 18:32 schrieb Rainer Jung: Am 24.04.2020 um 16:21 schrieb Joe Orton: On Fri, Apr 24, 2020 at 12:17:19PM +0200, Rainer Jung wrote: Thinking further: I think it would make sense to have a module or core implement the monitor hook to generate that derived data (requests/sec, bytes/sec, durationMs/request, avgConcurrency) in the last monitor interval and to provide that data to consumers like mod_systemd or - new - mod_status - instead of the long term averages since start. It could probably be added to the code that already provides "sload". That way mod_status would also profit from the more precise average values (taken over the last monitor interval). I definitely like the patch, it has bothered me that the "per second" stats are not very useful but wasn't sure how to make it better. This is also an interesting idea. So you would suggest having a new monitor hook which runs REALLY_LAST in the order, calls