Re: [users@httpd] mod_ratelimit working by steps ?

[Luca Toscano <to...@gmail.com>]

Hi everybody,

as part of the plan to add more documentation about httpd's internals I am
trying to debug more tricky bugs (at least for me) reported by our users,
in order for example to better understand how the filters chain works in
depth.

This one caught my attention, and after a bit of testing I have some follow
up questions, if you have time let me know your thoughts :)

2018-04-19 13:47 GMT+02:00 <ne...@free.fr>:

> Hello all,
>
> I'm using Apache 2.4.24 on Debian 9 Stable, behind a DSL connection, with
> an estimated upload capacity of ~130kB/s.
> I'm trying to limit the bandwidth available to my users (per-connection
> limit is fine).
> However, it seems to me that the rate-limit parameter is coarsely grained :
>
> - if I set it to 8, users are limited to 8 kB/s
> - if I set it to 20, or 30, users are limited to 40 kB/s
> - if I set it to 50, 60 or 80, users are limited to my BW, so ~120 kB/s
>
>
After following up with the user it seems that the issue happens with
proxied content. So I've set up the following experiment:

- Directory with a 4MB file inside
- Simple Location that proxies content via mod_proxy_http to a Python
process running a webserver, capable of returning the same 4MB file
outlined above.

I tested the rate limit using curl's summary (average Dload speed for
example).

This is what I gathered:

- when httpd serves the file directly, mod_ratelimit's output filter is
called once and the bucket brigade contains all the data contained in the
file. This is probably due to how bucket brigates work when morphing a file
content?

- when httpd serves the file via mod_proxy, the output filter is called
multiple times, and each time the buckets are maximum the size of
ProxyIOBufferSize (8192 by default). Still not completely sure about this
one, so please let me know if I am totally wrong :)

The main problem is, IIUC, in the output's filter logic that does this: it
calculates the size of a chunk, based on the rate-limit set in the httpd's
conf, and then it splits the bucket brigade, if necessary, in buckets of
that chunk size, interleaving them with FLUSH buckets (and sleeping 200ms).

So a trace of execution with say a chunk size of 8192 would be something
like:

First call of the filter: 8192 --> FLUSH --> sleep(200ms) --> 8192 --> ...
-> last chunk (either 8192 or something less).

This happens correctly when httpd serves directly the content, but not when
proxied:

First call of the filter: 8192 -> FLUSH (no sleep, since do_sleep turns to
1 only after the first flush)

Second call of the filter: 8192 -> FLUSH (no sleep)

...

So one way to alleviate this issue is to move do_sleep to the ctx data
structure, so if the filter gets called multiple times it will "remember"
to sleep between flushes (with the assumption that it is allocated for each
request). It remains the problem that when the rate-limit speed sets a
chunk size greater than the ProxyIOBufferSize (8192 by default) then the
client will be rate limited to the speed dictated by the buffer size (for
example, 8192 should correspond to ~40KB/s).

Without the patch of do_sleep in ctx though, as reported by the user, after
some rate-limit there won't be any sleep anymore and hence almost no
ratelimit (only FLUSH buckets that might slow down the overall throughput).

Thanks for reading so far, I hope that what I wrote makes sense. If so, I'd
document this use case in the mod_ratelimit documentation page and possibly
submit a patch, otherwise I'll try to study more following up from your
comments :)

Luca

Re: [users@httpd] mod_ratelimit working by steps ?

[Yann Ylavic <yl...@gmail.com>]

On Mon, Jun 18, 2018 at 1:42 PM, Luca Toscano <to...@gmail.com> wrote:
>
> It was a stupid doubt but I understood a lot of things, I'll try to avoid
> spamming the mailing list the next time :D

I wish I received spams like this every day :)
Review is always welcome!

>
> Will commit your patch as soon as possible!

Well, your patch mostly, just an "optimization" on my side ;)

Re: [users@httpd] mod_ratelimit working by steps ?

[Luca Toscano <to...@gmail.com>]

Hi Yann!

2018-06-18 12:04 GMT+02:00 Yann Ylavic <yl...@gmail.com>:

> Hi Luca,
>
> On Sat, Jun 16, 2018 at 8:56 AM, Luca Toscano <to...@gmail.com>
> wrote:
> >
> > While reading the code I didn't get if one particular use case may or may
> > not happen at all, it is the only big doubt that I have before
> committing.
> > Is it possible that the first bb to process (during the first invocation
> of
> > the filter) is equal to the chunk_size + ctx->burst? IIUC the code
> relies on
> > the fact that bb is not empty to decide whether or not to pass ctx->tmpp
> to
> > the next filter in the chain, but if this is not the case then it may
> loop?
>
> I'm not sure to follow, a loop in mod_ratelimit's filter itself?
> 1/ If bb is given empty then the filter does nothing (AFAICT), this
> should not happen but it's not each filter's business to handle the
> case either (just not crash or loop indefinitely of course).
> 2/ If bb is less than chunk_size + burst bytes, then data are retained
> in holdingbb until the next call (provided not EOS/FLUSH and so
> on...).
> 3/ If bb is greater or equal to chunk_size + burst bytes (the case you
> are talking about AIUI), then each chunk is sent rate limited and
> either 1/ or 2/ applies for the rest.
>
> Which case exactly do you care about?
>

Thanks for the patience! You are of course correct, I didn't put the use
case that I had in mind into a proper context. Basically I was worried that
this may have happened:

1) brigade with something less or equal than chunk size + burst
2) apr_brigade_partition returning the brigade's sentilel
3) no EOS and bb empty, nothing passed to the next filter
4) data saved in holdingbb
5) next filter invocation, holdingbb moved to bb, back to 1) ...

While writing it down it is of course clear that a loop cannot happen,
because even if 1-4) happens, then there will be another filter invocation
that will either bring EOS or more data, that will make things go forward.

It was a stupid doubt but I understood a lot of things, I'll try to avoid
spamming the mailing list the next time :D

Will commit your patch as soon as possible!

Luca

Re: [users@httpd] mod_ratelimit working by steps ?

[Yann Ylavic <yl...@gmail.com>]

Hi Luca,

On Sat, Jun 16, 2018 at 8:56 AM, Luca Toscano <to...@gmail.com> wrote:
>
> While reading the code I didn't get if one particular use case may or may
> not happen at all, it is the only big doubt that I have before committing.
> Is it possible that the first bb to process (during the first invocation of
> the filter) is equal to the chunk_size + ctx->burst? IIUC the code relies on
> the fact that bb is not empty to decide whether or not to pass ctx->tmpp to
> the next filter in the chain, but if this is not the case then it may loop?

I'm not sure to follow, a loop in mod_ratelimit's filter itself?
1/ If bb is given empty then the filter does nothing (AFAICT), this
should not happen but it's not each filter's business to handle the
case either (just not crash or loop indefinitely of course).
2/ If bb is less than chunk_size + burst bytes, then data are retained
in holdingbb until the next call (provided not EOS/FLUSH and so
on...).
3/ If bb is greater or equal to chunk_size + burst bytes (the case you
are talking about AIUI), then each chunk is sent rate limited and
either 1/ or 2/ applies for the rest.

Which case exactly do you care about?

Regards,
Yann.

Re: [users@httpd] mod_ratelimit working by steps ?

[Luca Toscano <to...@gmail.com>]

2018-06-08 12:43 GMT+02:00 Luca Toscano <to...@gmail.com>:

> Hi Yann,
>
> 2018-06-07 12:59 GMT+02:00 Yann Ylavic <yl...@gmail.com>:
>
>> On Thu, Jun 7, 2018 at 10:17 AM, Yann Ylavic <yl...@gmail.com>
>> wrote:
>> > On Thu, Jun 7, 2018 at 9:21 AM, Yann Ylavic <yl...@gmail.com>
>> wrote:
>> >>
>> >> Feel free to take what can serve you, or burn it ;)
>> >
>> > Burn it! Here is v2, hopefully a less buggy :)
>>
>> FWIW, a v3 with less changes and a few comments where needed.
>> Less changes because it keeps using tmpbb in the RATE_LIMIT loop (with
>> APR_BRIGADE_CONCAT(ctx->tmpbb, bb) first).
>> HTH...
>>
>
> All my consistency tests are green, and after a first (and quick!) pass of
> the code in your patch I can definitely say that your version is much
> better and coincise, I like it! Will try to review it in detail during the
> next days and then I'll commit it.
>

Sorry for the delay :)

While reading the code I didn't get if one particular use case may or may
not happen at all, it is the only big doubt that I have before committing.
Is it possible that the first bb to process (during the first invocation of
the filter) is equal to the chunk_size + ctx->burst? IIUC the code relies
on the fact that bb is not empty to decide whether or not to pass ctx->tmpp
to the next filter in the chain, but if this is not the case then it may
loop?

I tried to dump the brigades of my tests (proxying content via
mod_proxy_http and directly from httpd), the response headers in the
beginning seems to always guarantee an initial bb of some bytes that make
everything work.

Not sure if this is a valuable comment, the rest looks super good :)

Thanks for the patience!

Luca

Re: [users@httpd] mod_ratelimit working by steps ?

[Luca Toscano <to...@gmail.com>]

Hi Yann,

2018-06-07 12:59 GMT+02:00 Yann Ylavic <yl...@gmail.com>:

> On Thu, Jun 7, 2018 at 10:17 AM, Yann Ylavic <yl...@gmail.com> wrote:
> > On Thu, Jun 7, 2018 at 9:21 AM, Yann Ylavic <yl...@gmail.com>
> wrote:
> >>
> >> Feel free to take what can serve you, or burn it ;)
> >
> > Burn it! Here is v2, hopefully a less buggy :)
>
> FWIW, a v3 with less changes and a few comments where needed.
> Less changes because it keeps using tmpbb in the RATE_LIMIT loop (with
> APR_BRIGADE_CONCAT(ctx->tmpbb, bb) first).
> HTH...
>

All my consistency tests are green, and after a first (and quick!) pass of
the code in your patch I can definitely say that your version is much
better and coincise, I like it! Will try to review it in detail during the
next days and then I'll commit it.

Thanks!

Luca

Re: [users@httpd] mod_ratelimit working by steps ?

[Yann Ylavic <yl...@gmail.com>]

On Thu, Jun 7, 2018 at 10:17 AM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Jun 7, 2018 at 9:21 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> Feel free to take what can serve you, or burn it ;)
>
> Burn it! Here is v2, hopefully a less buggy :)

FWIW, a v3 with less changes and a few comments where needed.
Less changes because it keeps using tmpbb in the RATE_LIMIT loop (with
APR_BRIGADE_CONCAT(ctx->tmpbb, bb) first).
HTH...

Re: [users@httpd] mod_ratelimit working by steps ?

[Yann Ylavic <yl...@gmail.com>]

On Thu, Jun 7, 2018 at 9:21 AM, Yann Ylavic <yl...@gmail.com> wrote:
>
> Feel free to take what can serve you, or burn it ;)

Burn it! Here is v2, hopefully a less buggy :)

Re: [users@httpd] mod_ratelimit working by steps ?

[Yann Ylavic <yl...@gmail.com>]

Hi Luca,

On Wed, Jun 6, 2018 at 12:35 PM, Luca Toscano <to...@gmail.com> wrote:
>
> Tried to improve the ctx->tmpbb usage adding a new ctx->buffer brigade, with
> the only duty of buffering buckets between filter's invocations (if needed):
>
> http://home.apache.org/~elukey/httpd-trunk-mod_ratelimit-rate_limit_proxied_content.patch
>
> I didn't touch holdingbb since it is already used to allow buckets to skip
> the rate limit and go full speed, even if I am not sure if this
> functionality has it ever been used. I would prefer not to touch that
> special logic and fix only this use case.

We possibly don't need another brigade, and can save the remaining
buckets at the end only (not far from your patch, see attached).
This patch (over yours) uses apr_brigade_split_ex() instead of
open-coding it, and returns errors immediately (the error logic was
buggy and this makes the code simpler).

Also as you said, all the RATE_FULLSPEED/RATE_LIMIT logic seems to be
dead code, I think we can axe it (at least in trunk) as follow up.
This would remove rl_buckets and complexity in the loop, but for 2.4.x
we probably want to keep it since it's public API (any third party
module using these buckets?).

Feel free to take what can serve you, or burn it ;)
Anyway, thanks for working on this!

Re: [users@httpd] mod_ratelimit working by steps ?

[Luca Toscano <to...@gmail.com>]

2018-06-05 8:19 GMT+02:00 Luca Toscano <to...@gmail.com>:

>
>
> 2018-06-04 16:22 GMT+02:00 Luca Toscano <to...@gmail.com>:
>
>> Hi Yann!
>>
>> 2018-06-04 15:59 GMT+02:00 Yann Ylavic <yl...@gmail.com>:
>>
>>> Hi Luca,
>>>
>>> On Mon, Jun 4, 2018 at 11:23 AM, Luca Toscano <to...@gmail.com>
>>> wrote:
>>> >
>>> > To keep archives happy: opened
>>> > https://bz.apache.org/bugzilla/show_bug.cgi?id=62362 and added a
>>> patch in
>>> > there, if anybody wants to review it and give me suggestions I'd be
>>> happy :)
>>>
>>> The semantic of tmpbb is not very clear in your patch, it's both the
>>> brigade where buckets are saved (for the next call?) and the one that
>>> gets passed to the next filter finally.
>>> It doesn't look right to me, if tmpbb is to be forwarded in the same
>>> pass there is no need to change its buckets' lifetime.
>>> Couldn't we ap_save_brigade(f, &ctx->holdingbb, &ctx->tmpbb, ..) at
>>> the end of the filter only?
>>>
>>
>> Thanks for the review, bare in mind that this is my first "big" patch so
>> I'd probably need a better grasp of the internals first :) I haven't
>> touched holdingbb since I saw that was used elsewhere (in RATE_FULLSPEED),
>> but I can try to check it as well. The idea of my patch (that I aimed to)
>> is to pass the ctx->tmbbb only if it reaches chunk_size (or EOS is seen)
>> and buffer otherwise the buckets in it using ap_save_brigade (waiting for
>> the next call to see if chunk_size is reached).
>>
>> So if I got your point correctly you would use ctx->holdingbb to store
>> the buckets (and changing their lifetime possibly) between calls, and tmpbb
>> only within the same filter invocation?
>>
>>
> After re-reading the code and I think I got your point Yann, I'll try to
> re-work my idea and create a new patch. Thanks!
>

Tried to improve the ctx->tmpbb usage adding a new ctx->buffer brigade,
with the only duty of buffering buckets between filter's invocations (if
needed):

http://home.apache.org/~elukey/httpd-trunk-mod_ratelimit-rate_limit_proxied_content.patch

I didn't touch holdingbb since it is already used to allow buckets to skip
the rate limit and go full speed, even if I am not sure if this
functionality has it ever been used. I would prefer not to touch that
special logic and fix only this use case.

I may have misunderstood Yann's suggestions, if so sorry for the spam, but
I'd be glad to get a bit more info about how a better patch should look
like :)

I promise documentation in the docs/developer section when the task is
done! (I can add a "If even Luca did this, you can as well!" section in the
docs :P)

Luca

Re: [users@httpd] mod_ratelimit working by steps ?

[Luca Toscano <to...@gmail.com>]

2018-06-04 16:22 GMT+02:00 Luca Toscano <to...@gmail.com>:

> Hi Yann!
>
> 2018-06-04 15:59 GMT+02:00 Yann Ylavic <yl...@gmail.com>:
>
>> Hi Luca,
>>
>> On Mon, Jun 4, 2018 at 11:23 AM, Luca Toscano <to...@gmail.com>
>> wrote:
>> >
>> > To keep archives happy: opened
>> > https://bz.apache.org/bugzilla/show_bug.cgi?id=62362 and added a patch
>> in
>> > there, if anybody wants to review it and give me suggestions I'd be
>> happy :)
>>
>> The semantic of tmpbb is not very clear in your patch, it's both the
>> brigade where buckets are saved (for the next call?) and the one that
>> gets passed to the next filter finally.
>> It doesn't look right to me, if tmpbb is to be forwarded in the same
>> pass there is no need to change its buckets' lifetime.
>> Couldn't we ap_save_brigade(f, &ctx->holdingbb, &ctx->tmpbb, ..) at
>> the end of the filter only?
>>
>
> Thanks for the review, bare in mind that this is my first "big" patch so
> I'd probably need a better grasp of the internals first :) I haven't
> touched holdingbb since I saw that was used elsewhere (in RATE_FULLSPEED),
> but I can try to check it as well. The idea of my patch (that I aimed to)
> is to pass the ctx->tmbbb only if it reaches chunk_size (or EOS is seen)
> and buffer otherwise the buckets in it using ap_save_brigade (waiting for
> the next call to see if chunk_size is reached).
>
> So if I got your point correctly you would use ctx->holdingbb to store the
> buckets (and changing their lifetime possibly) between calls, and tmpbb
> only within the same filter invocation?
>
>
After re-reading the code and I think I got your point Yann, I'll try to
re-work my idea and create a new patch. Thanks!

Luca

Re: [users@httpd] mod_ratelimit working by steps ?

[Luca Toscano <to...@gmail.com>]

Hi Yann!

2018-06-04 15:59 GMT+02:00 Yann Ylavic <yl...@gmail.com>:

> Hi Luca,
>
> On Mon, Jun 4, 2018 at 11:23 AM, Luca Toscano <to...@gmail.com>
> wrote:
> >
> > To keep archives happy: opened
> > https://bz.apache.org/bugzilla/show_bug.cgi?id=62362 and added a patch
> in
> > there, if anybody wants to review it and give me suggestions I'd be
> happy :)
>
> The semantic of tmpbb is not very clear in your patch, it's both the
> brigade where buckets are saved (for the next call?) and the one that
> gets passed to the next filter finally.
> It doesn't look right to me, if tmpbb is to be forwarded in the same
> pass there is no need to change its buckets' lifetime.
> Couldn't we ap_save_brigade(f, &ctx->holdingbb, &ctx->tmpbb, ..) at
> the end of the filter only?
>

Thanks for the review, bare in mind that this is my first "big" patch so
I'd probably need a better grasp of the internals first :) I haven't
touched holdingbb since I saw that was used elsewhere (in RATE_FULLSPEED),
but I can try to check it as well. The idea of my patch (that I aimed to)
is to pass the ctx->tmbbb only if it reaches chunk_size (or EOS is seen)
and buffer otherwise the buckets in it using ap_save_brigade (waiting for
the next call to see if chunk_size is reached).

So if I got your point correctly you would use ctx->holdingbb to store the
buckets (and changing their lifetime possibly) between calls, and tmpbb
only within the same filter invocation?

Luca

Re: [users@httpd] mod_ratelimit working by steps ?

[Yann Ylavic <yl...@gmail.com>]

Hi Luca,

On Mon, Jun 4, 2018 at 11:23 AM, Luca Toscano <to...@gmail.com> wrote:
>
> To keep archives happy: opened
> https://bz.apache.org/bugzilla/show_bug.cgi?id=62362 and added a patch in
> there, if anybody wants to review it and give me suggestions I'd be happy :)

The semantic of tmpbb is not very clear in your patch, it's both the
brigade where buckets are saved (for the next call?) and the one that
gets passed to the next filter finally.
It doesn't look right to me, if tmpbb is to be forwarded in the same
pass there is no need to change its buckets' lifetime.
Couldn't we ap_save_brigade(f, &ctx->holdingbb, &ctx->tmpbb, ..) at
the end of the filter only?

Thanks,
Yann.

Re: [users@httpd] mod_ratelimit working by steps ?

[Luca Toscano <to...@gmail.com>]

2018-05-04 14:46 GMT+02:00 Luca Toscano <to...@gmail.com>:

> Hi everybody,
>
> as part of the plan to add more documentation about httpd's internals I am
> trying to debug more tricky bugs (at least for me) reported by our users,
> in order for example to better understand how the filters chain works in
> depth.
>
> This one caught my attention, and after a bit of testing I have some
> follow up questions, if you have time let me know your thoughts :)
>
> 2018-04-19 13:47 GMT+02:00 <ne...@free.fr>:
>
>> Hello all,
>>
>> I'm using Apache 2.4.24 on Debian 9 Stable, behind a DSL connection, with
>> an estimated upload capacity of ~130kB/s.
>> I'm trying to limit the bandwidth available to my users (per-connection
>> limit is fine).
>> However, it seems to me that the rate-limit parameter is coarsely grained
>> :
>>
>> - if I set it to 8, users are limited to 8 kB/s
>> - if I set it to 20, or 30, users are limited to 40 kB/s
>> - if I set it to 50, 60 or 80, users are limited to my BW, so ~120 kB/s
>>
>>
> After following up with the user it seems that the issue happens with
> proxied content. So I've set up the following experiment:
>
> - Directory with a 4MB file inside
> - Simple Location that proxies content via mod_proxy_http to a Python
> process running a webserver, capable of returning the same 4MB file
> outlined above.
>
> I tested the rate limit using curl's summary (average Dload speed for
> example).
>
> This is what I gathered:
>
> - when httpd serves the file directly, mod_ratelimit's output filter is
> called once and the bucket brigade contains all the data contained in the
> file. This is probably due to how bucket brigates work when morphing a file
> content?
>
> - when httpd serves the file via mod_proxy, the output filter is called
> multiple times, and each time the buckets are maximum the size of
> ProxyIOBufferSize (8192 by default). Still not completely sure about this
> one, so please let me know if I am totally wrong :)
>
> The main problem is, IIUC, in the output's filter logic that does this: it
> calculates the size of a chunk, based on the rate-limit set in the httpd's
> conf, and then it splits the bucket brigade, if necessary, in buckets of
> that chunk size, interleaving them with FLUSH buckets (and sleeping 200ms).
>
> So a trace of execution with say a chunk size of 8192 would be something
> like:
>
> First call of the filter: 8192 --> FLUSH --> sleep(200ms) --> 8192 --> ...
> -> last chunk (either 8192 or something less).
>
> This happens correctly when httpd serves directly the content, but not
> when proxied:
>
> First call of the filter: 8192 -> FLUSH (no sleep, since do_sleep turns to
> 1 only after the first flush)
>
> Second call of the filter: 8192 -> FLUSH (no sleep)
>
> ...
>
> So one way to alleviate this issue is to move do_sleep to the ctx data
> structure, so if the filter gets called multiple times it will "remember"
> to sleep between flushes (with the assumption that it is allocated for each
> request). It remains the problem that when the rate-limit speed sets a
> chunk size greater than the ProxyIOBufferSize (8192 by default) then the
> client will be rate limited to the speed dictated by the buffer size (for
> example, 8192 should correspond to ~40KB/s).
>
> Without the patch of do_sleep in ctx though, as reported by the user,
> after some rate-limit there won't be any sleep anymore and hence almost no
> ratelimit (only FLUSH buckets that might slow down the overall throughput).
>
> Thanks for reading so far, I hope that what I wrote makes sense. If so,
> I'd document this use case in the mod_ratelimit documentation page and
> possibly submit a patch, otherwise I'll try to study more following up from
> your comments :)
>
>
>
To keep archives happy: opened
https://bz.apache.org/bugzilla/show_bug.cgi?id=62362 and added a patch in
there, if anybody wants to review it and give me suggestions I'd be happy :)

Thanks!

Luca