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 1E8B4C4332F for ; Mon, 19 Dec 2022 12:25:02 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id ABAE98E0005; Mon, 19 Dec 2022 07:25:01 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id A43838E0001; Mon, 19 Dec 2022 07:25:01 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 90D898E0005; Mon, 19 Dec 2022 07:25:01 -0500 (EST) 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 813CB8E0001 for ; Mon, 19 Dec 2022 07:25:01 -0500 (EST) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 4CEEB160AF0 for ; Mon, 19 Dec 2022 12:25:01 +0000 (UTC) X-FDA: 80258975202.20.C5116B2 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf23.hostedemail.com (Postfix) with ESMTP id E15AC14000E for ; Mon, 19 Dec 2022 12:24:58 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=YHGTCsCM; spf=pass (imf23.hostedemail.com: domain of bhe@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=bhe@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1671452699; 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=ZJjKwl9viRzOtHWuPnaZZpLHGevx7oKmf4B2oFCUswI=; b=hQDoJs3P7ygPRL44pYzUeE4S5lTWSCaBUrChb487w1gkUKdosKL/M/yiPNUNpNipx0BcPn NZR3X43sJWXsQyvWzRKGAxJZ9+EpHO7rLXpoMj2mdeCfkl8jGal0LEuJaOqNqfsc3rPzFK 2uivK/if+38KuIQe8DS4MZgQ1mUF++Y= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=YHGTCsCM; spf=pass (imf23.hostedemail.com: domain of bhe@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=bhe@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1671452699; a=rsa-sha256; cv=none; b=AbMZLNA/pFT6DiQEDrjcBmpZKSpy4yFR2pCCZ7V6tq8oUXEJoBf4QR3tifjRJDJJc0mBGA 7AZwH6cqiVT/gQlTU1UDnaS1HrKQMYCX/oegkr5FUWGa1LZwil1h3u9Rn/RI2zDAlhH+0Y NOG+FiY2Z/0yY5UxWABGTuqMHopc79U= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1671452698; 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=ZJjKwl9viRzOtHWuPnaZZpLHGevx7oKmf4B2oFCUswI=; b=YHGTCsCMBZOuLvQ51XVBIFHQCfQaT3Ohe9OzKIjewa+msVfBXrZujnKA4eS4asQMF4/Br6 2Y2l64W10EdDESIGHmsXRJTjD6bkZKvDzJmQuVriLqM5pQXCINnAIiOe+JCoePv9eUmEm/ l9ZBNYE1qz0IIJoiJBYM6n/mwol216M= Received: from mimecast-mx02.redhat.com (mx3-rdu2.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-99-cGoprQawMYWMl6k7v-feqA-1; Mon, 19 Dec 2022 07:24:53 -0500 X-MC-Unique: cGoprQawMYWMl6k7v-feqA-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 81DA01C07562; Mon, 19 Dec 2022 12:24:52 +0000 (UTC) Received: from localhost (ovpn-12-41.pek2.redhat.com [10.72.12.41]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2413C1410DD7; Mon, 19 Dec 2022 12:24:50 +0000 (UTC) Date: Mon, 19 Dec 2022 20:24:47 +0800 From: Baoquan He To: Lorenzo Stoakes Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, urezki@gmail.com, stephen.s.brennan@oracle.com, willy@infradead.org, akpm@linux-foundation.org, hch@infradead.org Subject: Re: [PATCH v2 2/7] mm/vmalloc.c: add flags to mark vm_map_ram area Message-ID: References: <20221217015435.73889-1-bhe@redhat.com> <20221217015435.73889-3-bhe@redhat.com> 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.7 X-Stat-Signature: jkrc8irgwe8f483j5fsh49ot966yftg7 X-Rspam-User: X-Rspamd-Queue-Id: E15AC14000E X-Rspamd-Server: rspam06 X-HE-Tag: 1671452698-456341 X-HE-Meta: U2FsdGVkX18jppMXsYz3F9iz+TfshmTuKZhM8VUkZheqXM51c94mZGZtNPP4wo3Is+jScD0XqH2H6fnaSS8ucTNHwFoJUxof2PfeqUUGe1Co9/0UTjlEHo33yzYOkbzRZqjXSi7NgN9x7s0UDk+rqGVCEh5+DqhunTcBkGw6srghAHtmYIbfFihXYLfVrmgVBBEv6X8pqsj4KWpL78N02ntiaMsklX9CHQHAm5Qmkb/Y1gHeacjayAiKJhyWvFdkHvDTJZnhFHT0uq3tPj0/FoYvKX0QNxwwMpxlHGeh13V6H5bj31ASwHmTIVT5UehM9dQAW7+LJVq8X+7lptbf0HdDhZxaSdD9Ylgf2fq//q/DbA2Pnw/5ulip5BH0CVmIhq99FP9YmfK/Th8kdH2g3dyjZTHYr4NGuWgfNurHigGAah1MBxIHa2iuVNhsFBc/m3YA5Z87xUEoen28euPGzWe498aRV9AFlTcn1K32TuJSBUhBd7jGFUbK363pu3kvjnSfAYvXxD2DNLbx6in6xlmGsmoBqZeE1bX8QVv80GF+FYYUWbY9PiyE4FU7wXEmD5jBCqBtERMhVrXAAhZrimhNYuSodpxjZMGaFtOxA5mGWH1EX4Dz1dqTE0GYtDgA8nTG0/JV2uyQn2+77SgwM23rUJgD3yPTPH9BdTuudVog7smZHENOfPGN6zTvPdo+Fi0f4xonNwYdzQFE6A6Q7t3W7FcXM3x7MGMnfNIP5Op0bMc8R8yHAgCWxE7SlZxYcSTiFNsESt8stUKF2yB+jWTcVtNqbWO44gPYCDM/tJEjp9Q7KazHNpsp+XCI3R8FyQeCfbgEDUT6T+OV/2rcJSKmPVZBul84ZUHfLfsCT7KuceQiafCF5OaEt33YJYeUS2FAT6/LKNF/w4ojQQAcfp+nGKvS/ZmqLFgzwD1kHSlwlYIba7HZXFvSkYTsXnnZ01tRhLGnpDci3DMSsAP mew== 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 12/19/22 at 09:09am, Lorenzo Stoakes wrote: > On Mon, Dec 19, 2022 at 04:01:00PM +0800, Baoquan He wrote: > > On 12/17/22 at 11:44am, Lorenzo Stoakes wrote: > > > On Sat, Dec 17, 2022 at 09:54:30AM +0800, Baoquan He wrote: > > > > @@ -2229,8 +2236,12 @@ void vm_unmap_ram(const void *mem, unsigned int count) > > > > return; > > > > } > > > > > > > > - va = find_vmap_area(addr); > > > > + spin_lock(&vmap_area_lock); > > > > + va = __find_vmap_area((unsigned long)addr, &vmap_area_root); > > > > BUG_ON(!va); > > > > + if (va) > > > > + va->flags &= ~VMAP_RAM; > > > > + spin_unlock(&vmap_area_lock); > > > > debug_check_no_locks_freed((void *)va->va_start, > > > > (va->va_end - va->va_start)); > > > > free_unmap_vmap_area(va); > > > > > > Would it be better to perform the BUG_ON() after the lock is released? You > > > already check if va exists before unmasking so it's safe. > > > > It's a little unclear to me why we care BUG_ON() is performed before or > > after the lock released. We won't have a stable kernel after BUG_ON()(), > > right? > > BUG_ON()'s can be recoverable in user context and it would be a very simple > change that would not fundamentally alter anything to simply place the added > lines prior to the BUG_ON(). > > The code as-is doesn't really make sense anyway, you BUG_ON(!va) then check if > va is non-null, then immediately the function afterwards passes va around as if > it were not null, so I think it'd also be an aesthetic and logical improvement > :) In fact, I should not do the checking, but do the clearing anyway. If I change it as below, does it look better to you? diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 5e578563784a..369b848d097a 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -2253,8 +2253,7 @@ void vm_unmap_ram(const void *mem, unsigned int count) spin_lock(&vmap_area_lock); va = __find_vmap_area((unsigned long)addr, &vmap_area_root); BUG_ON(!va); - if (va) - va->flags &= ~VMAP_RAM; + va->flags &= ~VMAP_RAM; spin_unlock(&vmap_area_lock); debug_check_no_locks_freed((void *)va->va_start, (va->va_end - va->va_start)); > > > > > > > Also, do we want to clear VMAP_BLOCK here? > > > > I do, but I don't find a good place to clear VMAP_BLOCK. > > > > In v1, I tried to clear it in free_vmap_area_noflush() as below, > > Uladzislau dislikes it. So I remove it. My thinking is when we unmap and > > free the vmap area, the vmap_area is moved from vmap_area_root into > > &free_vmap_area_root. When we allocate a new vmap_area via > > alloc_vmap_area(), we will allocate a new va by kmem_cache_alloc_node(), > > the va->flags must be 0. Seems not initializing it to 0 won't impact > > thing. > > > > You are at this point clearing the VMAP_RAM flag though, so if it is unimportant > what the flags are after this call, why are you clearing this one? With my understanding, We had better do the clearing. Currently, from the code, not doing the clearing won't cause issue. If possible, I would like to clear it as below draft code. > > It is just a little confusing, I wonder whether the VMAP_BLOCK flag is necessary > at all, is it possible to just treat a non-VMAP_BLOCK VMAP_RAM area as if it > were simply a fully occupied block? Do we gain much by the distinction? Yeah, VMAP_BLOCK flag is necessary. vmap_block contains used region, or dirty/free regions. While the non-vmap_blcok vm_map_ram area is similar with the non vm_map_ram area. When reading out vm_map_ram regions, vmap_block regions need be treated differently. > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > index 5d3fd3e6fe09..d6f376060d83 100644 > > --- a/mm/vmalloc.c > > +++ b/mm/vmalloc.c > > @@ -1815,6 +1815,7 @@ static void free_vmap_area_noflush(struct vmap_area *va) > > > > spin_lock(&vmap_area_lock); > > unlink_va(va, &vmap_area_root); > > + va->flags = 0; > > spin_unlock(&vmap_area_lock); > > > > nr_lazy = atomic_long_add_return((va->va_end - va->va_start) >> > > > > > > > >