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 C41D6C32793 for ; Wed, 18 Jan 2023 12:20:34 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 37F6B6B0071; Wed, 18 Jan 2023 07:20:34 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 32F926B0072; Wed, 18 Jan 2023 07:20:34 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1D0686B0074; Wed, 18 Jan 2023 07:20:34 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 0E2076B0071 for ; Wed, 18 Jan 2023 07:20:34 -0500 (EST) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id C4B3A1402C3 for ; Wed, 18 Jan 2023 12:20:33 +0000 (UTC) X-FDA: 80367827946.04.5460CAA Received: from mail-ej1-f41.google.com (mail-ej1-f41.google.com [209.85.218.41]) by imf09.hostedemail.com (Postfix) with ESMTP id F2B3F140003 for ; Wed, 18 Jan 2023 12:20:23 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=SlmHpYfE; spf=pass (imf09.hostedemail.com: domain of urezki@gmail.com designates 209.85.218.41 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=1674044424; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=TOfcHtXWrkPXq0b76hWR3k9czL7UIXL/o9UPLYecdBk=; b=tnlJdQ5zcllbiS41vya4z4LvnLFzNj8fYErdTUCsevLZRxWcJhPRnfMfXciu/IVrEiPAUh 8HlkreUus92gA7JF0FtwFIAJvvijIkkhP8Snt1aM/OWfI3jgOCw7tC8++b3Ht2ZJCRtuXu uMmGD2exTqzOpWIwanCMFlPPsZxYbOY= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=SlmHpYfE; spf=pass (imf09.hostedemail.com: domain of urezki@gmail.com designates 209.85.218.41 as permitted sender) smtp.mailfrom=urezki@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1674044424; a=rsa-sha256; cv=none; b=Dm8ArWeTXxvTb3kqpEBSBQBUu82AVCqdaEhuDtw82v+803DYhT1/G9y40AVoyjoagMKX7W NSsjOqUrrVaLEbQNJmHPiCyfd6BFkfZuak9i5Mn5ynvr6z4G+1rnLYKW8Joqo3IsIwI0wD PboPHLWE374QtS36RZK1FBi3yT0FARg= Received: by mail-ej1-f41.google.com with SMTP id ud5so82831634ejc.4 for ; Wed, 18 Jan 2023 04:20:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:date:from:from:to :cc:subject:date:message-id:reply-to; bh=TOfcHtXWrkPXq0b76hWR3k9czL7UIXL/o9UPLYecdBk=; b=SlmHpYfE7dkuQqubWiwGjH0eOF8uIfTOIGGSbkP6kO00y1JO/eNL838z5TqT8Obw09 ZTDNDs1eGxaCGITvM9plCkuhtPFh+90YeMMzRc3/PfBufX3mZd0FJQPoezbCsx3phFYw jPedj9kL7KhpK5E00bBbqZeSrIn8bkmN/CVU5H8DEn4OUEdcOP0ozml0KNcgMvSApyk2 jjOTXAdGYTWnv/WOEnnQik06k7H/xrWETK4YyBCYkFG0pTgiLwumV+SgIcM2eUC6hJvZ pQ4K6sdZlkPUAHVaazIa3D9hzXQT++Y4Bs5lrOrrcI+qmJbb90eoH/+t6JMLqsPAi69i JJrA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-transfer-encoding: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=TOfcHtXWrkPXq0b76hWR3k9czL7UIXL/o9UPLYecdBk=; b=6RAeS8ZpeZpSVSwSlWlmX54BEtYk8CjaLvxB6sLb+XW962aw5z4BJD0KyBL74YO8bU Pa4+raOVoAeEePaIETCn+sT5wFgca3wSSmb/KoD3Y9XG/R2ag1x/AtFvgaVd+42GfP21 RTQchYkUppcCJ+wg0jFiL9viiMe9ptHomsLodHvs9bKBzRIO5c1IHCMPN0Q6s3JigwKi llJWk51FVfn+oSYn+v4XJ+azadylBF/7qx12pCCwGJbTYSdOE0zV/Gfsk+tEgHdoSUVH JhbCrbQOtfyKk0gRAIiY3Y/SmnJaxIN47WzmWERO+IVHh2wxvBFqwFvPp/JyZ6tNtY9L m7Lw== X-Gm-Message-State: AFqh2kpSCETdvF3hXqrVDBAc3IjiHrSjA09dVuM2Ru/EytUqx9pWCrhL t1oY2vPQzj3z3J1pN8sCpvM= X-Google-Smtp-Source: AMrXdXuWVkzlYX4FwcpvWw6F2i3lWIfMMWBWR+hWQ8qr9Vwpcr1t3XYu5sbXe9abthOlo/gYrcX2Cg== X-Received: by 2002:a17:907:d490:b0:870:8e31:7e33 with SMTP id vj16-20020a170907d49000b008708e317e33mr7632430ejc.15.1674044422300; Wed, 18 Jan 2023 04:20:22 -0800 (PST) Received: from pc636 (host-90-233-205-199.mobileonline.telia.com. [90.233.205.199]) by smtp.gmail.com with ESMTPSA id l18-20020a1709060cd200b0084c723a626csm14421062ejh.209.2023.01.18.04.20.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 18 Jan 2023 04:20:21 -0800 (PST) From: Uladzislau Rezki X-Google-Original-From: Uladzislau Rezki Date: Wed, 18 Jan 2023 13:20:18 +0100 To: Baoquan He Cc: Uladzislau Rezki , linux-kernel@vger.kernel.org, linux-mm@kvack.org, 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=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: F2B3F140003 X-Stat-Signature: m4wjjfgr59oa1gzjd33oqzse1ypgqirz X-Rspam-User: X-HE-Tag: 1674044423-699559 X-HE-Meta: U2FsdGVkX1+HvYkzgoYnrj2LPEgUD7Jx3iQ9Qg3DySGTFWKQHS7w+siN6+h2js7jBb01+72jnVTe2CQh46sRozYLZggmAXrV1A4joHkESDDYpuKyL5cJ0cLgbhe/Nzb7oT6331wW+HsuPSgoAZvcbWnO1TiTKo30MQm1a6Hmr/sNvFnhz7JsLwQJ3H0y5eYWCVHATznICEyu/iouYm2B8BIzCmgzprgz4jSJZszaqXRCv6dHMlgal0ARmxJB809/RvCzzfHbKD9gDJ1N9soNU0uiLB6Nf+SU4N9xIJy0xyOZRgEVaAfFwX7apVF12jAPDzfLulolBzddKLIRUoShCiyOMF4rrPUfbCvd7TLJGACVTt6OY7juHJdC9KuA4PsjGACu6RQ4My5Am1fRAhokIkVfM+bfED7Xd3DM1iOjm2xEtf7pPPgVm1HxamAtAEKRpfvB6h+zrnx3lBZZd2NLdwfEo9RhvxjwOi4zDmrofWc8ncmzwsaVYyEFkbAifxN3KdwgJ6kqK7Sn9jy4cPIlfZBvR1hag097tOTqaB5hK6CRdPvwRcibdCGYeDkti6Lzz0PjXefe6rpV2Boci1uZl5WePcs7bpAOcdMdlaeLfN1gEN2bJWXUj/6pr2bskSLLNyllypJevEUNSOlne0wXHyhU6V83AAlCp41AqI6S+liO1ZjFNbV1XF+sfB2OutwtUq23I+VZb+3WInUdl4UZU0/VrXYLD83nWHF6OphgL9fvEc37lH0dJvTEEcaDT/UN02FosowYU/r4MHMP7lHJunwxSMdVT13N46WIIGozTK8JYEOFIAiFoVgKrKKSHKV/QFxsxHuOgpeVOTimsnY21RoiJsxh7g9KscaF+ZOT9hZsaOszNLJFO/fA8Rij/2DnAMMccF0DnV92GABPoH/qCcI8zX5h2U8Ijn+7k/L9MVjPEpN9ebhY1lhMrSW3124k0AQM5ufCpZmoF3/a3/n c7oeWibZ rXEC5VYJOmYZP2U74Zmwyljs5kDfRQ4ipQLUXyXSG/c6XGL5sKIA+kcjWDmkGPQ4pg8/8LeK2H4T4C35pxbs20XyvPm8iuj+dwvZNgwJpZSmZKAMBVONZj6TsY03G3paWKP7LsLxDUqZzAedKxmwnoTQJT/XRiiDJD2yD3pDRppXFmiZ0BjFErJVJEEOzj5Xw4xa+TgCaZlnQxuhZ6ZDuuJjJwA== 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, Jan 18, 2023 at 11:09:44AM +0800, Baoquan He wrote: > On 01/16/23 at 06:54pm, Uladzislau Rezki wrote: > > On Fri, Jan 13, 2023 at 11:55:07AM +0800, Baoquan He wrote: > > > Hi Uladzislau Rezkiļ¼Œ > > > > > > On 12/23/22 at 12:14pm, Baoquan He wrote: > > > > On 12/20/22 at 05:55pm, Uladzislau Rezki wrote: > > > ...... > > > > > spin_lock(&vmap_area_lock); > > > > > > insert_vmap_area(va, &vmap_area_root, &vmap_area_list); > > > > > > @@ -1887,6 +1889,10 @@ struct vmap_area *find_vmap_area(unsigned long addr) > > > > > > > > > > > > #define VMAP_BLOCK_SIZE (VMAP_BBMAP_BITS * PAGE_SIZE) > > > > > > > > > > > > +#define VMAP_RAM 0x1 > > > > > > +#define VMAP_BLOCK 0x2 > > > > > > +#define VMAP_FLAGS_MASK 0x3 > > > > > > > > > > > Maybe to rename a VMAP_BLOCK to something like VMAP_BLOCK_RESERVED or > > > > > VMAP_PER_CPU_BLOCK? > > > > > > > > Both VMAP_BLOCK or VMAP_PER_CPU_BLOCK look good to me, please see my > > > > explanation at below. > > > > > > > > > > > > > > > struct vmap_block_queue { > > > > > > spinlock_t lock; > > > > > > struct list_head free; > > > > > > @@ -1962,7 +1968,8 @@ static void *new_vmap_block(unsigned int order, gfp_t gfp_mask) > > > > > > > > > > > > va = alloc_vmap_area(VMAP_BLOCK_SIZE, VMAP_BLOCK_SIZE, > > > > > > VMALLOC_START, VMALLOC_END, > > > > > > - node, gfp_mask); > > > > > > + node, gfp_mask, > > > > > > + VMAP_RAM|VMAP_BLOCK); > > > > > > > > > > > A new_vmap_block() is for a per-cpu path. As far as i see the VMAP_BLOCK > > > > > flag is used to mark a VA that corresponds to a reserved per-cpu free area. > > > > > > > > > > Whereas a VMAP_RAM is for VA that was obtained over per-cpu path but > > > > > over alloc_vmap_area() thus a VA should be read out over "busy" tree > > > > > directly. > > > > > > Rethinking about the vmap->flags and the bit0->VMAP_RAM, > > > bit1->VMAP_BLOCK correspondence, it looks better to use bit0->VMAP_RAM > > > to indicate the vm_map_ram area, no matter how it's handled inside > > > vm_map_ram() interface; and use bit1->VMAP_BLOCK to mark out the special > > > vm_map_ram area which is further subdivided and managed by struct > > > vmap_block. With these, you can see that we can identify vm_map_ram area > > > and treat it as one type of vmalloc area, e.g in vread(), s_show(). > > > > > > Means when we are talking about vm_map_ram areas, we use > > > (vmap->flags & VMAP_RAM) to recognize them; when we need to > > > differentiate and handle vm_map_ram areas respectively, we use > > > (vmap->flags & VMAP_BLOCK) to pick out the area which is further managed > > > by vmap_block. Please help check if this is OK to you. > > > > > > > > > > > > > Why do you need to set here both VMAP_RAM and VMAP_BLOCK? > > > > > > > > My understanding is that the vm_map_ram area has two types, one is > > > > the vb percpu area via vb_alloc(), the other is allocated via > > > > alloc_vmap_area(). While both of them is got from vm_map_ram() > > > > interface, this is the main point that distinguishes the vm_map_ram area > > > > than the normal vmalloc area, and this makes vm_map_ram area not owning > > > > va->vm pointer. So here, I use flag VMAP_RAM to mark the vm_map_ram > > > > area, including the two types; meanwhile, I add VMAP_BLOCK to mark out > > > > the vb percpu area. > > > > > > > > I understand people could have different view about them, e.g as you > > > > said, use VMAP_RAM to mark the type of vm_map_ram area allocated through > > > > alloc_vmap_area(), while use VMAP_PER_CPU_BLOCK to mark vb percpu area > > > > from vb_alloc. In this way, we may need to rename VMAP_RAM to reflect > > > > the area allocated from alloc_vmap_area() only. Both is fine to me. > > > > > > > > > > > > > > > if (IS_ERR(va)) { > > > > > > kfree(vb); > > > > > > return ERR_CAST(va); > > > > > > @@ -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, > > > > > > > > > > > Agree with Lorenzo. BUG_ON() should be out of spinlock(). Furthermore > > > > > i think it makes sense to go with WARN_ON_ONCE() and do not kill a system. > > > > > Instead emit a warning and bailout. > > > > > > > > > > What do you think? Maybe separate patch for it? > > > > > > > > Agree, your patch looks great to me. Thanks. > > > > > > > > > > > > > > > (va->va_end - va->va_start)); > > > > > > free_unmap_vmap_area(va); > > > > > > @@ -2265,7 +2276,8 @@ void *vm_map_ram(struct page **pages, unsigned int count, int node) > > > > > > } else { > > > > > > struct vmap_area *va; > > > > > > va = alloc_vmap_area(size, PAGE_SIZE, > > > > > > - VMALLOC_START, VMALLOC_END, node, GFP_KERNEL); > > > > > > + VMALLOC_START, VMALLOC_END, > > > > > > + node, GFP_KERNEL, VMAP_RAM); > > > > > > if (IS_ERR(va)) > > > > > > return NULL; > > > > > > > > > > > > @@ -2505,7 +2517,7 @@ static struct vm_struct *__get_vm_area_node(unsigned long size, > > > > > > if (!(flags & VM_NO_GUARD)) > > > > > > size += PAGE_SIZE; > > > > > > > > > > > > - va = alloc_vmap_area(size, align, start, end, node, gfp_mask); > > > > > > + va = alloc_vmap_area(size, align, start, end, node, gfp_mask, 0); > > > > > > if (IS_ERR(va)) { > > > > > > kfree(area); > > > > > > return NULL; > > > > > > > > > > > I know we have already discussed the new parameter. But what if we just > > > > > use atomic_set operation to mark VA as either vmap-ram or vmap-block? > > > > > > As I replied at above, I take the vm_map_ram as one kind of vmalloc > > > area, and mark out the percpu vmap block handling of vm_map_ram area. > > > Seems the passing in the flags through function parameter is better. Not > > > sure if I got your suggestion correctly, and my code change is > > > appropriate. I have sent v3 according to your and Lorenzo's comments and > > > suggestion, and my rethinking after reading your words. I make some > > > adjustment to try to remove misundersanding or confusion when reading > > > patch and code. Please help check if it's OK. > > > > > OK, if we decided to go with a parameter it is OK, it is not a big deal > > and complexity. If needed it can be adjusted later on if there is a > > need. > > My preference for function parameter passing is we don't need do the > atomic reading when we want to check va->flags. However, in va->flags > setting side, atomic_set() code is simpler than function parameter. > > flags = atomic_read(&va->flags); > if (flags & VMAP_RAM) { > > } > > I checked code, and feel it doesn't have much difference, so keep the > current code. If there's other thing I didn't think of, we can still > change. Thanks. > That is fine. -- Uladzislau Rezki