Re: memory leak in 2.2.x balancer?
Akins, Brian wrote: On 11/28/07 1:41 PM, "Mladen Turk" <[EMAIL PROTECTED]> wrote: This code is simply a scoreboard callback. We were talking of the API that would allow module to register the 'shared memory intention'. Then the core will take care of creating/attaching/removing the shared memory. It will be the separate per-module shared memory handled the same way, with the same security as the scoreboard is. If you strip away the "per thread" stuff in mod_slotmem, it does that. Right, but I was talking on the extending scoreboard.c so that module can hook the scoreboard creation, as well hooking things like: ap_increment_counts ap_update_child_status that will be called for each shared memory a particular module created. So in essence you will have a scoreboard and a number of module scoreboards. Each core scoreboard callback will have a particular module hook, thus allowing module ABI, not core one. Regards, Mladen
Re: memory leak in 2.2.x balancer?
On 11/28/07 1:41 PM, "Mladen Turk" <[EMAIL PROTECTED]> wrote: > > This code is simply a scoreboard callback. > > We were talking of the API that would allow module to > register the 'shared memory intention'. Then the > core will take care of creating/attaching/removing the shared > memory. It will be the separate per-module shared memory > handled the same way, with the same security as the scoreboard is. If you strip away the "per thread" stuff in mod_slotmem, it does that. -- Brian Akins Chief Operations Engineer Turner Digital Media Technologies
Re: memory leak in 2.2.x balancer?
Akins, Brian wrote: On 11/28/07 11:20 AM, "Jim Jagielski" <[EMAIL PROTECTED]> wrote: Agreed. However, we should have an httpd api that will allow module to register his own shared memory, that will be managed and handled like scoreboard with probably the 'generation' extension to allow graceful restarts with configuration changed. +1 http://marc.info/?l=apache-httpd-dev&m=115694865205979&w=2 This code is simply a scoreboard callback. We were talking of the API that would allow module to register the 'shared memory intention'. Then the core will take care of creating/attaching/removing the shared memory. It will be the separate per-module shared memory handled the same way, with the same security as the scoreboard is. The scoreboard lacks one major feature, and that is graceful restart generation. Modules that can have changed config during restart (like adding new worker in mod_proxy_balancer) can observe strange things if changed config reorders the config directives. This requires that core tracks the two parallel shared memories, and destroys the original when all child exits. The API itself shouldn't be a problem. A separate hook should be enough. We can even add persistence to it so modules can save data on restarts (this would require merging the original shared memory), but it's doable. Regards, Mladen
Re: memory leak in 2.2.x balancer?
On 11/28/07 11:20 AM, "Jim Jagielski" <[EMAIL PROTECTED]> wrote: >> Agreed. >> However, we should have an httpd api that will allow >> module to register his own shared memory, that will >> be managed and handled like scoreboard with probably >> the 'generation' extension to allow graceful restarts >> with configuration changed. >> > > +1 http://marc.info/?l=apache-httpd-dev&m=115694865205979&w=2 -- Brian Akins Chief Operations Engineer Turner Digital Media Technologies
Re: memory leak in 2.2.x balancer?
On Nov 28, 2007, at 10:04 AM, Mladen Turk wrote: Jim Jagielski wrote: On Nov 28, 2007, at 3:39 AM, jean-frederic clere wrote: One of the question is should we go on using scoreboard to store the balancers and workers information or should we already add a layer to a provider that will provide all the features we need to handle the balancers and workers informations. The scoreboard as a central place where everyone plops data in is an experiment which, imo, has failed. There's really no need for that anymore... Agreed. However, we should have an httpd api that will allow module to register his own shared memory, that will be managed and handled like scoreboard with probably the 'generation' extension to allow graceful restarts with configuration changed. +1
Re: memory leak in 2.2.x balancer?
Jim Jagielski wrote: On Nov 28, 2007, at 3:39 AM, jean-frederic clere wrote: One of the question is should we go on using scoreboard to store the balancers and workers information or should we already add a layer to a provider that will provide all the features we need to handle the balancers and workers informations. The scoreboard as a central place where everyone plops data in is an experiment which, imo, has failed. There's really no need for that anymore... Agreed. However, we should have an httpd api that will allow module to register his own shared memory, that will be managed and handled like scoreboard with probably the 'generation' extension to allow graceful restarts with configuration changed. Regards, Mladen
Re: memory leak in 2.2.x balancer?
On Nov 28, 2007, at 3:39 AM, jean-frederic clere wrote: One of the question is should we go on using scoreboard to store the balancers and workers information or should we already add a layer to a provider that will provide all the features we need to handle the balancers and workers informations. The scoreboard as a central place where everyone plops data in is an experiment which, imo, has failed. There's really no need for that anymore...
Re: memory leak in 2.2.x balancer?
On 11/28/07 3:39 AM, "jean-frederic clere" <[EMAIL PROTECTED]> wrote: > One of the question is should we go on using scoreboard to store the > balancers and workers information No. There were a couple of alternatives discussed a few (?) months ago. I know I showed some "per-module scoreboard" example code. I think others had a couple of ideas as well. This does not belong in the "core" scoreboard. -- Brian Akins Chief Operations Engineer Turner Digital Media Technologies
Re: memory leak in 2.2.x balancer?
Jim Jagielski wrote:
>
> On Nov 27, 2007, at 10:16 AM, jean-frederic clere wrote:
>
>> Jeff Trawick wrote:
>>> On Nov 27, 2007 8:47 AM, Plüm, Rüdiger, VF-Group
>>> <[EMAIL PROTECTED]> wrote:
>>>>
>>>>> -Ursprüngliche Nachricht-
>>>>> Von: Jeff Trawick
>>>>> Gesendet: Dienstag, 27. November 2007 14:21
>>>>> An: dev@httpd.apache.org
>>>>> Betreff: memory leak in 2.2.x balancer?
>>>>>
>>>>> looks like a leak to me; what do you think?
>>>>>
>>>>> Index: modules/proxy/mod_proxy_balancer.c
>>>>> ===
>>>>> --- modules/proxy/mod_proxy_balancer.c (revision 598305)
>>>>> +++ modules/proxy/mod_proxy_balancer.c (working copy)
>>>>> @@ -654,7 +654,7 @@
>>>>> const char *val;
>>>>> if ((val = apr_table_get(params, "ss"))) {
>>>>> if (strlen(val))
>>>>> -bsel->sticky = apr_pstrdup(conf->pool, val);
>>>>> +bsel->sticky = apr_pstrdup(r->pool, val);
>>>>> else
>>>>> bsel->sticky = NULL;
>>>>> }
>>>>>
>>>>> trunk looks much different here. Does anyone plan to backport the
>>>>> larger changes to 2.2.x in the near term, or should we go for this
>>>>> tweak?
>>>> This is no leak as if it were possible to adjust the balancer
>>>> settings via
>>>> the balancer manager their memory would need to be taken from a long
>>>> living
>>>> pool or more precise from the shared memory. This is not implemented
>>>> now.
>>>> So it makes no sense to adjust the balancer settings via the
>>>> balancer manager.
>>>> For this reason this possibility was removed from trunk. So far no
>>>> one has
>>>> found the energy to propose a clear backport of this change.
>>>> So if someone find the energy soon, fine. If not please leave
>>>> everything as it
>>>> is as we face segfaults otherwise.
>>>
>>> Thanks for the explanation. I'll try to find time to find a patch to
>>> remove this misfeature from the 2.2.x branch.
>>>
>>
>> The only thing that can be changed is what is stored in the scoreboard
>> the rest may work (threaded only) or won't work (prefork).
>>
>
> well, what happens is that it's not universal among all balancers
> and so depending on which process (and therefore which threads
> in worker) gets the request, it could be operating under
> different conditions :)
>
> It's too much work with the existing 2.2 architecture to fix
> this, but it is viable for 2.4->
One of the question is should we go on using scoreboard to store the
balancers and workers information or should we already add a layer to a
provider that will provide all the features we need to handle the
balancers and workers informations.
Cheers
Jean-Frederic
Re: memory leak in 2.2.x balancer?
On Nov 27, 2007, at 10:16 AM, jean-frederic clere wrote:
Jeff Trawick wrote:
On Nov 27, 2007 8:47 AM, Plüm, Rüdiger, VF-Group
<[EMAIL PROTECTED]> wrote:
-Ursprüngliche Nachricht-
Von: Jeff Trawick
Gesendet: Dienstag, 27. November 2007 14:21
An: dev@httpd.apache.org
Betreff: memory leak in 2.2.x balancer?
looks like a leak to me; what do you think?
Index: modules/proxy/mod_proxy_balancer.c
===
--- modules/proxy/mod_proxy_balancer.c (revision 598305)
+++ modules/proxy/mod_proxy_balancer.c (working copy)
@@ -654,7 +654,7 @@
const char *val;
if ((val = apr_table_get(params, "ss"))) {
if (strlen(val))
-bsel->sticky = apr_pstrdup(conf->pool, val);
+bsel->sticky = apr_pstrdup(r->pool, val);
else
bsel->sticky = NULL;
}
trunk looks much different here. Does anyone plan to backport the
larger changes to 2.2.x in the near term, or should we go for this
tweak?
This is no leak as if it were possible to adjust the balancer
settings via
the balancer manager their memory would need to be taken from a
long living
pool or more precise from the shared memory. This is not
implemented now.
So it makes no sense to adjust the balancer settings via the
balancer manager.
For this reason this possibility was removed from trunk. So far
no one has
found the energy to propose a clear backport of this change.
So if someone find the energy soon, fine. If not please leave
everything as it
is as we face segfaults otherwise.
Thanks for the explanation. I'll try to find time to find a patch to
remove this misfeature from the 2.2.x branch.
The only thing that can be changed is what is stored in the scoreboard
the rest may work (threaded only) or won't work (prefork).
well, what happens is that it's not universal among all balancers
and so depending on which process (and therefore which threads
in worker) gets the request, it could be operating under
different conditions :)
It's too much work with the existing 2.2 architecture to fix
this, but it is viable for 2.4->
Re: memory leak in 2.2.x balancer?
On Nov 27, 2007, at 8:20 AM, Jeff Trawick wrote:
looks like a leak to me; what do you think?
Index: modules/proxy/mod_proxy_balancer.c
===
--- modules/proxy/mod_proxy_balancer.c (revision 598305)
+++ modules/proxy/mod_proxy_balancer.c (working copy)
@@ -654,7 +654,7 @@
const char *val;
if ((val = apr_table_get(params, "ss"))) {
if (strlen(val))
-bsel->sticky = apr_pstrdup(conf->pool, val);
+bsel->sticky = apr_pstrdup(r->pool, val);
else
bsel->sticky = NULL;
}
trunk looks much different here. Does anyone plan to backport the
larger changes to 2.2.x in the near term, or should we go for this
tweak?
It's not a leak but yes all these changes do need to be
backported... I simply haven't found the cycles yet, but
we should leave the existing code as-is due to the regressions
and dumps that will arise. I'll try to propose a backport this
week (since we have time) that removes "adjusting" the
balancer as well as other low-hanging fruit changes between
trunk and 2.2
Re: memory leak in 2.2.x balancer?
Jeff Trawick wrote:
> On Nov 27, 2007 8:47 AM, Plüm, Rüdiger, VF-Group
> <[EMAIL PROTECTED]> wrote:
>>
>>> -Ursprüngliche Nachricht-
>>> Von: Jeff Trawick
>>> Gesendet: Dienstag, 27. November 2007 14:21
>>> An: dev@httpd.apache.org
>>> Betreff: memory leak in 2.2.x balancer?
>>>
>>> looks like a leak to me; what do you think?
>>>
>>> Index: modules/proxy/mod_proxy_balancer.c
>>> ===
>>> --- modules/proxy/mod_proxy_balancer.c (revision 598305)
>>> +++ modules/proxy/mod_proxy_balancer.c (working copy)
>>> @@ -654,7 +654,7 @@
>>> const char *val;
>>> if ((val = apr_table_get(params, "ss"))) {
>>> if (strlen(val))
>>> -bsel->sticky = apr_pstrdup(conf->pool, val);
>>> +bsel->sticky = apr_pstrdup(r->pool, val);
>>> else
>>> bsel->sticky = NULL;
>>> }
>>>
>>> trunk looks much different here. Does anyone plan to backport the
>>> larger changes to 2.2.x in the near term, or should we go for this
>>> tweak?
>> This is no leak as if it were possible to adjust the balancer settings via
>> the balancer manager their memory would need to be taken from a long living
>> pool or more precise from the shared memory. This is not implemented now.
>> So it makes no sense to adjust the balancer settings via the balancer
>> manager.
>> For this reason this possibility was removed from trunk. So far no one has
>> found the energy to propose a clear backport of this change.
>> So if someone find the energy soon, fine. If not please leave everything as
>> it
>> is as we face segfaults otherwise.
>
> Thanks for the explanation. I'll try to find time to find a patch to
> remove this misfeature from the 2.2.x branch.
>
The only thing that can be changed is what is stored in the scoreboard
the rest may work (threaded only) or won't work (prefork).
Cheers
Jean-Frederic
Re: memory leak in 2.2.x balancer?
On Nov 27, 2007 8:47 AM, Plüm, Rüdiger, VF-Group
<[EMAIL PROTECTED]> wrote:
>
>
> > -Ursprüngliche Nachricht-
> > Von: Jeff Trawick
> > Gesendet: Dienstag, 27. November 2007 14:21
> > An: dev@httpd.apache.org
> > Betreff: memory leak in 2.2.x balancer?
>
> >
> >
> > looks like a leak to me; what do you think?
> >
> > Index: modules/proxy/mod_proxy_balancer.c
> > ===
> > --- modules/proxy/mod_proxy_balancer.c (revision 598305)
> > +++ modules/proxy/mod_proxy_balancer.c (working copy)
> > @@ -654,7 +654,7 @@
> > const char *val;
> > if ((val = apr_table_get(params, "ss"))) {
> > if (strlen(val))
> > -bsel->sticky = apr_pstrdup(conf->pool, val);
> > +bsel->sticky = apr_pstrdup(r->pool, val);
> > else
> > bsel->sticky = NULL;
> > }
> >
> > trunk looks much different here. Does anyone plan to backport the
> > larger changes to 2.2.x in the near term, or should we go for this
> > tweak?
>
> This is no leak as if it were possible to adjust the balancer settings via
> the balancer manager their memory would need to be taken from a long living
> pool or more precise from the shared memory. This is not implemented now.
> So it makes no sense to adjust the balancer settings via the balancer manager.
> For this reason this possibility was removed from trunk. So far no one has
> found the energy to propose a clear backport of this change.
> So if someone find the energy soon, fine. If not please leave everything as it
> is as we face segfaults otherwise.
Thanks for the explanation. I'll try to find time to find a patch to
remove this misfeature from the 2.2.x branch.
Re: memory leak in 2.2.x balancer?
> -Ursprüngliche Nachricht-
> Von: Jeff Trawick
> Gesendet: Dienstag, 27. November 2007 14:21
> An: dev@httpd.apache.org
> Betreff: memory leak in 2.2.x balancer?
>
>
> looks like a leak to me; what do you think?
>
> Index: modules/proxy/mod_proxy_balancer.c
> ===
> --- modules/proxy/mod_proxy_balancer.c (revision 598305)
> +++ modules/proxy/mod_proxy_balancer.c (working copy)
> @@ -654,7 +654,7 @@
> const char *val;
> if ((val = apr_table_get(params, "ss"))) {
> if (strlen(val))
> -bsel->sticky = apr_pstrdup(conf->pool, val);
> +bsel->sticky = apr_pstrdup(r->pool, val);
> else
> bsel->sticky = NULL;
> }
>
> trunk looks much different here. Does anyone plan to backport the
> larger changes to 2.2.x in the near term, or should we go for this
> tweak?
This is no leak as if it were possible to adjust the balancer settings via
the balancer manager their memory would need to be taken from a long living
pool or more precise from the shared memory. This is not implemented now.
So it makes no sense to adjust the balancer settings via the balancer manager.
For this reason this possibility was removed from trunk. So far no one has
found the energy to propose a clear backport of this change.
So if someone find the energy soon, fine. If not please leave everything as it
is as we face segfaults otherwise.
Regards
Rüdiger
Re: memory leak in 2.2.x balancer?
Worse than a leak; conf->pool should be const, touching it explodes the
copy-on-write semantics of the conf pool memory (which otherwise is a
single common resource). There are better pools to abuse, if unavoidable,
such as process pool.
Jeff Trawick wrote:
looks like a leak to me; what do you think?
Index: modules/proxy/mod_proxy_balancer.c
===
--- modules/proxy/mod_proxy_balancer.c (revision 598305)
+++ modules/proxy/mod_proxy_balancer.c (working copy)
@@ -654,7 +654,7 @@
const char *val;
if ((val = apr_table_get(params, "ss"))) {
if (strlen(val))
-bsel->sticky = apr_pstrdup(conf->pool, val);
+bsel->sticky = apr_pstrdup(r->pool, val);
else
bsel->sticky = NULL;
}
trunk looks much different here. Does anyone plan to backport the
larger changes to 2.2.x in the near term, or should we go for this
tweak?
memory leak in 2.2.x balancer?
looks like a leak to me; what do you think?
Index: modules/proxy/mod_proxy_balancer.c
===
--- modules/proxy/mod_proxy_balancer.c (revision 598305)
+++ modules/proxy/mod_proxy_balancer.c (working copy)
@@ -654,7 +654,7 @@
const char *val;
if ((val = apr_table_get(params, "ss"))) {
if (strlen(val))
-bsel->sticky = apr_pstrdup(conf->pool, val);
+bsel->sticky = apr_pstrdup(r->pool, val);
else
bsel->sticky = NULL;
}
trunk looks much different here. Does anyone plan to backport the
larger changes to 2.2.x in the near term, or should we go for this
tweak?
