Re: [users@httpd] mod_ratelimit working by steps ?
On Mon, Jun 18, 2018 at 1:42 PM, Luca Toscano 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 ?
Hi Yann! 2018-06-18 12:04 GMT+02:00 Yann Ylavic : > Hi Luca, > > On Sat, Jun 16, 2018 at 8:56 AM, Luca Toscano > 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 ?
Hi Luca, On Sat, Jun 16, 2018 at 8:56 AM, Luca Toscano 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 ?
2018-06-08 12:43 GMT+02:00 Luca Toscano : > Hi Yann, > > 2018-06-07 12:59 GMT+02:00 Yann Ylavic : > >> On Thu, Jun 7, 2018 at 10:17 AM, Yann Ylavic >> wrote: >> > On Thu, Jun 7, 2018 at 9:21 AM, Yann Ylavic >> 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 ?
Hi Yann, 2018-06-07 12:59 GMT+02:00 Yann Ylavic : > On Thu, Jun 7, 2018 at 10:17 AM, Yann Ylavic wrote: > > On Thu, Jun 7, 2018 at 9:21 AM, Yann Ylavic > 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 ?
On Thu, Jun 7, 2018 at 10:17 AM, Yann Ylavic wrote: > On Thu, Jun 7, 2018 at 9:21 AM, Yann Ylavic 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... Index: modules/filters/mod_ratelimit.c === --- modules/filters/mod_ratelimit.c (revision 1833084) +++ modules/filters/mod_ratelimit.c (working copy) @@ -26,7 +26,6 @@ typedef enum rl_state_e { -RATE_ERROR, RATE_LIMIT, RATE_FULLSPEED } rl_state_e; @@ -36,6 +35,7 @@ typedef struct rl_ctx_t int speed; int chunk_size; int burst; +int do_sleep; rl_state_e state; apr_bucket_brigade *tmpbb; apr_bucket_brigade *holdingbb; @@ -57,21 +57,12 @@ static void brigade_dump(request_rec *r, apr_bucke #endif /* RLFDEBUG */ static apr_status_t -rate_limit_filter(ap_filter_t *f, apr_bucket_brigade *input_bb) +rate_limit_filter(ap_filter_t *f, apr_bucket_brigade *bb) { apr_status_t rv = APR_SUCCESS; rl_ctx_t *ctx = f->ctx; -apr_bucket *fb; -int do_sleep = 0; apr_bucket_alloc_t *ba = f->r->connection->bucket_alloc; -apr_bucket_brigade *bb = input_bb; -if (f->c->aborted) { -ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, f->r, APLOGNO(01454) "rl: conn aborted"); -apr_brigade_cleanup(bb); -return APR_ECONNABORTED; -} - /* Set up our rl_ctx_t on first use */ if (ctx == NULL) { @@ -120,6 +111,7 @@ static apr_status_t ctx->state = RATE_LIMIT; ctx->speed = ratelimit; ctx->burst = burst; +ctx->do_sleep = 0; /* calculate how many bytes / interval we want to send */ /* speed is bytes / second, so, how many (speed / 1000 % interval) */ @@ -127,56 +119,40 @@ static apr_status_t ctx->tmpbb = apr_brigade_create(f->r->pool, ba); ctx->holdingbb = apr_brigade_create(f->r->pool, ba); } +else { +APR_BRIGADE_PREPEND(bb, ctx->holdingbb); +} -while (ctx->state != RATE_ERROR && - (!APR_BRIGADE_EMPTY(bb) || !APR_BRIGADE_EMPTY(ctx->holdingbb))) { +while (!APR_BRIGADE_EMPTY(bb)) { apr_bucket *e; -if (!APR_BRIGADE_EMPTY(ctx->holdingbb)) { -APR_BRIGADE_CONCAT(bb, ctx->holdingbb); -} - -while (ctx->state == RATE_FULLSPEED && !APR_BRIGADE_EMPTY(bb)) { +if (ctx->state == RATE_FULLSPEED) { /* Find where we 'stop' going full speed. */ for (e = APR_BRIGADE_FIRST(bb); e != APR_BRIGADE_SENTINEL(bb); e = APR_BUCKET_NEXT(e)) { if (AP_RL_BUCKET_IS_END(e)) { -apr_bucket *f; -f = APR_RING_LAST(&bb->list); -APR_RING_UNSPLICE(e, f, link); -APR_RING_SPLICE_TAIL(&ctx->holdingbb->list, e, f, - apr_bucket, link); +apr_brigade_split_ex(bb, e, ctx->holdingbb); ctx->state = RATE_LIMIT; break; } } -if (f->c->aborted) { -apr_brigade_cleanup(bb); -ctx->state = RATE_ERROR; -break; -} - -fb = apr_bucket_flush_create(ba); -APR_BRIGADE_INSERT_TAIL(bb, fb); +e = apr_bucket_flush_create(ba); +APR_BRIGADE_INSERT_TAIL(bb, e); rv = ap_pass_brigade(f->next, bb); +apr_brigade_cleanup(bb); if (rv != APR_SUCCESS) { -ctx->state = RATE_ERROR; ap_log_rerror(APLOG_MARK, APLOG_TRACE1, rv, f->r, APLOGNO(01455) "rl: full speed brigade pass failed."); +return rv; } } - -while (ctx->state == RATE_LIMIT && !APR_BRIGADE_EMPTY(bb)) { +else { for (e = APR_BRIGADE_FIRST(bb); e != APR_BRIGADE_SENTINEL(bb); e = APR_BUCKET_NEXT(e)) { if (AP_RL_BUCKET_IS_START(e)) { -apr_bucket *f; -f = APR_RING_LAST(&bb->list); -APR_RING_UNSPLICE(e, f, link); -APR_RING_SPLICE_TAIL(&ctx->holdingbb->list, e, f, - apr_bucket, link); +apr_brigade_split_ex(bb, e, ctx->holdingbb); ctx->state = RATE_FULLSPEED; break; } @@ -183,24 +159,10 @@ static apr_status_t } while (!APR_BRIGADE_EMPTY(bb)) { -apr_bucket *stop_point; -apr_off_t len = 0; +apr_off_t len = ctx->chunk_size + ctx->burst; -if (f->c->aborted) { -
Re: [users@httpd] mod_ratelimit working by steps ?
On Thu, Jun 7, 2018 at 9:21 AM, Yann Ylavic wrote: > > Feel free to take what can serve you, or burn it ;) Burn it! Here is v2, hopefully a less buggy :) Index: modules/filters/mod_ratelimit.c === --- modules/filters/mod_ratelimit.c (revision 1833084) +++ modules/filters/mod_ratelimit.c (working copy) @@ -26,7 +26,6 @@ typedef enum rl_state_e { -RATE_ERROR, RATE_LIMIT, RATE_FULLSPEED } rl_state_e; @@ -36,6 +35,7 @@ typedef struct rl_ctx_t int speed; int chunk_size; int burst; +int do_sleep; rl_state_e state; apr_bucket_brigade *tmpbb; apr_bucket_brigade *holdingbb; @@ -57,21 +57,12 @@ static void brigade_dump(request_rec *r, apr_bucke #endif /* RLFDEBUG */ static apr_status_t -rate_limit_filter(ap_filter_t *f, apr_bucket_brigade *input_bb) +rate_limit_filter(ap_filter_t *f, apr_bucket_brigade *bb) { apr_status_t rv = APR_SUCCESS; rl_ctx_t *ctx = f->ctx; -apr_bucket *fb; -int do_sleep = 0; apr_bucket_alloc_t *ba = f->r->connection->bucket_alloc; -apr_bucket_brigade *bb = input_bb; -if (f->c->aborted) { -ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, f->r, APLOGNO(01454) "rl: conn aborted"); -apr_brigade_cleanup(bb); -return APR_ECONNABORTED; -} - /* Set up our rl_ctx_t on first use */ if (ctx == NULL) { @@ -120,6 +111,7 @@ static apr_status_t ctx->state = RATE_LIMIT; ctx->speed = ratelimit; ctx->burst = burst; +ctx->do_sleep = 0; /* calculate how many bytes / interval we want to send */ /* speed is bytes / second, so, how many (speed / 1000 % interval) */ @@ -127,56 +119,40 @@ static apr_status_t ctx->tmpbb = apr_brigade_create(f->r->pool, ba); ctx->holdingbb = apr_brigade_create(f->r->pool, ba); } +else { +APR_BRIGADE_PREPEND(bb, ctx->holdingbb); +} -while (ctx->state != RATE_ERROR && - (!APR_BRIGADE_EMPTY(bb) || !APR_BRIGADE_EMPTY(ctx->holdingbb))) { +while (!APR_BRIGADE_EMPTY(bb)) { apr_bucket *e; -if (!APR_BRIGADE_EMPTY(ctx->holdingbb)) { -APR_BRIGADE_CONCAT(bb, ctx->holdingbb); -} - -while (ctx->state == RATE_FULLSPEED && !APR_BRIGADE_EMPTY(bb)) { +if (ctx->state == RATE_FULLSPEED) { /* Find where we 'stop' going full speed. */ for (e = APR_BRIGADE_FIRST(bb); e != APR_BRIGADE_SENTINEL(bb); e = APR_BUCKET_NEXT(e)) { if (AP_RL_BUCKET_IS_END(e)) { -apr_bucket *f; -f = APR_RING_LAST(&bb->list); -APR_RING_UNSPLICE(e, f, link); -APR_RING_SPLICE_TAIL(&ctx->holdingbb->list, e, f, - apr_bucket, link); +apr_brigade_split_ex(bb, e, ctx->holdingbb); ctx->state = RATE_LIMIT; break; } } -if (f->c->aborted) { -apr_brigade_cleanup(bb); -ctx->state = RATE_ERROR; -break; -} - -fb = apr_bucket_flush_create(ba); -APR_BRIGADE_INSERT_TAIL(bb, fb); +e = apr_bucket_flush_create(ba); +APR_BRIGADE_INSERT_TAIL(bb, e); rv = ap_pass_brigade(f->next, bb); +apr_brigade_cleanup(bb); if (rv != APR_SUCCESS) { -ctx->state = RATE_ERROR; ap_log_rerror(APLOG_MARK, APLOG_TRACE1, rv, f->r, APLOGNO(01455) "rl: full speed brigade pass failed."); +return rv; } } - -while (ctx->state == RATE_LIMIT && !APR_BRIGADE_EMPTY(bb)) { +else { for (e = APR_BRIGADE_FIRST(bb); e != APR_BRIGADE_SENTINEL(bb); e = APR_BUCKET_NEXT(e)) { if (AP_RL_BUCKET_IS_START(e)) { -apr_bucket *f; -f = APR_RING_LAST(&bb->list); -APR_RING_UNSPLICE(e, f, link); -APR_RING_SPLICE_TAIL(&ctx->holdingbb->list, e, f, - apr_bucket, link); +apr_brigade_split_ex(bb, e, ctx->holdingbb); ctx->state = RATE_FULLSPEED; break; } @@ -183,24 +159,8 @@ static apr_status_t } while (!APR_BRIGADE_EMPTY(bb)) { -apr_bucket *stop_point; -apr_off_t len = 0; +apr_off_t len = ctx->chunk_size + ctx->burst; -if (f->c->aborted) { -apr_brigade_cleanup(bb); -ctx->state = RATE_ERROR; -break; -} - -if (do_sleep) { -apr_sleep(RATE_INTERVAL_MS * 1000); -} -
Re: [users@httpd] mod_ratelimit working by steps ?
Hi Luca, On Wed, Jun 6, 2018 at 12:35 PM, Luca Toscano 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! Index: modules/filters/mod_ratelimit.c === --- modules/filters/mod_ratelimit.c (revision 1833084) +++ modules/filters/mod_ratelimit.c (working copy) @@ -26,7 +26,6 @@ typedef enum rl_state_e { -RATE_ERROR, RATE_LIMIT, RATE_FULLSPEED } rl_state_e; @@ -36,6 +35,7 @@ typedef struct rl_ctx_t int speed; int chunk_size; int burst; +int do_sleep; rl_state_e state; apr_bucket_brigade *tmpbb; apr_bucket_brigade *holdingbb; @@ -57,21 +57,12 @@ static void brigade_dump(request_rec *r, apr_bucke #endif /* RLFDEBUG */ static apr_status_t -rate_limit_filter(ap_filter_t *f, apr_bucket_brigade *input_bb) +rate_limit_filter(ap_filter_t *f, apr_bucket_brigade *bb) { apr_status_t rv = APR_SUCCESS; rl_ctx_t *ctx = f->ctx; -apr_bucket *fb; -int do_sleep = 0; apr_bucket_alloc_t *ba = f->r->connection->bucket_alloc; -apr_bucket_brigade *bb = input_bb; -if (f->c->aborted) { -ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, f->r, APLOGNO(01454) "rl: conn aborted"); -apr_brigade_cleanup(bb); -return APR_ECONNABORTED; -} - /* Set up our rl_ctx_t on first use */ if (ctx == NULL) { @@ -120,6 +111,7 @@ static apr_status_t ctx->state = RATE_LIMIT; ctx->speed = ratelimit; ctx->burst = burst; +ctx->do_sleep = 0; /* calculate how many bytes / interval we want to send */ /* speed is bytes / second, so, how many (speed / 1000 % interval) */ @@ -127,56 +119,40 @@ static apr_status_t ctx->tmpbb = apr_brigade_create(f->r->pool, ba); ctx->holdingbb = apr_brigade_create(f->r->pool, ba); } +else { +APR_BRIGADE_PREPEND(bb, ctx->holdingbb); +} -while (ctx->state != RATE_ERROR && - (!APR_BRIGADE_EMPTY(bb) || !APR_BRIGADE_EMPTY(ctx->holdingbb))) { +while (!APR_BRIGADE_EMPTY(bb)) { apr_bucket *e; -if (!APR_BRIGADE_EMPTY(ctx->holdingbb)) { -APR_BRIGADE_CONCAT(bb, ctx->holdingbb); -} - -while (ctx->state == RATE_FULLSPEED && !APR_BRIGADE_EMPTY(bb)) { +if (ctx->state == RATE_FULLSPEED) { /* Find where we 'stop' going full speed. */ for (e = APR_BRIGADE_FIRST(bb); e != APR_BRIGADE_SENTINEL(bb); e = APR_BUCKET_NEXT(e)) { if (AP_RL_BUCKET_IS_END(e)) { -apr_bucket *f; -f = APR_RING_LAST(&bb->list); -APR_RING_UNSPLICE(e, f, link); -APR_RING_SPLICE_TAIL(&ctx->holdingbb->list, e, f, - apr_bucket, link); +apr_brigade_split_ex(bb, e, ctx->holdingbb); ctx->state = RATE_LIMIT; break; } } -if (f->c->aborted) { -apr_brigade_cleanup(bb); -ctx->state = RATE_ERROR; -break; -} - -fb = apr_bucket_flush_create(ba); -APR_BRIGADE_INSERT_TAIL(bb, fb); +e = apr_bucket_flush_create(ba); +APR_BRIGADE_INSERT_TAIL(bb, e); rv = ap_pass_brigade(f->next, bb); +apr_brigade_cleanup(bb); if (rv != APR_SUCCESS) { -ctx->state = RATE_ERROR; ap_log_rerror(APLOG_MARK, APLOG_TRACE1, rv, f->r, APLOGNO(01455) "rl: full speed brigade pass failed."); +return rv; } } - -while (ctx->state == RATE_LIMIT && !APR_BRIGADE_EMPTY(bb)) { +else { for
Re: [users@httpd] mod_ratelimit working by steps ?
2018-06-05 8:19 GMT+02:00 Luca Toscano : > > > 2018-06-04 16:22 GMT+02:00 Luca Toscano : > >> Hi Yann! >> >> 2018-06-04 15:59 GMT+02:00 Yann Ylavic : >> >>> Hi Luca, >>> >>> On Mon, Jun 4, 2018 at 11:23 AM, Luca Toscano >>> 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 ?
2018-06-04 16:22 GMT+02:00 Luca Toscano : > Hi Yann! > > 2018-06-04 15:59 GMT+02:00 Yann Ylavic : > >> Hi Luca, >> >> On Mon, Jun 4, 2018 at 11:23 AM, Luca Toscano >> 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 ?
Hi Yann! 2018-06-04 15:59 GMT+02:00 Yann Ylavic : > Hi Luca, > > On Mon, Jun 4, 2018 at 11:23 AM, Luca Toscano > 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 ?
Hi Luca, On Mon, Jun 4, 2018 at 11:23 AM, Luca Toscano 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 ?
2018-05-04 14:46 GMT+02:00 Luca Toscano : > 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 : > >> 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
Re: [users@httpd] mod_ratelimit working by steps ?
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 : > 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