| Message ID | cc09f36e506145399fe470c71ad34c7c@EX13D12UWA002.ant.amazon.com |
|---|---|
| State | New |
| Headers | show |
| Series | PM: Batch hibernate and resume IO requests | expand |
On Tue, Sep 22, 2020 at 6:19 PM Chen, Xiaoyi <cxiaoyi@amazon.com> wrote: > > > Hibernate and resume process submits individual IO requests for each page > of the data. With this patch, we use blk_plug to improve the batching of > these requests. > > Tested this patch with hibernate and resumes, and consistently observed the > merging of the IO requests. We see more than an order of magnitude > improvement in hibernate and resume speed. > > One hibernate and resume cycle for 16GB used RAM out of 32GB takes around > 21 minutes before the change, and 1 minutes after the change on systems > with limited storage IOPS. > > Signed-off-by: Xiaoyi Chen <cxiaoyi@amazon.com> > Signed-off-by: Anchal Agarwal <anchalag@amazon.com> > --- > kernel/power/swap.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/kernel/power/swap.c b/kernel/power/swap.c > index 01e2858b5fe3..961615365b5c 100644 > --- a/kernel/power/swap.c > +++ b/kernel/power/swap.c > @@ -226,6 +226,7 @@ struct hib_bio_batch { > atomic_t count; > wait_queue_head_t wait; > blk_status_t error; > + struct blk_plug plug; > }; > > static void hib_init_batch(struct hib_bio_batch *hb) > @@ -233,6 +234,12 @@ static void hib_init_batch(struct hib_bio_batch *hb) > atomic_set(&hb->count, 0); > init_waitqueue_head(&hb->wait); > hb->error = BLK_STS_OK; > + blk_start_plug(&hb->plug); > +} > + > +static void hib_finish_batch(struct hib_bio_batch *hb) > +{ > + blk_finish_plug(&hb->plug); > } > > static void hib_end_io(struct bio *bio) > @@ -294,6 +301,10 @@ static int hib_submit_io(int op, int op_flags, pgoff_t page_off, void *addr, > > static blk_status_t hib_wait_io(struct hib_bio_batch *hb) > { > + /* > + * We are relying on the behavior of blk_plug that a thread with > + * a plug will flush the plug list before sleeping. > + */ > wait_event(hb->wait, atomic_read(&hb->count) == 0); > return blk_status_to_errno(hb->error); > } > @@ -561,6 +572,7 @@ static int save_image(struct swap_map_handle *handle, > nr_pages++; > } > err2 = hib_wait_io(&hb); > + hib_finish_batch(&hb); > stop = ktime_get(); > if (!ret) > ret = err2; > @@ -854,6 +866,7 @@ static int save_image_lzo(struct swap_map_handle *handle, > pr_info("Image saving done\n"); > swsusp_show_speed(start, stop, nr_to_write, "Wrote"); > out_clean: > + hib_finish_batch(&hb); > if (crc) { > if (crc->thr) > kthread_stop(crc->thr); > @@ -1084,6 +1097,7 @@ static int load_image(struct swap_map_handle *handle, > nr_pages++; > } > err2 = hib_wait_io(&hb); > + hib_finish_batch(&hb); > stop = ktime_get(); > if (!ret) > ret = err2; > @@ -1447,6 +1461,7 @@ static int load_image_lzo(struct swap_map_handle *handle, > } > swsusp_show_speed(start, stop, nr_to_read, "Read"); > out_clean: > + hib_finish_batch(&hb); > for (i = 0; i < ring_size; i++) > free_page((unsigned long)page[i]); > if (crc) { > -- Applied as 5.10 material with some subject and changelog edits, but: 1. The patch is white-space-damaged and I needed to fix that up manually which was not fun. 2. I dropped the second S-o-b, because it was not clear to me whether a Co-developed-by tag was missing or Reviewed-by should have been used instead. Thanks!
On 9/25/20 4:27 PM, Rafael J. Wysocki wrote: > > > On Tue, Sep 22, 2020 at 6:19 PM Chen, Xiaoyi <cxiaoyi@amazon.com> wrote: >> >> >> Hibernate and resume process submits individual IO requests for each page >> of the data. With this patch, we use blk_plug to improve the batching of >> these requests. >> >> Tested this patch with hibernate and resumes, and consistently observed the >> merging of the IO requests. We see more than an order of magnitude >> improvement in hibernate and resume speed. >> >> One hibernate and resume cycle for 16GB used RAM out of 32GB takes around >> 21 minutes before the change, and 1 minutes after the change on systems >> with limited storage IOPS. >> >> Signed-off-by: Xiaoyi Chen <cxiaoyi@amazon.com> >> Signed-off-by: Anchal Agarwal <anchalag@amazon.com> >> --- >> kernel/power/swap.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/kernel/power/swap.c b/kernel/power/swap.c >> index 01e2858b5fe3..961615365b5c 100644 >> --- a/kernel/power/swap.c >> +++ b/kernel/power/swap.c >> @@ -226,6 +226,7 @@ struct hib_bio_batch { >> atomic_t count; >> wait_queue_head_t wait; >> blk_status_t error; >> + struct blk_plug plug; >> }; >> >> static void hib_init_batch(struct hib_bio_batch *hb) >> @@ -233,6 +234,12 @@ static void hib_init_batch(struct hib_bio_batch *hb) >> atomic_set(&hb->count, 0); >> init_waitqueue_head(&hb->wait); >> hb->error = BLK_STS_OK; >> + blk_start_plug(&hb->plug); >> +} >> + >> +static void hib_finish_batch(struct hib_bio_batch *hb) >> +{ >> + blk_finish_plug(&hb->plug); >> } >> >> static void hib_end_io(struct bio *bio) >> @@ -294,6 +301,10 @@ static int hib_submit_io(int op, int op_flags, pgoff_t page_off, void *addr, >> >> static blk_status_t hib_wait_io(struct hib_bio_batch *hb) >> { >> + /* >> + * We are relying on the behavior of blk_plug that a thread with >> + * a plug will flush the plug list before sleeping. >> + */ >> wait_event(hb->wait, atomic_read(&hb->count) == 0); >> return blk_status_to_errno(hb->error); >> } >> @@ -561,6 +572,7 @@ static int save_image(struct swap_map_handle *handle, >> nr_pages++; >> } >> err2 = hib_wait_io(&hb); >> + hib_finish_batch(&hb); >> stop = ktime_get(); >> if (!ret) >> ret = err2; >> @@ -854,6 +866,7 @@ static int save_image_lzo(struct swap_map_handle *handle, >> pr_info("Image saving done\n"); >> swsusp_show_speed(start, stop, nr_to_write, "Wrote"); >> out_clean: >> + hib_finish_batch(&hb); >> if (crc) { >> if (crc->thr) >> kthread_stop(crc->thr); >> @@ -1084,6 +1097,7 @@ static int load_image(struct swap_map_handle *handle, >> nr_pages++; >> } >> err2 = hib_wait_io(&hb); >> + hib_finish_batch(&hb); >> stop = ktime_get(); >> if (!ret) >> ret = err2; >> @@ -1447,6 +1461,7 @@ static int load_image_lzo(struct swap_map_handle *handle, >> } >> swsusp_show_speed(start, stop, nr_to_read, "Read"); >> out_clean: >> + hib_finish_batch(&hb); >> for (i = 0; i < ring_size; i++) >> free_page((unsigned long)page[i]); >> if (crc) { >> -- > > Applied as 5.10 material with some subject and changelog edits, but: > 1. The patch is white-space-damaged and I needed to fix that up > manually which was not fun. > 2. I dropped the second S-o-b, because it was not clear to me whether > a Co-developed-by tag was missing or Reviewed-by should have been used > instead. > > Thanks! > Thanks for the prompt review. Apologies for the white-space and tags issues, will do better next time. Would you mind adding following tags if not too late? Signed-off-by: Anchal Agarwal <anchalag@amazon.com> Co-Developed-by: Anchal Agarwal <anchalag@amazon.com>
On Fri, Sep 25, 2020 at 9:26 PM Chen, Xiaoyi <cxiaoyi@amazon.com> wrote: > > On 9/25/20 4:27 PM, Rafael J. Wysocki wrote: > > > > > > On Tue, Sep 22, 2020 at 6:19 PM Chen, Xiaoyi <cxiaoyi@amazon.com> wrote: > >> > >> > >> Hibernate and resume process submits individual IO requests for each page > >> of the data. With this patch, we use blk_plug to improve the batching of > >> these requests. > >> > >> Tested this patch with hibernate and resumes, and consistently observed the > >> merging of the IO requests. We see more than an order of magnitude > >> improvement in hibernate and resume speed. > >> > >> One hibernate and resume cycle for 16GB used RAM out of 32GB takes around > >> 21 minutes before the change, and 1 minutes after the change on systems > >> with limited storage IOPS. > >> > >> Signed-off-by: Xiaoyi Chen <cxiaoyi@amazon.com> > >> Signed-off-by: Anchal Agarwal <anchalag@amazon.com> > >> --- > >> kernel/power/swap.c | 15 +++++++++++++++ > >> 1 file changed, 15 insertions(+) > >> > >> diff --git a/kernel/power/swap.c b/kernel/power/swap.c > >> index 01e2858b5fe3..961615365b5c 100644 > >> --- a/kernel/power/swap.c > >> +++ b/kernel/power/swap.c > >> @@ -226,6 +226,7 @@ struct hib_bio_batch { > >> atomic_t count; > >> wait_queue_head_t wait; > >> blk_status_t error; > >> + struct blk_plug plug; > >> }; > >> > >> static void hib_init_batch(struct hib_bio_batch *hb) > >> @@ -233,6 +234,12 @@ static void hib_init_batch(struct hib_bio_batch *hb) > >> atomic_set(&hb->count, 0); > >> init_waitqueue_head(&hb->wait); > >> hb->error = BLK_STS_OK; > >> + blk_start_plug(&hb->plug); > >> +} > >> + > >> +static void hib_finish_batch(struct hib_bio_batch *hb) > >> +{ > >> + blk_finish_plug(&hb->plug); > >> } > >> > >> static void hib_end_io(struct bio *bio) > >> @@ -294,6 +301,10 @@ static int hib_submit_io(int op, int op_flags, pgoff_t page_off, void *addr, > >> > >> static blk_status_t hib_wait_io(struct hib_bio_batch *hb) > >> { > >> + /* > >> + * We are relying on the behavior of blk_plug that a thread with > >> + * a plug will flush the plug list before sleeping. > >> + */ > >> wait_event(hb->wait, atomic_read(&hb->count) == 0); > >> return blk_status_to_errno(hb->error); > >> } > >> @@ -561,6 +572,7 @@ static int save_image(struct swap_map_handle *handle, > >> nr_pages++; > >> } > >> err2 = hib_wait_io(&hb); > >> + hib_finish_batch(&hb); > >> stop = ktime_get(); > >> if (!ret) > >> ret = err2; > >> @@ -854,6 +866,7 @@ static int save_image_lzo(struct swap_map_handle *handle, > >> pr_info("Image saving done\n"); > >> swsusp_show_speed(start, stop, nr_to_write, "Wrote"); > >> out_clean: > >> + hib_finish_batch(&hb); > >> if (crc) { > >> if (crc->thr) > >> kthread_stop(crc->thr); > >> @@ -1084,6 +1097,7 @@ static int load_image(struct swap_map_handle *handle, > >> nr_pages++; > >> } > >> err2 = hib_wait_io(&hb); > >> + hib_finish_batch(&hb); > >> stop = ktime_get(); > >> if (!ret) > >> ret = err2; > >> @@ -1447,6 +1461,7 @@ static int load_image_lzo(struct swap_map_handle *handle, > >> } > >> swsusp_show_speed(start, stop, nr_to_read, "Read"); > >> out_clean: > >> + hib_finish_batch(&hb); > >> for (i = 0; i < ring_size; i++) > >> free_page((unsigned long)page[i]); > >> if (crc) { > >> -- > > > > Applied as 5.10 material with some subject and changelog edits, but: > > 1. The patch is white-space-damaged and I needed to fix that up > > manually which was not fun. > > 2. I dropped the second S-o-b, because it was not clear to me whether > > a Co-developed-by tag was missing or Reviewed-by should have been used > > instead. > > > > Thanks! > > > > Thanks for the prompt review. Apologies for the white-space and tags > issues, will do better next time. > > Would you mind adding following tags if not too late? > Signed-off-by: Anchal Agarwal <anchalag@amazon.com> > Co-Developed-by: Anchal Agarwal <anchalag@amazon.com> Done, thanks!
diff --git a/kernel/power/swap.c b/kernel/power/swap.c index 01e2858b5fe3..961615365b5c 100644 --- a/kernel/power/swap.c +++ b/kernel/power/swap.c @@ -226,6 +226,7 @@ struct hib_bio_batch { atomic_t count; wait_queue_head_t wait; blk_status_t error; + struct blk_plug plug; }; static void hib_init_batch(struct hib_bio_batch *hb) @@ -233,6 +234,12 @@ static void hib_init_batch(struct hib_bio_batch *hb) atomic_set(&hb->count, 0); init_waitqueue_head(&hb->wait); hb->error = BLK_STS_OK; + blk_start_plug(&hb->plug); +} + +static void hib_finish_batch(struct hib_bio_batch *hb) +{ + blk_finish_plug(&hb->plug); } static void hib_end_io(struct bio *bio) @@ -294,6 +301,10 @@ static int hib_submit_io(int op, int op_flags, pgoff_t page_off, void *addr, static blk_status_t hib_wait_io(struct hib_bio_batch *hb) { + /* + * We are relying on the behavior of blk_plug that a thread with + * a plug will flush the plug list before sleeping. + */ wait_event(hb->wait, atomic_read(&hb->count) == 0); return blk_status_to_errno(hb->error); } @@ -561,6 +572,7 @@ static int save_image(struct swap_map_handle *handle, nr_pages++; } err2 = hib_wait_io(&hb); + hib_finish_batch(&hb); stop = ktime_get(); if (!ret) ret = err2; @@ -854,6 +866,7 @@ static int save_image_lzo(struct swap_map_handle *handle, pr_info("Image saving done\n"); swsusp_show_speed(start, stop, nr_to_write, "Wrote"); out_clean: + hib_finish_batch(&hb); if (crc) { if (crc->thr) kthread_stop(crc->thr); @@ -1084,6 +1097,7 @@ static int load_image(struct swap_map_handle *handle, nr_pages++; } err2 = hib_wait_io(&hb); + hib_finish_batch(&hb); stop = ktime_get(); if (!ret) ret = err2; @@ -1447,6 +1461,7 @@ static int load_image_lzo(struct swap_map_handle *handle, } swsusp_show_speed(start, stop, nr_to_read, "Read"); out_clean: + hib_finish_batch(&hb); for (i = 0; i < ring_size; i++) free_page((unsigned long)page[i]); if (crc) {