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 BBDF6E82CA5 for ; Wed, 27 Sep 2023 15:25:44 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1837C6B017B; Wed, 27 Sep 2023 11:25:44 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1315D6B017D; Wed, 27 Sep 2023 11:25:44 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F143F6B0180; Wed, 27 Sep 2023 11:25:43 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id E35416B017B for ; Wed, 27 Sep 2023 11:25:43 -0400 (EDT) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id C5E5D1CA033 for ; Wed, 27 Sep 2023 15:25:43 +0000 (UTC) X-FDA: 81282752166.15.1E8DCD0 Received: from mail-lf1-f50.google.com (mail-lf1-f50.google.com [209.85.167.50]) by imf14.hostedemail.com (Postfix) with ESMTP id 7AAC710002C for ; Wed, 27 Sep 2023 15:25:41 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ZC52wIbS; spf=pass (imf14.hostedemail.com: domain of urezki@gmail.com designates 209.85.167.50 as permitted sender) smtp.mailfrom=urezki@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1695828341; 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=FOgg0rCTECztA3ZNecwIX4pkEzp5RUc43wOtDUEPPfs=; b=x0fAb9pW7LnDxoTkSGG+HBMOwHuRdhXWvvLUZ16IJQxSK5nDqrurgkKaAu1QlpX5TJurCt jTxx5IuRCvdcHgLzY3RGBxMKCAkvMT9MWH1P0LO6BNvIl0MDYSV9HIKydFZsc9tDXxfNH8 JYHD8zIE4/3V3aG3dv2knfjT/qKSUJM= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1695828341; a=rsa-sha256; cv=none; b=b7CBLvod9DOFHcpSiOlLabMRsx941ohyAlN9G4zHL+ttVuKqFFD4tPSJDdIbZAG5U2i81t b98pgSggk1Wy1OoZjTQBsR5xGXLS5TVE3kIXiSR5qNWvQNgqB8d1FaZmb2Utn7GuU0V3XC yKtsaGsrYyj+NWG4xf780NhJIzQPrqc= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ZC52wIbS; spf=pass (imf14.hostedemail.com: domain of urezki@gmail.com designates 209.85.167.50 as permitted sender) smtp.mailfrom=urezki@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-lf1-f50.google.com with SMTP id 2adb3069b0e04-50437f39c9dso16070350e87.3 for ; Wed, 27 Sep 2023 08:25:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695828339; x=1696433139; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:date:from:from:to:cc:subject:date:message-id:reply-to; bh=FOgg0rCTECztA3ZNecwIX4pkEzp5RUc43wOtDUEPPfs=; b=ZC52wIbSASIv4ZxqpNmHg6xAcJSqp7da1YsQIKMEbWxtprTf6Pgr1N57SCgN4aGkOP /hNULkOUGFgiyrQAfcz9LAaqHoBBLHpuzkcw3G6jeh3bEooEkUQBg3VhpafYkX5n7Fxv hqYvdZ2w0xG28yIalAzQ8RTAJ+jZirPaU9LNya/KwLNSYbVKSxPvUwmRdcH6N/PPpZSE QdrIUHCzLW55CyZ/pzxWmTzxYQ8fi+8clYHAeX7re/cYwc2Z0KIbXlURiBY3iWLr803s 4qq9w1wAh9WiQLB1UiRmb700KZkTdwRN6V3hV33IUaBnGFkr9XkT8RPQyEashSU1vjrh lHrg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695828339; x=1696433139; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:date:from:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=FOgg0rCTECztA3ZNecwIX4pkEzp5RUc43wOtDUEPPfs=; b=Fbe9ijkcCsGsp0HRQ6nqlMlxQrbFcrLZEoPTFqxunnjorXJ49wF1/5eT78VbQF0Rah Vn1hd8OslOTdRijxnbX8ZNYIadjcdF/hlAeX3q87vKoOv9EM+8Y6lIKf4qWBDIwBm+gL MqjePo7ellbQJt5BgWOA4uIEug0XN0hJlPruQdX0QLMRvI/HOjqndcRDdury3eMam+ik zZNGU7VYWOsdmwUOwLJhLjArM60DJ0xyEwd4MIy6LEbVcozx+FGOOQg2jyU/5xSWM8zz S+u2W/6QldJ2ycpdWmPWz1f9HrXzFUPualhWpPaf2Y8Ha0m0PgCRGRQpKv08KkRGKnND Ls4g== X-Gm-Message-State: AOJu0Yw5Ptz/fm3d9+q40G+5d/w5Sdvw1JvpGfWbjL//bDFM7KosA0hU eTtnulRhbWoQhpj3FM6ayGg= X-Google-Smtp-Source: AGHT+IE81EFp1/1I6Lw/7zU5N54oHaFwo53gq3+sGuZIgunJ+3ylGMrsLQp9tAEOoiKmSp4UkLXrJQ== X-Received: by 2002:a05:6512:752:b0:4fa:ad2d:6c58 with SMTP id c18-20020a056512075200b004faad2d6c58mr1778813lfs.61.1695828339262; Wed, 27 Sep 2023 08:25:39 -0700 (PDT) Received: from pc636 (host-90-233-214-51.mobileonline.telia.com. [90.233.214.51]) by smtp.gmail.com with ESMTPSA id o6-20020ac24c46000000b004ff91a94156sm519014lfk.121.2023.09.27.08.25.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 27 Sep 2023 08:25:38 -0700 (PDT) From: Uladzislau Rezki X-Google-Original-From: Uladzislau Rezki Date: Wed, 27 Sep 2023 17:25:36 +0200 To: "bhe@redhat.com" , Jaeseon Sim Cc: Uladzislau Rezki , 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-Stat-Signature: 36yd5iihd9rizfpzmqxhe44o4bcdoc7d X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 7AAC710002C X-Rspam-User: X-HE-Tag: 1695828341-878935 X-HE-Meta: U2FsdGVkX19/a1o2WZzpQ9sP6wsO7+/CH1ynsaOH30FcUQ68dojfF+f5EZ2VPt3FAqxWMUOhd+LpUatau9DOvXS6N/VCrSZRN4JMl27WOeqDHkkhVE7cw8vcakH/POM9XvDyqI3OP+bvU7G53aSv/tDsF0TA4myKLAD9x2B5i0xqVoAl86CdpaXWW12KtVZ01M/XHZaInGR4XB6ovETDgp/O71THmQ4rpibeAZLYkMicSAvYU9MRxXRYu953FE5s7TTrB0AvuHdBVzFli0iZc7QPl0u/j6D1vidM8LMqXRBoiVh0LnbJbjRap7Fx6Tvz/KqIVc2bRFPOOzCa5m2LLTMZzwmEM0Ke55EKTTMUb1Rs5VELuPhfoV0qQ95akyFOVfrnQit/l1mn4Cpt216FkM+SDJgXuW4K6ZBYFXCD5D/lE0ihS+DeFwEAvfKMO25JlF4Ch3MKkxLjrtUb7f73di34sqf5LG2UmLlzwQr+v5fzqoSoi/DUFDRRs9W2NNtq5MBVIacPmNKViCqGIajRANwQd03jDq9PV8DTl7zUkyvzKPROq0LL6xZvQe4jxxeafPsZrCEvPLi0lXpFDYqD2DqdSbxoIUX4Nw86M1G07vo8YDE+e4I4nUZQuyOZW3hZrWQciVrkCS5VU2LEdq6MGcY8FJO1oHe0Yw8M5KVAc9B3cYoJv43h9phIghpbRtzLfo/QmGltocaHW2hgBDsobjukvMz8H++LyytNDaHA7iypxqsp8ep0cEa5se3BpRJlMMYlLBNSSIIytTngjNCMln+4/ueNALeCi6OX5ERHeadu0MMWMmJoPz2v+7VSbp+OdXaL9/p1pt6FoEvHrxOtVxd6pqtWpsYyxGldTUOmyML5ib6MvLW4NlyMC6vY7SQP2oWBj4Ph6eGjmQRMfw3hRrobXQqQTGqo782QN762XiHSxvZafgR9vUHGQxWO7thCfI7TnPNWOUPVSBp9iXf ZFBe7MzO RZGJkm2uJOuDVL0KVohxPZuZbJ05OoDA+lOPkIhAvVILnR71kVIz0bLyolw8oXGe8iHaas731fvc7OJTHYqN0wY4LBgXkv/gFb98gWq4R2Itd6o5UCYVbsbpjrzlMIYz5c+fSj58ugljMT17b6623huJSb2dpy4vUOwZtf8Qrf/mf1c/kGKiS4Uoz+GTOZPv7o+qxewspnC/cFYVNeEVwxN+9E8tEa8sXRmuYrIppFIJA4DU+nYS7mlbJdzmxhiDquT+v8ahMyG8539X0Dt9ApkgRyWpCm5AZdiEv3JLBSKau/LeVkOo7daSvTsVav+2E1GyjKV8GMN/nxvyqyARMYwxvFyybmL292NoEqr9moxdeK7BHo1orda+KXIC328PnmG/kCYECmJZvdi6R8jM5+o2IiwcpJ5yL+SvWqpuGYzpPG5HcnELUWVype/hvwtieLktsaA98ygyHrGWUbAsUuWYMIPRKUUx6Ue/o5VYR6aZjodEvXikzrNggohiEGEqUhVfJDfbr3KzieiSTVjnFPfz1CuSUM2xVLIOIVv0RiIp1iOqyPIh+PYLpnvvJH7Dr8KePCkj+AD5Na/c= 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 Wed, Sep 27, 2023 at 09:33:04PM +0800, bhe@redhat.com wrote: > 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? > Agree. Embedding an error to "addr" is much more better way of handling it, so we do not need an extra storage for errors. This is good :) Jaeseon Sim - do you agree on it? If so please re-spin the patch if you do not want please let us know. > 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? > -- Uladzislau Rezki