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 C2942EE49A3 for ; Tue, 22 Aug 2023 14:06:27 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 739DB280030; Tue, 22 Aug 2023 10:06:26 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6C2D3280023; Tue, 22 Aug 2023 10:06:26 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 53C5D280030; Tue, 22 Aug 2023 10:06:26 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 409A7280023 for ; Tue, 22 Aug 2023 10:06:26 -0400 (EDT) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 1B1FBA0237 for ; Tue, 22 Aug 2023 14:06:26 +0000 (UTC) X-FDA: 81151915572.03.E1D73F7 Received: from mail-oo1-f46.google.com (mail-oo1-f46.google.com [209.85.161.46]) by imf19.hostedemail.com (Postfix) with ESMTP id 3616A1A003B for ; Tue, 22 Aug 2023 14:06:24 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=jJfQh1lO; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf19.hostedemail.com: domain of mjguzik@gmail.com designates 209.85.161.46 as permitted sender) smtp.mailfrom=mjguzik@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1692713184; 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=x0GIc82FQ8pRqPHwhaCl7OoSHr/0L8bu2OoU4MGFCuc=; b=EAZHpx1p9fI+/SycNQfkyC4DDEmIVFf41+Bsjd3rJqLVuWugk8kIYi1O63onA7gp4317Ho 2wUrbn4lw4IFceHR030BkHtEplwy0hPhsd+SDtezB/5ZvYVXTdLXv6957qD0HJxvH1FJpL ciamEuCJBmVBZIM4eNKyZBL5bH80j0s= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=jJfQh1lO; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf19.hostedemail.com: domain of mjguzik@gmail.com designates 209.85.161.46 as permitted sender) smtp.mailfrom=mjguzik@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1692713184; a=rsa-sha256; cv=none; b=uvvwR1txl4m8kHq4/BPuHsyrT99tGCJS6/jgucErVDADfQS8JRmrGKUkU3AwPNth7rboZx Q02+ITBA5utKu4/CvUTOkHRq1Ldoy8VDDx2Pc0iw8NQmhqEm8jxVddB4uEwbl4YW/J9LsQ UwzdVo/HrCvgCfCYebcCL7vhvl+yzjc= Received: by mail-oo1-f46.google.com with SMTP id 006d021491bc7-56c4c4e822eso2903217eaf.3 for ; Tue, 22 Aug 2023 07:06:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1692713183; x=1693317983; h=cc:to:subject:message-id:date:from:references:in-reply-to :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=x0GIc82FQ8pRqPHwhaCl7OoSHr/0L8bu2OoU4MGFCuc=; b=jJfQh1lOSZ525yjv8rIb1ikUFQID7CoA4PS6VjcGZ4wDSOtKTOUe3U+l1/xFaPJtqi y+Kcak7jvFDg5z86+Z6FSNfUGriXPOT9iMl/TrfsxBF5TU0vynjIXwZotK7B8HfiDUfT G7MVC0fTPadgbvfYa069m4FIKXFTcQVQ66GEfNbH69ruDWXQektFUTzikD9FZyXQOVFV K4B6snN+2fz+PhS9HkA0+VxLH6JFqQ7Xi0DxFHszO+O7RbUF8ae/g+XLoDBtTkCIGPPU 01dDS7nCpIyl6JxRo5Gm9A0KmHMtdTdGCoJNp1OWx7I9NBc1Ah54zJ4IG59MdKfLcTw+ AHMw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1692713183; x=1693317983; h=cc:to:subject:message-id:date:from:references:in-reply-to :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=x0GIc82FQ8pRqPHwhaCl7OoSHr/0L8bu2OoU4MGFCuc=; b=G4P0m2OJwJHiJqYl7eFfCXdAZc/wIuFFX3GsoChx4rHwajcBnEcwELTZq5LWA+OGZY nWqrvVWAf10uPW9Wk9ut86V6pUChdL01hLJdHlbaAR2NA3O6xJ5LZnN7YsonQDqXW0kT LkaeEaz5wcycVIzgODm2BdwlnR3MvubPKgv8wZbjpOtsscY5Zf3taWORDK8qQmYYesrX kisJZ3XzAQ2Hn7nDdDedL/KAwNdyXNH/tVNpFCcr78Vk3wgrCX0Y3mWcra6qJ1WWl0eo RJEwuPqWnLs5CzNMssB/Zamyl7tqAiyLLH7ghiTidk74tHltwnBQWv35zMfngV3poLJT h9Zw== X-Gm-Message-State: AOJu0YxTbpdtEbSEDAuYOAytQPSxdidnEPhR4ug1Bzp98EST1AhEpzsE Cij7xZEXW8u3QfAFqRvlcEBf62kFf3D3HUW2REg= X-Google-Smtp-Source: AGHT+IFnKO7eXAdo9UIr+jyQDrLzwsDW953+I/ef3r7+/3FY5d6jyUqkU9BrrclZIcIyfDnkkpSpX+z7hhwIX4PhGd4= X-Received: by 2002:a4a:654e:0:b0:566:ed69:422d with SMTP id z14-20020a4a654e000000b00566ed69422dmr10109935oog.7.1692713183150; Tue, 22 Aug 2023 07:06:23 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:ac9:6647:0:b0:4f0:1250:dd51 with HTTP; Tue, 22 Aug 2023 07:06:22 -0700 (PDT) In-Reply-To: References: <20230821202829.2163744-1-mjguzik@gmail.com> <20230821202829.2163744-2-mjguzik@gmail.com> From: Mateusz Guzik Date: Tue, 22 Aug 2023 16:06:22 +0200 Message-ID: Subject: Re: [PATCH 1/2] pcpcntr: add group allocation/free To: Vegard Nossum Cc: linux-kernel@vger.kernel.org, dennis@kernel.org, tj@kernel.org, cl@linux.com, akpm@linux-foundation.org, shakeelb@google.com, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 3616A1A003B X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: 4r9ojkoir7yd1t3z3yj71djno88a5msz X-HE-Tag: 1692713184-663195 X-HE-Meta: U2FsdGVkX1+NqLX3k4VhbSr+NXjCku2uJgkYjXhPgyuIj99VcxUVygRASlYC5KTD3+TKaJaTvyddDHXKuAOWipHkr4eaARq34qAkm2YIpZX0B/BtF/HgGMaxiIJ1F3Iqa+wqkiTp7JwrnMdLKFrBjPK6DP5HoLE0MOg11VDSHfEdW2dP8RaSSQlfSNX4YWpf+nqYgHjsNBuokZ4BYDBRzwb2vjxDXhLdYPjq0A4ZhL6ZAiSFxT5dgR8m48avLX41F9xywIZ2/VOofseP5QwdfPZIjyBaoa50oXKN2z7I07REByO3uiJzaF6pekYu5HPsj1Y5h3OoXG8xyPMkeKT6oSZm0AcDpLJIzgmYWv9n0N7I7OiWtIGKxFKjfGQPAf+FgJBT1cu52nNw+8qAN48hn1O89jS7kXnOoZXC9CI/6bsjm6AXqpbmYLr0dzgx4dajeijfpG4EDZKcRe8kirCRdhjCk5ds1R65RRKgTR00slGxcw7qufSPIMNUqiJxyfspM6nvGXFPbvEBD6H9oonO1rl05aZND57TE2a1JQG3uAiVJh/jGeUL6Aj30ODf6RHMefuYT7uYHd2tF2kpFd3fWSNA6G8kXWP8v2NVcPrURGx20pU/CZL/oFjZI54ZGj5btZTa7YAgZZIksWkt0pRSf6F6U7BlMXFig9IK5lbOejiLSU69vOjyvcyNFhh1xYvzeI0yS9dGqpuxq4frVA7RzYmk4rqbnsbzq7pvSxOcXJ9YWLKCZwFtWbQG5nBrZnZvvgMol1p5160smvOqmtMw1h5alLaA6ESoj0uxe+BLAE8c1wjRM6e1nfqAFiX7kXGul5smVKPewjLEnJ45jdZnmFzcZ+LNbc+AD/NBWpCsW8Als80UPBNKcgv5M3ZSdgmOPjm/V+gwRE+0ZnjIiD+ER/Of6mpV7HGqtjdil6VLy6Duwhp3+XgoCwzz5ixbHoYjvXihq7CwvN7D4dSH+Zf PI0Ywqlt s2xy+iPB+qN7ADPPGVMUd42LuCpLuh453hexaO9Pxnol7tIE1f7C4oXL1VCCXFxAoO92cgsbysnY9UV/KWYN0X34ultCIdn9E1jX7dq4GjS3LRZCqzvC4BtuqAApo3UDf3IUSI6wkkKHfW+vsdOTn7b4lymNHmfCrCbMjgaC1Cw6WdzbTOQsH8XIZCEhBkE84q9MohxEgn9zA+LrO/sCPh9rIKuYpxKC52chpOQQ97E6bE5BnOhkJs8FfGcZ1IWZ1EPncwR5MJHTUIXljsaOj4IgCRtGjzXysG1UPuWljYMdYoMXhUsVB077TmIlyfOW68xHAZxgfUmEm7lKYpUyCT34x5u1svqzOt9+Uo28OIqwfiwnm/VCUYsg7xTE2a8UknLAJ5i337RAjJ7IJcZttH9t/pg== 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 8/22/23, Vegard Nossum wrote: > > Testing out a review style with very detailed comments. Let me know if > you hate it. Review notes: > I do, very noisy and I don't think it adds any value. ;) If something like this becomes the norm I'm confident people are going to start skimming responses to their mail, as opposed to reading them. But then again, I had serious disagreement with review folk in the past, so... > On 8/21/23 22:28, Mateusz Guzik wrote: >> + counters = __alloc_percpu_gfp(sizeof(*counters) * count, >> + sizeof(*counters), gfp); > > The second argument here is the alignment. I see other callers using > __alignof__(type), which is what alloc_percpu_gfp() does as well. In > practice I think it shouldn't matter, but for clarity/consistency maybe > this should be __alignof__ as well? > Ye, I neglected to patch it up after a whipping out a PoC. > Presumably multiplication overflow is not an issue here as it is with > kmalloc and friends since the count can't be controlled by userspace. > I wanted to assert on the count being sensible to begin with, but there is no general "only assert with debug enabled" macro. Perhaps a warn_once + bail out would be preferred? >> + if (!counters) { >> + fbc[0].counters = NULL; >> return -ENOMEM; >> + } > > Checked that __alloc_percpu_gfp() returns NULL on failure. > > Checked that nothing else before this in the function needs cleanup. > > In the old code, fbc->count would have gotten initialized but it > shouldn't matter here, I think, as long as the counter is never activated. > > I'm not sure why only fbc[0].counters is set to NULL, should this happen > for all the "count" members? [PS: percpu_counter_destroy_many() below > has a check for fbc[0].counters] > Consumers looked fishy to me with zeroing the counter prior to calling the init func. I added the NULL assignment so that a call to destroy does nothing. > In summary, my only slight concern is sizeof(*counters) being passed as > the alignment to __alloc_percpu_gfp() when maybe it would be more > appropriate to pass __alignof__() -- not that it makes a difference at > runtime since both are 4 for s32. > Agreed, will patch later. > One other thing: I find it a bit odd that the "amount" parameter > (initial value) is s64 when the counters themselves are s32. Maybe just > a leftover from an old version? > I don't know the reasoning by the authors, but seems a clear case to me that the per-CPU counts were left int-sized to reduce memory usage and reduce deviation between the central counter and the real state. -- Mateusz Guzik