Re: Align worker's worker's fdqueue with event's?

[Luca Toscano]

2018-01-12 13:34 GMT+01:00 Ruediger Pluem :

>
>
> On 01/12/2018 01:32 PM, Eric Covener wrote:
> > On Fri, Jan 12, 2018 at 6:51 AM, Yann Ylavic 
> wrote:
> >> A bit orthogonal, I'd also like to sync 2.4.x "event" with trunk's
> >> w.r.t. cosmetic changes before (and to help) further backport
> >> proposals.
> >>
> >> That's possibly something that'll help *us* for later backports, but
> >> not necessarily distros with (security-)fixes only policy.
> >> Is that something we should more care about? I suppose distro
> >> maintainers do care...
> >>
> >> For instance, the three attached patches are how I would stage latest
> >> "event" changes in 2.4.x:
> >> - patch 1: align with trunk what can/needs to be (cosmetics);
> >> - patch 2: optimizations and correctness which don't seem to have
> >> bitten us so far (not a proven fix someow);
> >> - patch 3: a wakeup fix (corner case) that applies almost cleanly
> >> thanks to 1/ and 2/.
> >>
> >> Would this work or should I go with 3/ directly and resolve backport
> >> conflicts there?
> >> Or maybe go with 3/ then 2/ then 1/, for the same result but at least
> >> distros would care of the first step only (for this time...)?
> >
> > Looks reasonable to me, better to rip the band-aid off then pay the
> > price/risk every time something needs to be backported.
> >
>
> +1
>
>
+1!

Re: Align worker's worker's fdqueue with event's?

[Jim Jagielski]

> On Jan 12, 2018, at 7:32 AM, Eric Covener  wrote:
> 
> On Fri, Jan 12, 2018 at 6:51 AM, Yann Ylavic  > wrote:
>> A bit orthogonal, I'd also like to sync 2.4.x "event" with trunk's
>> w.r.t. cosmetic changes before (and to help) further backport
>> proposals.
>> 
>> That's possibly something that'll help *us* for later backports, but
>> not necessarily distros with (security-)fixes only policy.
>> Is that something we should more care about? I suppose distro
>> maintainers do care...
>> 
>> For instance, the three attached patches are how I would stage latest
>> "event" changes in 2.4.x:
>> - patch 1: align with trunk what can/needs to be (cosmetics);
>> - patch 2: optimizations and correctness which don't seem to have
>> bitten us so far (not a proven fix someow);
>> - patch 3: a wakeup fix (corner case) that applies almost cleanly
>> thanks to 1/ and 2/.
>> 
>> Would this work or should I go with 3/ directly and resolve backport
>> conflicts there?
>> Or maybe go with 3/ then 2/ then 1/, for the same result but at least
>> distros would care of the first step only (for this time...)?
> 
> Looks reasonable to me, better to rip the band-aid off then pay the
> price/risk every time something needs to be backported.

+1

BTW, there is no need for some of this stuff to be part of the public
API.

Re: Align worker's worker's fdqueue with event's?

[Eric Covener]

> I don't mean to make it API, still a private (unix specific/common)
> thing, something like "os/unix/unixd.c"'s non-AP_DECLARE things.

seems like there should be no big surprises here with analogs like
unixd, mpm_common, etc

Re: Align worker's worker's fdqueue with event's?

[Ruediger Pluem]

On 01/12/2018 01:32 PM, Eric Covener wrote:
> On Fri, Jan 12, 2018 at 6:51 AM, Yann Ylavic  wrote:
>> A bit orthogonal, I'd also like to sync 2.4.x "event" with trunk's
>> w.r.t. cosmetic changes before (and to help) further backport
>> proposals.
>>
>> That's possibly something that'll help *us* for later backports, but
>> not necessarily distros with (security-)fixes only policy.
>> Is that something we should more care about? I suppose distro
>> maintainers do care...
>>
>> For instance, the three attached patches are how I would stage latest
>> "event" changes in 2.4.x:
>> - patch 1: align with trunk what can/needs to be (cosmetics);
>> - patch 2: optimizations and correctness which don't seem to have
>> bitten us so far (not a proven fix someow);
>> - patch 3: a wakeup fix (corner case) that applies almost cleanly
>> thanks to 1/ and 2/.
>>
>> Would this work or should I go with 3/ directly and resolve backport
>> conflicts there?
>> Or maybe go with 3/ then 2/ then 1/, for the same result but at least
>> distros would care of the first step only (for this time...)?
> 
> Looks reasonable to me, better to rip the band-aid off then pay the
> price/risk every time something needs to be backported.
> 

+1

Regards

Rüdiger

Re: Align worker's worker's fdqueue with event's?

[Eric Covener]

On Fri, Jan 12, 2018 at 6:51 AM, Yann Ylavic  wrote:
> A bit orthogonal, I'd also like to sync 2.4.x "event" with trunk's
> w.r.t. cosmetic changes before (and to help) further backport
> proposals.
>
> That's possibly something that'll help *us* for later backports, but
> not necessarily distros with (security-)fixes only policy.
> Is that something we should more care about? I suppose distro
> maintainers do care...
>
> For instance, the three attached patches are how I would stage latest
> "event" changes in 2.4.x:
> - patch 1: align with trunk what can/needs to be (cosmetics);
> - patch 2: optimizations and correctness which don't seem to have
> bitten us so far (not a proven fix someow);
> - patch 3: a wakeup fix (corner case) that applies almost cleanly
> thanks to 1/ and 2/.
>
> Would this work or should I go with 3/ directly and resolve backport
> conflicts there?
> Or maybe go with 3/ then 2/ then 1/, for the same result but at least
> distros would care of the first step only (for this time...)?

Looks reasonable to me, better to rip the band-aid off then pay the
price/risk every time something needs to be backported.

Re: Align worker's worker's fdqueue with event's?

[Yann Ylavic]

A bit orthogonal, I'd also like to sync 2.4.x "event" with trunk's
w.r.t. cosmetic changes before (and to help) further backport
proposals.

That's possibly something that'll help *us* for later backports, but
not necessarily distros with (security-)fixes only policy.
Is that something we should more care about? I suppose distro
maintainers do care...

For instance, the three attached patches are how I would stage latest
"event" changes in 2.4.x:
- patch 1: align with trunk what can/needs to be (cosmetics);
- patch 2: optimizations and correctness which don't seem to have
bitten us so far (not a proven fix someow);
- patch 3: a wakeup fix (corner case) that applies almost cleanly
thanks to 1/ and 2/.

Would this work or should I go with 3/ directly and resolve backport
conflicts there?
Or maybe go with 3/ then 2/ then 1/, for the same result but at least
distros would care of the first step only (for this time...)?
Merge r1605328, r1629576 from trunk:

event: minify local variables scope.

event: have_idle_worker must not be cleared in every listener_thread iteration.
Fixes bug when workers were not stopped after graceful restart (introduced in
r1605328).

Submitted by: takashi, jkaluza

diff --git a/server/mpm/event/event.c b/server/mpm/event/event.c
index 54c9256074..1a5f70e55c 100644
--- a/server/mpm/event/event.c
+++ b/server/mpm/event/event.c
@@ -1545,22 +1545,14 @@ static void process_keepalive_queue(apr_time_t timeout_time)
 
 static void * APR_THREAD_FUNC listener_thread(apr_thread_t * thd, void *dummy)
 {
-timer_event_t *te;
 apr_status_t rc;
 proc_info *ti = dummy;
 int process_slot = ti->pslot;
 struct process_score *ps = ap_get_scoreboard_process(process_slot);
 apr_pool_t *tpool = apr_thread_pool_get(thd);
-void *csd = NULL;
-apr_pool_t *ptrans; /* Pool for per-transaction stuff */
-ap_listen_rec *lr;
-int have_idle_worker = 0;
-const apr_pollfd_t *out_pfd;
-apr_int32_t num = 0;
-apr_interval_time_t timeout_interval;
-apr_time_t timeout_time = 0, now, last_log;
-listener_poll_type *pt;
 int closed = 0, listeners_disabled = 0;
+int have_idle_worker = 0;
+apr_time_t last_log;
 
 last_log = apr_time_now();
 free(ti);
@@ -1581,6 +1573,11 @@ static void * APR_THREAD_FUNC listener_thread(apr_thread_t * thd, void *dummy)
 apr_signal(LISTENER_SIGNAL, dummy_signal_handler);
 
 for (;;) {
+timer_event_t *te;
+const apr_pollfd_t *out_pfd;
+apr_int32_t num = 0;
+apr_interval_time_t timeout_interval;
+apr_time_t now, timeout_time;
 int workers_were_busy = 0;
 
 if (listener_may_exit) {
@@ -1693,7 +1690,7 @@ static void * APR_THREAD_FUNC listener_thread(apr_thread_t * thd, void *dummy)
 }
 
 while (num) {
-pt = (listener_poll_type *) out_pfd->client_data;
+listener_poll_type *pt = (listener_poll_type *) out_pfd->client_data;
 if (pt->type == PT_CSD) {
 /* one of the sockets is readable */
 event_conn_state_t *cs = (event_conn_state_t *) pt->baton;
@@ -1785,7 +1782,9 @@ static void * APR_THREAD_FUNC listener_thread(apr_thread_t * thd, void *dummy)
 enable_listensocks(process_slot);
 }
 if (!listeners_disabled) {
-lr = (ap_listen_rec *) pt->baton;
+void *csd = NULL;
+ap_listen_rec *lr = (ap_listen_rec *) pt->baton;
+apr_pool_t *ptrans; /* Pool for per-transaction stuff */
 ap_pop_pool(, worker_queue_info);
 
 if (ptrans == NULL) {
@@ -1968,12 +1967,8 @@ static void *APR_THREAD_FUNC worker_thread(apr_thread_t * thd, void *dummy)
 proc_info *ti = dummy;
 int process_slot = ti->pslot;
 int thread_slot = ti->tslot;
-apr_socket_t *csd = NULL;
-event_conn_state_t *cs;
-apr_pool_t *ptrans; /* Pool for per-transaction stuff */
 apr_status_t rv;
 int is_idle = 0;
-timer_event_t *te = NULL;
 
 free(ti);
 
@@ -1984,6 +1979,11 @@ static void *APR_THREAD_FUNC worker_thread(apr_thread_t * thd, void *dummy)
 SERVER_STARTING, NULL);
 
 while (!workers_may_exit) {
+apr_socket_t *csd = NULL;
+event_conn_state_t *cs;
+timer_event_t *te = NULL;
+apr_pool_t *ptrans; /* Pool for per-transaction stuff */
+
 if (!is_idle) {
 rv = ap_queue_info_set_idle(worker_queue_info, NULL);
 if (rv != APR_SUCCESS) {
@@ -2007,7 +2007,6 @@ static void *APR_THREAD_FUNC worker_thread(apr_thread_t * thd, void *dummy)
 break;
 }
 
-te = NULL;
 rv = ap_queue_pop_something(worker_queue, , , , );
 
 if (rv != APR_SUCCESS) {
Merge r1643279, r1703241, r1802535, r1819847, r1819848, r1819852, r1819853 from trunk:

mpm_event(opt): avoid 

Re: Align worker's worker's fdqueue with event's?

[Yann Ylavic]

On Thu, Jan 11, 2018 at 1:34 PM, Stefan Eissing
 wrote:
>> Am 11.01.2018 um 13:02 schrieb Yann Ylavic :
>>
>> there a several optimizations and correctness fixes in event/fdqueue.c
>> that don't land in worker/fdqueue.c.
[]
> If we had a single, nice build system, I'd be all for it. With the
> current state of things, I am not sure it is worth our time to
> think about common code that is not in the public API,

At least that'd avoid missing fixes/improvements made in one code and
not the other.
Not sure fixing twice or more isn't what can consume our time too.

>
> Making both files the same, might still be worth it since future
> enhancements can be copied/patched more easily. And there are not
> *that* many people working in this area.

Which also suggests code should be centralized IMO, because new comers
may not know about the duplication and the need to fix in multiple
places.

>
>> For now things that are event only are not aligned (e.g. timers), but
>> ultimately I'd like to have a single fdqueue.[ch] for both MPMs (not
>> too far once this patch is applied), that'd certainly help maintenance
>> and improvements for both.
>> If you agree on this, what would be the best practice/place for the common 
>> code?
>
> Making things part of the API has a high price in back porting costs and
> not being able to take things away again.

I don't mean to make it API, still a private (unix specific/common)
thing, something like "os/unix/unixd.c"'s non-AP_DECLARE things.

>
> Related issues exist in modules too. mod_http2 and mod_proxy_http2 would
> like to share. mod_md once had an executable with shared code.
>
> I eliminated the sharing and the executable rather than having to deal
> with that pain. Not ideal, but there is limited time to put into such
> things.

Yeah, sharing from/between modules (APR_OPTIONAL) is another beast I'd
like to avoid here for 2 "unix" MPMs, hence the core non-AP_DECLARE
(hidden) proposal maybe.


Thanks,
Yann.

Re: Align worker's worker's fdqueue with event's?

[Stefan Eissing]

> Am 11.01.2018 um 13:02 schrieb Yann Ylavic :
> 
> Hi,
> 
> there a several optimizations and correctness fixes in event/fdqueue.c
> that don't land in worker/fdqueue.c.
> 
> The patch would look like the attached.
> It also include some cosmectic changes in event (mainly s/type *
> arg/type *arg/) ala worker which suits more with the style used
> elsewhere in httpd.
> 
> Thoughts?

If we had a single, nice build system, I'd be all for it. With the
current state of things, I am not sure it is worth our time to
think about common code that is not in the public API,

Making both files the same, might still be worth it since future
enhancements can be copied/patched more easily. And there are not
*that* many people working in this area.

> For now things that are event only are not aligned (e.g. timers), but
> ultimately I'd like to have a single fdqueue.[ch] for both MPMs (not
> too far once this patch is applied), that'd certainly help maintenance
> and improvements for both.
> If you agree on this, what would be the best practice/place for the common 
> code?

Making things part of the API has a high price in back porting costs and 
not being able to take things away again. 

Related issues exist in modules too. mod_http2 and mod_proxy_http2 would
like to share. mod_md once had an executable with shared code.

I eliminated the sharing and the executable rather than having to deal 
with that pain. Not ideal, but there is limited time to put into such
things.


Cheers,

Stefan