On Thu, 5 Jul 2018, John Stoffel wrote:
> >>>>> "Mikulas" == Mikulas Patocka <[email protected]> writes:
>
> Mikulas> It has been noticed that congestion throttling can slow down
> Mikulas> allocations path that participate in the IO and thus help the
> Mikulas> memory reclaim. Stalling those allocation is therefore not
> Mikulas> productive. Moreover mempool allocator and md variants of the
> Mikulas> same already implement their own throttling which has a
> Mikulas> better way to be feedback driven. Stalling at the page
> Mikulas> allocator is therefore even counterproductive.
>
> Can you show numbers for this claim?
This is bug report where this problem is reported:
https://www.redhat.com/archives/dm-devel/2018-June/msg00161.html
A process takes dm-bufio lock, allocates memory and memory allocation
sleeps in wait_iff_congested. kswapd is attempting to take the dm-bufio
lock as well, so sleeping in wait_iff_congested delays kswapd. Delaying
kswapd prevents the system from reclaiming memory.
> I would think that throttling needs to be done as close to the disk as
> possible, and propogate back up the layers to have this all work well,
> so that faster devices (and the layers stacked on them) will work better
> without stalling on a slow device.
The drivers that participate in memory reclaim shouldn't be throttled at
all. If you throttle them, you slow down reclaim - and if you slow down
reclaim, the congestion will not be resolved.
> Mikulas> PF_LESS_THROTTLE is a task flag denoting allocation context that is
> Mikulas> participating in the memory reclaim which fits into these IO paths
> Mikulas> model, so use the flag and make the page allocator aware they are
> Mikulas> special and they do not really want any dirty data throttling.
>
> Mikulas> The throttling causes stalls on Android - it uses the dm-verity
> driver
> Mikulas> that uses dm-bufio. Allocations in dm-bufio were observed to sleep
> in
> Mikulas> wait_iff_congested repeatedly.
>
> Mikulas> Signed-off-by: Mikulas Patocka <[email protected]>
> Mikulas> Acked-by: Michal Hocko <[email protected]> # mempool_alloc and
> bvec_alloc
> Mikulas> Cc: [email protected]
>
> Mikulas> ---
> Mikulas> block/bio.c | 4 ++++
> Mikulas> drivers/md/dm-bufio.c | 14 +++++++++++---
> Mikulas> drivers/md/dm-crypt.c | 8 ++++++++
> Mikulas> drivers/md/dm-integrity.c | 4 ++++
> Mikulas> drivers/md/dm-kcopyd.c | 3 +++
> Mikulas> drivers/md/dm-verity-target.c | 4 ++++
> Mikulas> drivers/md/dm-writecache.c | 4 ++++
> Mikulas> mm/mempool.c | 4 ++++
> Mikulas> 8 files changed, 42 insertions(+), 3 deletions(-)
>
> Mikulas> Index: linux-2.6/mm/mempool.c
> Mikulas> ===================================================================
> Mikulas> --- linux-2.6.orig/mm/mempool.c 2018-06-29 03:47:16.290000000
> +0200
> Mikulas> +++ linux-2.6/mm/mempool.c 2018-06-29 03:47:16.270000000 +0200
> Mikulas> @@ -369,6 +369,7 @@ void *mempool_alloc(mempool_t *pool, gfp
> Mikulas> unsigned long flags;
> Mikulas> wait_queue_entry_t wait;
> Mikulas> gfp_t gfp_temp;
> Mikulas> + unsigned old_flags;
>
> Mikulas> VM_WARN_ON_ONCE(gfp_mask & __GFP_ZERO);
> Mikulas> might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);
> Mikulas> @@ -381,7 +382,10 @@ void *mempool_alloc(mempool_t *pool, gfp
>
> Mikulas> repeat_alloc:
>
> Mikulas> + old_flags = current->flags & PF_LESS_THROTTLE;
> Mikulas> + current->flags |= PF_LESS_THROTTLE;
> Mikulas> element = pool->alloc(gfp_temp, pool->pool_data);
> Mikulas> + current_restore_flags(old_flags, PF_LESS_THROTTLE);
> Mikulas> if (likely(element != NULL))
> Mikulas> return element;
>
> Mikulas> Index: linux-2.6/block/bio.c
> Mikulas> ===================================================================
> Mikulas> --- linux-2.6.orig/block/bio.c 2018-06-29 03:47:16.290000000
> +0200
> Mikulas> +++ linux-2.6/block/bio.c 2018-06-29 03:47:16.270000000 +0200
> Mikulas> @@ -217,6 +217,7 @@ fallback:
> Mikulas> } else {
> Mikulas> struct biovec_slab *bvs = bvec_slabs + *idx;
> Mikulas> gfp_t __gfp_mask = gfp_mask & ~(__GFP_DIRECT_RECLAIM |
> __GFP_IO);
> Mikulas> + unsigned old_flags;
>
> Mikulas> /*
> Mikulas> * Make this allocation restricted and don't dump info
> on
> Mikulas> @@ -229,7 +230,10 @@ fallback:
> Mikulas> * Try a slab allocation. If this fails and
> __GFP_DIRECT_RECLAIM
> Mikulas> * is set, retry with the 1-entry mempool
> Mikulas> */
> Mikulas> + old_flags = current->flags & PF_LESS_THROTTLE;
> Mikulas> + current->flags |= PF_LESS_THROTTLE;
> Mikulas> bvl = kmem_cache_alloc(bvs->slab, __gfp_mask);
> Mikulas> + current_restore_flags(old_flags, PF_LESS_THROTTLE);
> Mikulas> if (unlikely(!bvl && (gfp_mask &
> __GFP_DIRECT_RECLAIM))) {
> Mikulas> *idx = BVEC_POOL_MAX;
> Mikulas> goto fallback;
> Mikulas> Index: linux-2.6/drivers/md/dm-bufio.c
> Mikulas> ===================================================================
> Mikulas> --- linux-2.6.orig/drivers/md/dm-bufio.c 2018-06-29
> 03:47:16.290000000 +0200
> Mikulas> +++ linux-2.6/drivers/md/dm-bufio.c 2018-06-29 03:47:16.270000000
> +0200
> Mikulas> @@ -356,6 +356,7 @@ static void __cache_size_refresh(void)
> Mikulas> static void *alloc_buffer_data(struct dm_bufio_client *c, gfp_t
> gfp_mask,
> Mikulas> unsigned char *data_mode)
> Mikulas> {
> Mikulas> + void *ptr;
> Mikulas> if (unlikely(c->slab_cache != NULL)) {
> Mikulas> *data_mode = DATA_MODE_SLAB;
> Mikulas> return kmem_cache_alloc(c->slab_cache, gfp_mask);
> Mikulas> @@ -363,9 +364,14 @@ static void *alloc_buffer_data(struct dm
>
> Mikulas> if (c->block_size <= KMALLOC_MAX_SIZE &&
> Mikulas> gfp_mask & __GFP_NORETRY) {
> Mikulas> + unsigned old_flags;
> Mikulas> *data_mode = DATA_MODE_GET_FREE_PAGES;
> Mikulas> - return (void *)__get_free_pages(gfp_mask,
> Mikulas> + old_flags = current->flags & PF_LESS_THROTTLE;
> Mikulas> + current->flags |= PF_LESS_THROTTLE;
> Mikulas> + ptr = (void *)__get_free_pages(gfp_mask,
> c-> sectors_per_block_bits - (PAGE_SHIFT - SECTOR_SHIFT));
> Mikulas> + current_restore_flags(old_flags, PF_LESS_THROTTLE);
> Mikulas> + return ptr;
> Mikulas> }
>
> Mikulas> *data_mode = DATA_MODE_VMALLOC;
> Mikulas> @@ -381,8 +387,10 @@ static void *alloc_buffer_data(struct dm
> Mikulas> */
> Mikulas> if (gfp_mask & __GFP_NORETRY) {
> Mikulas> unsigned noio_flag = memalloc_noio_save();
> Mikulas> - void *ptr = __vmalloc(c->block_size, gfp_mask,
> PAGE_KERNEL);
> Mikulas> -
> Mikulas> + unsigned old_flags = current->flags & PF_LESS_THROTTLE;
> Mikulas> + current->flags |= PF_LESS_THROTTLE;
> Mikulas> + ptr = __vmalloc(c->block_size, gfp_mask, PAGE_KERNEL);
> Mikulas> + current_restore_flags(old_flags, PF_LESS_THROTTLE);
> Mikulas> memalloc_noio_restore(noio_flag);
> Mikulas> return ptr;
> Mikulas> }
> Mikulas> Index: linux-2.6/drivers/md/dm-integrity.c
> Mikulas> ===================================================================
> Mikulas> --- linux-2.6.orig/drivers/md/dm-integrity.c 2018-06-29
> 03:47:16.290000000 +0200
> Mikulas> +++ linux-2.6/drivers/md/dm-integrity.c 2018-06-29
> 03:47:16.270000000 +0200
> Mikulas> @@ -1318,6 +1318,7 @@ static void integrity_metadata(struct wo
> Mikulas> int r;
>
> Mikulas> if (ic->internal_hash) {
> Mikulas> + unsigned old_flags;
> Mikulas> struct bvec_iter iter;
> Mikulas> struct bio_vec bv;
> Mikulas> unsigned digest_size =
> crypto_shash_digestsize(ic->internal_hash);
> Mikulas> @@ -1331,8 +1332,11 @@ static void integrity_metadata(struct wo
> Mikulas> if (unlikely(ic->mode == 'R'))
> Mikulas> goto skip_io;
>
> Mikulas> + old_flags = current->flags & PF_LESS_THROTTLE;
> Mikulas> + current->flags |= PF_LESS_THROTTLE;
> Mikulas> checksums = kmalloc((PAGE_SIZE >> SECTOR_SHIFT >>
> ic->sb->log2_sectors_per_block) * ic->tag_size + extra_space,
> Mikulas> GFP_NOIO | __GFP_NORETRY |
> __GFP_NOWARN);
> Mikulas> + current_restore_flags(old_flags, PF_LESS_THROTTLE);
> Mikulas> if (!checksums)
> Mikulas> checksums = checksums_onstack;
>
> Mikulas> Index: linux-2.6/drivers/md/dm-kcopyd.c
> Mikulas> ===================================================================
> Mikulas> --- linux-2.6.orig/drivers/md/dm-kcopyd.c 2018-06-29
> 03:47:16.290000000 +0200
> Mikulas> +++ linux-2.6/drivers/md/dm-kcopyd.c 2018-06-29 03:47:16.270000000
> +0200
> Mikulas> @@ -245,7 +245,10 @@ static int kcopyd_get_pages(struct dm_kc
> Mikulas> *pages = NULL;
>
> Mikulas> do {
> Mikulas> + unsigned old_flags = current->flags & PF_LESS_THROTTLE;
> Mikulas> + current->flags |= PF_LESS_THROTTLE;
> Mikulas> pl = alloc_pl(__GFP_NOWARN | __GFP_NORETRY |
> __GFP_KSWAPD_RECLAIM);
> Mikulas> + current_restore_flags(old_flags, PF_LESS_THROTTLE);
> Mikulas> if (unlikely(!pl)) {
> Mikulas> /* Use reserved pages */
> Mikulas> pl = kc->pages;
> Mikulas> Index: linux-2.6/drivers/md/dm-verity-target.c
> Mikulas> ===================================================================
> Mikulas> --- linux-2.6.orig/drivers/md/dm-verity-target.c 2018-06-29
> 03:47:16.290000000 +0200
> Mikulas> +++ linux-2.6/drivers/md/dm-verity-target.c 2018-06-29
> 03:47:16.280000000 +0200
> Mikulas> @@ -596,9 +596,13 @@ no_prefetch_cluster:
> Mikulas> static void verity_submit_prefetch(struct dm_verity *v, struct
> dm_verity_io *io)
> Mikulas> {
> Mikulas> struct dm_verity_prefetch_work *pw;
> Mikulas> + unsigned old_flags;
>
> Mikulas> + old_flags = current->flags & PF_LESS_THROTTLE;
> Mikulas> + current->flags |= PF_LESS_THROTTLE;
> Mikulas> pw = kmalloc(sizeof(struct dm_verity_prefetch_work),
> Mikulas> GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC |
> __GFP_NOWARN);
> Mikulas> + current_restore_flags(old_flags, PF_LESS_THROTTLE);
>
> Mikulas> if (!pw)
> Mikulas> return;
> Mikulas> Index: linux-2.6/drivers/md/dm-writecache.c
> Mikulas> ===================================================================
> Mikulas> --- linux-2.6.orig/drivers/md/dm-writecache.c 2018-06-29
> 03:47:16.290000000 +0200
> Mikulas> +++ linux-2.6/drivers/md/dm-writecache.c 2018-06-29
> 03:47:16.280000000 +0200
> Mikulas> @@ -1473,6 +1473,7 @@ static void __writecache_writeback_pmem(
> Mikulas> unsigned max_pages;
>
> Mikulas> while (wbl->size) {
> Mikulas> + unsigned old_flags;
> wbl-> size--;
> Mikulas> e = container_of(wbl->list.prev, struct wc_entry, lru);
> Mikulas> list_del(&e->lru);
> Mikulas> @@ -1486,6 +1487,8 @@ static void __writecache_writeback_pmem(
> Mikulas> bio_set_dev(&wb->bio, wc->dev->bdev);
> wb-> bio.bi_iter.bi_sector = read_original_sector(wc, e);
> wb-> page_offset = PAGE_SIZE;
> Mikulas> + old_flags = current->flags & PF_LESS_THROTTLE;
> Mikulas> + current->flags |= PF_LESS_THROTTLE;
> Mikulas> if (max_pages <= WB_LIST_INLINE ||
> Mikulas> unlikely(!(wb->wc_list = kmalloc(max_pages *
> sizeof(struct wc_entry *),
> Mikulas> GFP_NOIO |
> __GFP_NORETRY |
> Mikulas> @@ -1493,6 +1496,7 @@ static void __writecache_writeback_pmem(
> wb-> wc_list = wb->wc_list_inline;
> Mikulas> max_pages = WB_LIST_INLINE;
> Mikulas> }
> Mikulas> + current_restore_flags(old_flags, PF_LESS_THROTTLE);
>
> Mikulas> BUG_ON(!wc_add_block(wb, e, GFP_NOIO));
>
> Mikulas> Index: linux-2.6/drivers/md/dm-crypt.c
> Mikulas> ===================================================================
> Mikulas> --- linux-2.6.orig/drivers/md/dm-crypt.c 2018-06-29
> 03:47:16.290000000 +0200
> Mikulas> +++ linux-2.6/drivers/md/dm-crypt.c 2018-06-29 03:47:16.280000000
> +0200
> Mikulas> @@ -2181,12 +2181,16 @@ static void *crypt_page_alloc(gfp_t gfp_
> Mikulas> {
> Mikulas> struct crypt_config *cc = pool_data;
> Mikulas> struct page *page;
> Mikulas> + unsigned old_flags;
>
> Mikulas> if (unlikely(percpu_counter_compare(&cc->n_allocated_pages,
> dm_crypt_pages_per_client) >= 0) &&
> Mikulas> likely(gfp_mask & __GFP_NORETRY))
> Mikulas> return NULL;
>
> Mikulas> + old_flags = current->flags & PF_LESS_THROTTLE;
> Mikulas> + current->flags |= PF_LESS_THROTTLE;
> Mikulas> page = alloc_page(gfp_mask);
> Mikulas> + current_restore_flags(old_flags, PF_LESS_THROTTLE);
> Mikulas> if (likely(page != NULL))
> Mikulas> percpu_counter_add(&cc->n_allocated_pages, 1);
>
> Mikulas> @@ -2893,7 +2897,10 @@ static int crypt_map(struct dm_target *t
>
> Mikulas> if (cc->on_disk_tag_size) {
> Mikulas> unsigned tag_len = cc->on_disk_tag_size *
> (bio_sectors(bio) >> cc->sector_shift);
> Mikulas> + unsigned old_flags;
>
> Mikulas> + old_flags = current->flags & PF_LESS_THROTTLE;
> Mikulas> + current->flags |= PF_LESS_THROTTLE;
> Mikulas> if (unlikely(tag_len > KMALLOC_MAX_SIZE) ||
> Mikulas> unlikely(!(io->integrity_metadata = kmalloc(tag_len,
> Mikulas> GFP_NOIO | __GFP_NORETRY |
> __GFP_NOMEMALLOC | __GFP_NOWARN)))) {
> Mikulas> @@ -2902,6 +2909,7 @@ static int crypt_map(struct dm_target *t
> io-> integrity_metadata = mempool_alloc(&cc->tag_pool, GFP_NOIO);
> io-> integrity_metadata_from_pool = true;
> Mikulas> }
> Mikulas> + current_restore_flags(old_flags, PF_LESS_THROTTLE);
> Mikulas> }
>
> Mikulas> if (crypt_integrity_aead(cc))
>
> Mikulas> --
> Mikulas> dm-devel mailing list
> Mikulas> [email protected]
> Mikulas> https://www.redhat.com/mailman/listinfo/dm-devel
>
--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel
