From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id E0058E810DA for ; Wed, 27 Sep 2023 13:33:17 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 692688D0054; Wed, 27 Sep 2023 09:33:17 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 641E38D0002; Wed, 27 Sep 2023 09:33:17 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 509698D0054; Wed, 27 Sep 2023 09:33:17 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 409348D0002 for ; Wed, 27 Sep 2023 09:33:17 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 153D8C072C for ; Wed, 27 Sep 2023 13:33:17 +0000 (UTC) X-FDA: 81282468834.07.7A32DEB Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf11.hostedemail.com (Postfix) with ESMTP id 2708C40003 for ; Wed, 27 Sep 2023 13:33:14 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=DuNx97ug; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf11.hostedemail.com: domain of bhe@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=bhe@redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1695821595; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=d191/MAoMDGu3fFed8wTFk0OMMbfYp7/ARWWDK1xHWw=; b=YnWqV+/uaDrEPY4lKJtklXbBUj05rsReGmy4cLHcC4y6VQSxJDvgQZgcvZTaNV2OwtzxlF 6LLmNa9kysf3K3BvlqOx5UsviHI8N6bUb8zh0DzIbAzAJuN8R5sVXXDj1Up310PhNlG8UE g+Pg2gVyCRAqYrRXAjNI7DwtWFHWZ+A= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=DuNx97ug; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf11.hostedemail.com: domain of bhe@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=bhe@redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1695821595; a=rsa-sha256; cv=none; b=ExR+zTXl9/3JqyvnVOf4P+bm00kyLSc4zWEX3JAVVRQ/yTZDy8vX+3ES1oQ4W5sPCrxqRX kxhi9s8cmDB6ETeAGVp+I2gFRSI06r6llKnzsLqKZaWO2FTdTPRPGivn0q6UJ67qLojO4d kvxI5jtlTjc5mu+kFjYNkImevE24FDQ= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1695821594; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=d191/MAoMDGu3fFed8wTFk0OMMbfYp7/ARWWDK1xHWw=; b=DuNx97ugTTeeQyxQiEZdEIaN7SRqmhvpTkMlnsl5g9tW+tdWv7JJj+8FKfKYZOPNZEPSv/ rX85eKmI0ON1oD6u2/ATD+Yx6y6BNMCmup+uWq191veRu27sWrFgD6k5IwHG2XPsKwDwHd 8tVKggYz26vc7sunah3Ppw5PYpgP+HI= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-13-3jjIPhGaO461glygK-Lq2Q-1; Wed, 27 Sep 2023 09:33:09 -0400 X-MC-Unique: 3jjIPhGaO461glygK-Lq2Q-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 4ED793C0C49E; Wed, 27 Sep 2023 13:33:09 +0000 (UTC) Received: from localhost (unknown [10.72.112.58]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 179491678B; Wed, 27 Sep 2023 13:33:07 +0000 (UTC) Date: Wed, 27 Sep 2023 21:33:04 +0800 From: "bhe@redhat.com" To: Uladzislau Rezki Cc: Jaeseon Sim , "akpm@linux-foundation.org" , "hch@infradead.org" , "lstoakes@gmail.com" , "linux-mm@kvack.org" , "linux-kernel@vger.kernel.org" , Jaewon Kim Subject: Re: [PATCH] mm/vmalloc: Remove WARN_ON_ONCE related to adjust_va_to_fit_type Message-ID: References: <20230922062704epcms1p1722f24d4489a0435b339ce21db754ded@epcms1p1> <20230925105154epcms1p782c335c2355f39a9b583489c56e972f6@epcms1p7> <20230926052158epcms1p7fd7f3e3f523e5209977d3f5c62e85afa@epcms1p7> <20230926120549epcms1p4d41733c1c3698bd00eaa7e5ea0de041d@epcms1p4> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 X-Rspamd-Queue-Id: 2708C40003 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: dxicr8o8u31hcd16tbz1qp11tjso4uaa X-HE-Tag: 1695821594-770742 X-HE-Meta: U2FsdGVkX19GXptJp7jaBOU+2J6WvVjkeW/TEJSxgF84FHezy9nNlu4KKvhse8z70yGWWel6UdPWTpXj+GlIMceVnvehQgBRO1EOqgu+49AUoHCBhViN3DyuIGuW8Agytmfjzp8DI+biv+/3MUXOlV5jd123+vGSKbqG1gacwUUGkHmpGA6V6f0AescGavAaztjZBjjoZevXrTttS0zd7ODoRPsbOEYAbB7b3vq/Wdl36GQZFZituZ6aWkD+Oap5ImD6JbhfmXT9Rj/aS1TBTNLs5uUm6fxldBtAKmi0ud4cr2qyFJVDKADOIZKiu4dfYqhLcK/0enmA8CzDW4mTP1K1QmDUEoQHHbGfCrinvpHxMBYNAMVfy6LT4yUGPnsmLs1V1LhEUJa8PGf8axZfnHE8XzjjqoHGvrs5NoTrC5Qn5sHD0olt75oAmIOVglCgOH/zOPDIghIvA5rWwDAi1BqRgZT1VZHtd2eD7qpfffFvEupdMCwN8ZN1dKdXEFkzjowbPcaAiakHiS3ZnDjflsLed0xKjXuNZ7LHp0LJSywxeQaFNW1CxqopkD9Dh8qkfzZI0WJoD+u4GZUQe0x1C4wYG/O67zj0D0geU9JV5t36gQ/YMPVU38E3IPmSjNFL6GN2pZ9Qs5zp75hlFOzwj9eIJvUIdV2Gpzy5wxlq232aIo1lcP8SYPz0Ul5yV2vUTxl3FFq5eynzUdQryJiCu7AWmD50iaTvO96yjScSatBneoCW0cV5dSybLah2F0meDv2LhY/1A+yPbBAfia/zUhOmrKABqK0MFMr6t7ph3Lbxyn+f70xSFt1eDmOQHHHUgHUJQ/kMzdu6ioGi1/MjMD39FBGpue1EVvgSWcSCm1mNA1ItAXUeM3VvavqNN42226f3C/hGMFG768rZzki7TChXDaXBExS1BBY91t1Gis2M6dcezd2w5W8gT9WvmFDlQtLaYwQzyJj84YoiuK0 IL/SOhP5 zzkFI9jGSZXKFHiv+N862OWibSir3h8nxkX2h6bXSWLYkpKKgDfZR13yIcEOphC3ZpotmUest5TYGKDm13Eh5RXgrepVzZy1aDm+sCPFAYXTa84B/55gFxFoIJut/rJJoNCS1NygPPkyJdrU+pZ3h6bM1xP03etnYYIVrSOEdMm5WC3U6TgIf2booX1U5Xe4EAJdRs0J4244Z0Jp1qwtKU8v6dwAWbxC3x4DyjCVRyRebWigKgtP9EcdHiRzykf5PFzsVZKPbtWTgZ9kKwNHj1uhJRh7celB+40nWseu/TmY5mPg= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 09/27/23 at 01:49pm, Uladzislau Rezki wrote: > > > Yes, but GFP_NOWAIT-alloc-error can easily occur for low-memory device. > > > > > Agree. You are really in a low memory condition. We end up here only if > > pre-loading also has not succeeded, i.e. GFP_KERNEL also fails. > > > > But i agree with you, we should "improve the warning" because we drain > > and repeat. > > > > > How about changing fix as below?: > > > > > > > > > --- a/mm/vmalloc.c > > > +++ b/mm/vmalloc.c > > > @@ -1468,6 +1468,7 @@ adjust_va_to_fit_type(struct rb_root *root, struct list_head *head, > > > */ > > > va->va_start = nva_start_addr + size; > > > } else { > > > + WARN_ON_ONCE(1); > > > return -1; > > > } > > > > > > @@ -1522,7 +1523,7 @@ __alloc_vmap_area(struct rb_root *root, struct list_head *head, > > > > > > /* Update the free vmap_area. */ > > > ret = adjust_va_to_fit_type(root, head, va, nva_start_addr, size); > > > - if (WARN_ON_ONCE(ret)) > > > + if (ret) > > > return vend; > > > > > > #if DEBUG_AUGMENT_LOWEST_MATCH_CHECK > > > @@ -4143,7 +4144,7 @@ struct vm_struct **pcpu_get_vm_areas(const unsigned long *offsets, > > > ret = adjust_va_to_fit_type(&free_vmap_area_root, > > > &free_vmap_area_list, > > > va, start, size); > > > - if (WARN_ON_ONCE(unlikely(ret))) > > > + if (unlikely(ret)) > > > /* It is a BUG(), but trigger recovery instead. */ > > > goto recovery; > > > > > > > > > It will WARN_ONCE_ONCE() only if classify_va_fit_type() is "(type == NOTHING_FIT)". > > > > > This is good but i think it should be improved further. We need to > > understand from the warning when no memory and when there is no a > > vmap space, so: > > > > - if NOTHING_FIT, we should WARN() for sure; > > - Second place in the pcpu_get_vm_area(), we do not use NE_FIT. Only in > > the begging after boot, but potentially we can trigger -ENOMEM and we > > should warn in this case. Otherwise you just hide it; > > - And last one if after repeating we still do not manage to allocate. > > > > We should understand a reason of failing. I think error handling should > be improved. Something like: This looks good to me, while the parameter 'error' looks a little ugly. How about this? diff --git a/mm/vmalloc.c b/mm/vmalloc.c index ef8599d394fd..32805c82373b 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -1454,7 +1454,7 @@ adjust_va_to_fit_type(struct rb_root *root, struct list_head *head, */ lva = kmem_cache_alloc(vmap_area_cachep, GFP_NOWAIT); if (!lva) - return -1; + return -ENOMEM; } /* @@ -1468,7 +1468,7 @@ adjust_va_to_fit_type(struct rb_root *root, struct list_head *head, */ va->va_start = nva_start_addr + size; } else { - return -1; + return -EINVAL; } if (type != FL_FIT_TYPE) { @@ -1509,7 +1509,7 @@ __alloc_vmap_area(struct rb_root *root, struct list_head *head, va = find_vmap_lowest_match(root, size, align, vstart, adjust_search_size); if (unlikely(!va)) - return vend; + return -ENOENT; if (va->va_start > vstart) nva_start_addr = ALIGN(va->va_start, align); @@ -1518,12 +1518,12 @@ __alloc_vmap_area(struct rb_root *root, struct list_head *head, /* Check the "vend" restriction. */ if (nva_start_addr + size > vend) - return vend; + return -ERANGE; /* Update the free vmap_area. */ ret = adjust_va_to_fit_type(root, head, va, nva_start_addr, size); - if (WARN_ON_ONCE(ret)) - return vend; + if (ret) + return ret; #if DEBUG_AUGMENT_LOWEST_MATCH_CHECK find_vmap_lowest_match_check(root, head, size, align); @@ -1616,13 +1616,13 @@ static struct vmap_area *alloc_vmap_area(unsigned long size, size, align, vstart, vend); spin_unlock(&free_vmap_area_lock); - trace_alloc_vmap_area(addr, size, align, vstart, vend, addr == vend); + trace_alloc_vmap_area(addr, size, align, vstart, vend, IS_ERR(addr)); /* - * If an allocation fails, the "vend" address is + * If an allocation fails, the error value is * returned. Therefore trigger the overflow path. */ - if (unlikely(addr == vend)) + if (unlikely(IS_ERR(addr))) goto overflow; va->va_start = addr; @@ -1662,8 +1662,9 @@ static struct vmap_area *alloc_vmap_area(unsigned long size, } if (!(gfp_mask & __GFP_NOWARN) && printk_ratelimit()) - pr_warn("vmap allocation for size %lu failed: use vmalloc= to increase size\n", - size); + pr_warn("vmap allocation for size %lu failed: " + "use vmalloc= to increase size, errno: (%d)\n", + size, addr); kmem_cache_free(vmap_area_cachep, va); return ERR_PTR(-EBUSY); @@ -4143,8 +4144,8 @@ struct vm_struct **pcpu_get_vm_areas(const unsigned long *offsets, ret = adjust_va_to_fit_type(&free_vmap_area_root, &free_vmap_area_list, va, start, size); - if (WARN_ON_ONCE(unlikely(ret))) - /* It is a BUG(), but trigger recovery instead. */ + if ((unlikely(ret))) + WARN_ONCE(1, "%s error: errno (%d)\n", __func__, ret); goto recovery; /* Allocated area. */ > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index ef8599d394fd..03a36921a3fc 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -1454,7 +1454,7 @@ adjust_va_to_fit_type(struct rb_root *root, struct list_head *head, > */ > lva = kmem_cache_alloc(vmap_area_cachep, GFP_NOWAIT); > if (!lva) > - return -1; > + return -ENOMEM; > } > > /* > @@ -1468,7 +1468,7 @@ adjust_va_to_fit_type(struct rb_root *root, struct list_head *head, > */ > va->va_start = nva_start_addr + size; > } else { > - return -1; > + return -EINVAL; > } > > if (type != FL_FIT_TYPE) { > @@ -1488,7 +1488,8 @@ adjust_va_to_fit_type(struct rb_root *root, struct list_head *head, > static __always_inline unsigned long > __alloc_vmap_area(struct rb_root *root, struct list_head *head, > unsigned long size, unsigned long align, > - unsigned long vstart, unsigned long vend) > + unsigned long vstart, unsigned long vend, > + int *error) > { > bool adjust_search_size = true; > unsigned long nva_start_addr; > @@ -1508,8 +1509,10 @@ __alloc_vmap_area(struct rb_root *root, struct list_head *head, > adjust_search_size = false; > > va = find_vmap_lowest_match(root, size, align, vstart, adjust_search_size); > - if (unlikely(!va)) > + if (unlikely(!va)) { > + *error = -ENOENT; > return vend; > + } > > if (va->va_start > vstart) > nva_start_addr = ALIGN(va->va_start, align); > @@ -1517,13 +1520,17 @@ __alloc_vmap_area(struct rb_root *root, struct list_head *head, > nva_start_addr = ALIGN(vstart, align); > > /* Check the "vend" restriction. */ > - if (nva_start_addr + size > vend) > + if (nva_start_addr + size > vend) { > + *error = -ERANGE; > return vend; > + } > > /* Update the free vmap_area. */ > ret = adjust_va_to_fit_type(root, head, va, nva_start_addr, size); > - if (WARN_ON_ONCE(ret)) > + if (ret) { > + *error = ret; > return vend; > + } > > #if DEBUG_AUGMENT_LOWEST_MATCH_CHECK > find_vmap_lowest_match_check(root, head, size, align); > @@ -1589,7 +1596,7 @@ static struct vmap_area *alloc_vmap_area(unsigned long size, > unsigned long freed; > unsigned long addr; > int purged = 0; > - int ret; > + int ret, error; > > if (unlikely(!size || offset_in_page(size) || !is_power_of_2(align))) > return ERR_PTR(-EINVAL); > @@ -1613,7 +1620,7 @@ static struct vmap_area *alloc_vmap_area(unsigned long size, > retry: > preload_this_cpu_lock(&free_vmap_area_lock, gfp_mask, node); > addr = __alloc_vmap_area(&free_vmap_area_root, &free_vmap_area_list, > - size, align, vstart, vend); > + size, align, vstart, vend, &error); > spin_unlock(&free_vmap_area_lock); > > trace_alloc_vmap_area(addr, size, align, vstart, vend, addr == vend); > @@ -1662,8 +1669,9 @@ static struct vmap_area *alloc_vmap_area(unsigned long size, > } > > if (!(gfp_mask & __GFP_NOWARN) && printk_ratelimit()) > - pr_warn("vmap allocation for size %lu failed: use vmalloc= to increase size\n", > - size); > + pr_warn("vmap allocation for size %lu failed: " > + "use vmalloc= to increase size, errno: (%d)\n", > + size, error); > > kmem_cache_free(vmap_area_cachep, va); > return ERR_PTR(-EBUSY); > @@ -4143,9 +4151,10 @@ struct vm_struct **pcpu_get_vm_areas(const unsigned long *offsets, > ret = adjust_va_to_fit_type(&free_vmap_area_root, > &free_vmap_area_list, > va, start, size); > - if (WARN_ON_ONCE(unlikely(ret))) > - /* It is a BUG(), but trigger recovery instead. */ > + if (unlikely(ret)) { > + WARN_ONCE(1, "%s error: errno (%d)\n", __func__, ret); > goto recovery; > + } > > /* Allocated area. */ > va = vas[area]; > > > Any thoughts?