* [PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance @ 2020-04-09 17:09 Jan Kara 2020-04-09 17:09 ` [PATCH 1/2] bfq: Fix check detecting whether waker queue should be selected Jan Kara ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Jan Kara @ 2020-04-09 17:09 UTC (permalink / raw) To: Paolo Valente; +Cc: linux-block, Andreas Herrmann, Jan Kara Hello, I was investigating why dbench performance (even for single dbench client) with BFQ is significantly worse than it used to be CFQ. The culprit is the idling logic in BFQ. The dbench workload is very fsync(2) heavy. In practice the time to complete fsync(2) calls is what determines the overall performance. For filesystems with a journal such as xfs or ext4 it is common that fsync(2) involves writing data from the process runningg fsync(2) - dbench in this case - and then waiting for the journalling machinery to flush out metadata from a separate process (jbd2 process in ext4 case, worker thread in xfs case). CFQ's heuristic was able to determine that it isn't worth to idle waiting for either dbench or jbd2 IO. BFQ's heuristic is not able to determine this and thus jbd2 process is often blocked waiting for idle timer of dbench queue to trigger. The first patch in the series is an obvious bugfix but is not enough to improve performance. The second patch does improve dbench performance from ~80 MB/s to ~200 MB/s on my test machine but I'm aware that it is probably way too aggressive and probably a different solution is needed. So I just wrote that patch to see the results and spark some discussion :). Any idea how to improve the waker logic so that dbench performance doesn't drop so dramatically? Honza ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] bfq: Fix check detecting whether waker queue should be selected 2020-04-09 17:09 [PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance Jan Kara @ 2020-04-09 17:09 ` Jan Kara 2021-01-10 9:20 ` Paolo Valente 2020-04-09 17:09 ` [PATCH 2/2] bfq: Allow short_ttime queues to have waker Jan Kara 2020-04-17 7:47 ` [PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance Paolo Valente 2 siblings, 1 reply; 10+ messages in thread From: Jan Kara @ 2020-04-09 17:09 UTC (permalink / raw) To: Paolo Valente; +Cc: linux-block, Andreas Herrmann, Jan Kara The check in bfq_select_queue() checking whether a waker queue should be selected has a bug and is checking bfqq->next_rq instead of bfqq->waker_bfqq->next_rq to verify whether the waker queue has a request to dispatch. This often results in the condition being false (most notably when the current queue is idling waiting for next request) and thus the waker queue logic is ineffective. Fix the condition. Signed-off-by: Jan Kara <jack@suse.cz> --- block/bfq-iosched.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 78ba57efd16b..18f85d474c9c 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -4498,7 +4498,7 @@ static struct bfq_queue *bfq_select_queue(struct bfq_data *bfqd) bfqq = bfqq->bic->bfqq[0]; else if (bfq_bfqq_has_waker(bfqq) && bfq_bfqq_busy(bfqq->waker_bfqq) && - bfqq->next_rq && + bfqq->waker_bfqq->next_rq && bfq_serv_to_charge(bfqq->waker_bfqq->next_rq, bfqq->waker_bfqq) <= bfq_bfqq_budget_left(bfqq->waker_bfqq) -- 2.16.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] bfq: Fix check detecting whether waker queue should be selected 2020-04-09 17:09 ` [PATCH 1/2] bfq: Fix check detecting whether waker queue should be selected Jan Kara @ 2021-01-10 9:20 ` Paolo Valente 2021-01-13 13:09 ` Jan Kara 0 siblings, 1 reply; 10+ messages in thread From: Paolo Valente @ 2021-01-10 9:20 UTC (permalink / raw) To: Jan Kara; +Cc: linux-block, Andreas Herrmann > Il giorno 9 apr 2020, alle ore 19:09, Jan Kara <jack@suse.cz> ha scritto: > > The check in bfq_select_queue() checking whether a waker queue should be > selected has a bug and is checking bfqq->next_rq instead of > bfqq->waker_bfqq->next_rq to verify whether the waker queue has a > request to dispatch. This often results in the condition being false > (most notably when the current queue is idling waiting for next request) > and thus the waker queue logic is ineffective. Fix the condition. > Hi Jan, my huge delay causes problems again, because a student of mine already made this same fix a few months ago. But I did not send it out yet, for lack of time. If ok for you, we could go for a common commit with two authors (I seem to remember it is feasible). Thanks. Paolo > Signed-off-by: Jan Kara <jack@suse.cz> > --- > block/bfq-iosched.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index 78ba57efd16b..18f85d474c9c 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -4498,7 +4498,7 @@ static struct bfq_queue *bfq_select_queue(struct bfq_data *bfqd) > bfqq = bfqq->bic->bfqq[0]; > else if (bfq_bfqq_has_waker(bfqq) && > bfq_bfqq_busy(bfqq->waker_bfqq) && > - bfqq->next_rq && > + bfqq->waker_bfqq->next_rq && > bfq_serv_to_charge(bfqq->waker_bfqq->next_rq, > bfqq->waker_bfqq) <= > bfq_bfqq_budget_left(bfqq->waker_bfqq) > -- > 2.16.4 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] bfq: Fix check detecting whether waker queue should be selected 2021-01-10 9:20 ` Paolo Valente @ 2021-01-13 13:09 ` Jan Kara 2021-01-13 13:11 ` Jan Kara 0 siblings, 1 reply; 10+ messages in thread From: Jan Kara @ 2021-01-13 13:09 UTC (permalink / raw) To: Paolo Valente; +Cc: Jan Kara, linux-block, Andreas Herrmann On Sun 10-01-21 10:20:22, Paolo Valente wrote: > > > > Il giorno 9 apr 2020, alle ore 19:09, Jan Kara <jack@suse.cz> ha scritto: > > > > The check in bfq_select_queue() checking whether a waker queue should be > > selected has a bug and is checking bfqq->next_rq instead of > > bfqq->waker_bfqq->next_rq to verify whether the waker queue has a > > request to dispatch. This often results in the condition being false > > (most notably when the current queue is idling waiting for next request) > > and thus the waker queue logic is ineffective. Fix the condition. > > > > Hi Jan, > my huge delay causes problems again, because a student of mine already > made this same fix a few months ago. But I did not send it out yet, > for lack of time. If ok for you, we could go for a common commit with > two authors (I seem to remember it is feasible). No problem for me. Or just give the student a credit instead of me. A commit in the kernel is likely more interesting for him than for me ;) Just reply to the v2 series I've sent today (you should be on CC) so that Jens knows the author should be changed. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] bfq: Fix check detecting whether waker queue should be selected 2021-01-13 13:09 ` Jan Kara @ 2021-01-13 13:11 ` Jan Kara 0 siblings, 0 replies; 10+ messages in thread From: Jan Kara @ 2021-01-13 13:11 UTC (permalink / raw) To: Paolo Valente; +Cc: Jan Kara, linux-block, Andreas Herrmann On Wed 13-01-21 14:09:33, Jan Kara wrote: > On Sun 10-01-21 10:20:22, Paolo Valente wrote: > > > > > > > Il giorno 9 apr 2020, alle ore 19:09, Jan Kara <jack@suse.cz> ha scritto: > > > > > > The check in bfq_select_queue() checking whether a waker queue should be > > > selected has a bug and is checking bfqq->next_rq instead of > > > bfqq->waker_bfqq->next_rq to verify whether the waker queue has a > > > request to dispatch. This often results in the condition being false > > > (most notably when the current queue is idling waiting for next request) > > > and thus the waker queue logic is ineffective. Fix the condition. > > > > > > > Hi Jan, > > my huge delay causes problems again, because a student of mine already > > made this same fix a few months ago. But I did not send it out yet, > > for lack of time. If ok for you, we could go for a common commit with > > two authors (I seem to remember it is feasible). > > No problem for me. Or just give the student a credit instead of me. A > commit in the kernel is likely more interesting for him than for me ;) Just > reply to the v2 series I've sent today (you should be on CC) so that Jens > knows the author should be changed. Oh, this is a different patch than I thought. I didn't resubmit this one. Nevertheless "I don't care" still holds :). Just submit whatever you find fit. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] bfq: Allow short_ttime queues to have waker 2020-04-09 17:09 [PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance Jan Kara 2020-04-09 17:09 ` [PATCH 1/2] bfq: Fix check detecting whether waker queue should be selected Jan Kara @ 2020-04-09 17:09 ` Jan Kara 2021-01-10 9:20 ` Paolo Valente 2020-04-17 7:47 ` [PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance Paolo Valente 2 siblings, 1 reply; 10+ messages in thread From: Jan Kara @ 2020-04-09 17:09 UTC (permalink / raw) To: Paolo Valente; +Cc: linux-block, Andreas Herrmann, Jan Kara Currently queues that have average think time shorter than slice_idle cannot have waker. However this requirement is too strict. E.g. dbench process always submits a one or two IOs (which is enough to pull its average think time below slice_idle) and then blocks waiting for jbd2 thread to commit a transaction. Due to idling logic jbd2 thread is often forced to wait for dbench's idle timer to trigger to be able to submit its IO and this severely delays the overall benchmark progress. E.g. on my test machine current dbench single-thread throughput is ~80 MB/s, with this patch it is ~200 MB/s. Signed-off-by: Jan Kara <jack@suse.cz> --- block/bfq-iosched.c | 1 - 1 file changed, 1 deletion(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 18f85d474c9c..416473ba80c8 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -1928,7 +1928,6 @@ static void bfq_add_request(struct request *rq) * I/O-plugging interval for bfqq. */ if (bfqd->last_completed_rq_bfqq && - !bfq_bfqq_has_short_ttime(bfqq) && ktime_get_ns() - bfqd->last_completion < 200 * NSEC_PER_USEC) { if (bfqd->last_completed_rq_bfqq != bfqq && -- 2.16.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] bfq: Allow short_ttime queues to have waker 2020-04-09 17:09 ` [PATCH 2/2] bfq: Allow short_ttime queues to have waker Jan Kara @ 2021-01-10 9:20 ` Paolo Valente 2021-01-13 13:25 ` Jan Kara 0 siblings, 1 reply; 10+ messages in thread From: Paolo Valente @ 2021-01-10 9:20 UTC (permalink / raw) To: Jan Kara; +Cc: linux-block, Andreas Herrmann > Il giorno 9 apr 2020, alle ore 19:09, Jan Kara <jack@suse.cz> ha scritto: > > Currently queues that have average think time shorter than slice_idle > cannot have waker. However this requirement is too strict. E.g. dbench > process always submits a one or two IOs (which is enough to pull its > average think time below slice_idle) and then blocks waiting for jbd2 > thread to commit a transaction. Due to idling logic jbd2 thread is > often forced to wait for dbench's idle timer to trigger to be able to > submit its IO and this severely delays the overall benchmark progress. > > E.g. on my test machine current dbench single-thread throughput is ~80 > MB/s, with this patch it is ~200 MB/s. > Hi Jan, I've modified this logic a little bit (in patches that I'm going to submit). And I don't see your boost in my tests. So it's difficult for me to validate this change. If ok for you, you could test it on top of the patches that I'll submit. If you see a boost, and (as I expect) I won't see any regression, this improvement is very welcome for me. Thanks, Paolo > Signed-off-by: Jan Kara <jack@suse.cz> > --- > block/bfq-iosched.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index 18f85d474c9c..416473ba80c8 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -1928,7 +1928,6 @@ static void bfq_add_request(struct request *rq) > * I/O-plugging interval for bfqq. > */ > if (bfqd->last_completed_rq_bfqq && > - !bfq_bfqq_has_short_ttime(bfqq) && > ktime_get_ns() - bfqd->last_completion < > 200 * NSEC_PER_USEC) { > if (bfqd->last_completed_rq_bfqq != bfqq && > -- > 2.16.4 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] bfq: Allow short_ttime queues to have waker 2021-01-10 9:20 ` Paolo Valente @ 2021-01-13 13:25 ` Jan Kara 0 siblings, 0 replies; 10+ messages in thread From: Jan Kara @ 2021-01-13 13:25 UTC (permalink / raw) To: Paolo Valente; +Cc: Jan Kara, linux-block, Andreas Herrmann On Sun 10-01-21 10:20:19, Paolo Valente wrote: > > > > Il giorno 9 apr 2020, alle ore 19:09, Jan Kara <jack@suse.cz> ha scritto: > > > > Currently queues that have average think time shorter than slice_idle > > cannot have waker. However this requirement is too strict. E.g. dbench > > process always submits a one or two IOs (which is enough to pull its > > average think time below slice_idle) and then blocks waiting for jbd2 > > thread to commit a transaction. Due to idling logic jbd2 thread is > > often forced to wait for dbench's idle timer to trigger to be able to > > submit its IO and this severely delays the overall benchmark progress. > > > > E.g. on my test machine current dbench single-thread throughput is ~80 > > MB/s, with this patch it is ~200 MB/s. > > > > Hi Jan, > I've modified this logic a little bit (in patches that I'm going to > submit). And I don't see your boost in my tests. So it's difficult > for me to validate this change. If ok for you, you could test it on > top of the patches that I'll submit. If you see a boost, and (as I > expect) I won't see any regression, this improvement is very welcome > for me. As I wrote in the cover letter, I'm not really convinced that patch conceptually makes sence. What you later implemented should be definitely a more sophisticated approach to the problem. So I'd just discard this patch. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance 2020-04-09 17:09 [PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance Jan Kara 2020-04-09 17:09 ` [PATCH 1/2] bfq: Fix check detecting whether waker queue should be selected Jan Kara 2020-04-09 17:09 ` [PATCH 2/2] bfq: Allow short_ttime queues to have waker Jan Kara @ 2020-04-17 7:47 ` Paolo Valente 2020-04-17 9:18 ` Jan Kara 2 siblings, 1 reply; 10+ messages in thread From: Paolo Valente @ 2020-04-17 7:47 UTC (permalink / raw) To: Jan Kara; +Cc: linux-block, Andreas Herrmann Hi Jan, I'm glad you're addressing these BFQ issues. Sorry for the delay, but I happen to be working on similar issues, for other sort of corner-case workloads. So I wanted to consolidate my changes before replying. Probably, the best first step for me, to check your proposal, is to merge it with my current changes, and test the outcome. In this respect, my problem is that, after our last improvements for dbench, we cannot reproduce regressions any longer. So, we would need your support to test both issues, i.e., to test throughput with dbench (on your side/machines), and possible other regressions of your and my changes (on our side/machines). Would it be ok for you to participate in this little collaboration? If it is, then I'll contact you privately to kick this off. Thanks, Paolo > Il giorno 9 apr 2020, alle ore 19:09, Jan Kara <jack@suse.cz> ha scritto: > > Hello, > > I was investigating why dbench performance (even for single dbench client) with > BFQ is significantly worse than it used to be CFQ. The culprit is the idling > logic in BFQ. The dbench workload is very fsync(2) heavy. In practice the time > to complete fsync(2) calls is what determines the overall performance. For > filesystems with a journal such as xfs or ext4 it is common that fsync(2) > involves writing data from the process runningg fsync(2) - dbench in this case > - and then waiting for the journalling machinery to flush out metadata from a > separate process (jbd2 process in ext4 case, worker thread in xfs case). > CFQ's heuristic was able to determine that it isn't worth to idle waiting for > either dbench or jbd2 IO. BFQ's heuristic is not able to determine this and > thus jbd2 process is often blocked waiting for idle timer of dbench queue to > trigger. > > The first patch in the series is an obvious bugfix but is not enough to improve > performance. The second patch does improve dbench performance from ~80 MB/s > to ~200 MB/s on my test machine but I'm aware that it is probably way too > aggressive and probably a different solution is needed. So I just wrote that > patch to see the results and spark some discussion :). Any idea how to > improve the waker logic so that dbench performance doesn't drop so > dramatically? > > Honza ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance 2020-04-17 7:47 ` [PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance Paolo Valente @ 2020-04-17 9:18 ` Jan Kara 0 siblings, 0 replies; 10+ messages in thread From: Jan Kara @ 2020-04-17 9:18 UTC (permalink / raw) To: Paolo Valente; +Cc: Jan Kara, linux-block, Andreas Herrmann Hi Paolo! On Fri 17-04-20 09:47:07, Paolo Valente wrote: > I'm glad you're addressing these BFQ issues. Sorry for the delay, but > I happen to be working on similar issues, for other sort of > corner-case workloads. So I wanted to consolidate my changes before > replying. > > Probably, the best first step for me, to check your proposal, is to > merge it with my current changes, and test the outcome. In this > respect, my problem is that, after our last improvements for dbench, > we cannot reproduce regressions any longer. So, we would need your > support to test both issues, i.e., to test throughput with dbench (on > your side/machines), and possible other regressions of your and my > changes (on our side/machines). > > Would it be ok for you to participate in this little collaboration? > If it is, then I'll contact you privately to kick this off. Sure, I can test whatever patches you send me on our machines. Honza > > Thanks, > Paolo > > > Il giorno 9 apr 2020, alle ore 19:09, Jan Kara <jack@suse.cz> ha scritto: > > > > Hello, > > > > I was investigating why dbench performance (even for single dbench client) with > > BFQ is significantly worse than it used to be CFQ. The culprit is the idling > > logic in BFQ. The dbench workload is very fsync(2) heavy. In practice the time > > to complete fsync(2) calls is what determines the overall performance. For > > filesystems with a journal such as xfs or ext4 it is common that fsync(2) > > involves writing data from the process runningg fsync(2) - dbench in this case > > - and then waiting for the journalling machinery to flush out metadata from a > > separate process (jbd2 process in ext4 case, worker thread in xfs case). > > CFQ's heuristic was able to determine that it isn't worth to idle waiting for > > either dbench or jbd2 IO. BFQ's heuristic is not able to determine this and > > thus jbd2 process is often blocked waiting for idle timer of dbench queue to > > trigger. > > > > The first patch in the series is an obvious bugfix but is not enough to improve > > performance. The second patch does improve dbench performance from ~80 MB/s > > to ~200 MB/s on my test machine but I'm aware that it is probably way too > > aggressive and probably a different solution is needed. So I just wrote that > > patch to see the results and spark some discussion :). Any idea how to > > improve the waker logic so that dbench performance doesn't drop so > > dramatically? > > > > Honza > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-01-13 13:26 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-09 17:09 [PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance Jan Kara 2020-04-09 17:09 ` [PATCH 1/2] bfq: Fix check detecting whether waker queue should be selected Jan Kara 2021-01-10 9:20 ` Paolo Valente 2021-01-13 13:09 ` Jan Kara 2021-01-13 13:11 ` Jan Kara 2020-04-09 17:09 ` [PATCH 2/2] bfq: Allow short_ttime queues to have waker Jan Kara 2021-01-10 9:20 ` Paolo Valente 2021-01-13 13:25 ` Jan Kara 2020-04-17 7:47 ` [PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance Paolo Valente 2020-04-17 9:18 ` Jan Kara
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.