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 3B785E7716D for ; Wed, 4 Dec 2024 21:15:05 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A668C6B0082; Wed, 4 Dec 2024 16:15:04 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id A157E6B0083; Wed, 4 Dec 2024 16:15:04 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8DD7E6B0085; Wed, 4 Dec 2024 16:15:04 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 690226B0082 for ; Wed, 4 Dec 2024 16:15:04 -0500 (EST) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id E996A1A10CF for ; Wed, 4 Dec 2024 21:15:03 +0000 (UTC) X-FDA: 82858531560.02.93F76F3 Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf16.hostedemail.com (Postfix) with ESMTP id 0004E180017 for ; Wed, 4 Dec 2024 21:14:46 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=lei0AyrP; spf=pass (imf16.hostedemail.com: domain of chrisl@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=chrisl@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1733346886; 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=vyKPZZa37z1Xa/elYapuIXVYWrQ+GRebuqQbOTKSpFw=; b=sozcnV98R9TkUWOt64YPvFEMj1kZbaa9VIrXVHOZEvst55+dw77DgjFXa8pYXtWdtRl2ji Pa33AsHYqTKBZwSpYMmE/AcMwnUQK3UBJ5e4MUjhVzB5UK33jVX71hCJ1lYD7FLDpE6DTq eFUQbfisKGLDcQ6VJtU5huvBNwoP8OU= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1733346886; a=rsa-sha256; cv=none; b=goznPaRceA2QijE2br9ZE6DdpBayWMit4Ymr7twEeJhV/J8BYySoedJ5NfrfW1k49S57Ao GjQsLRkPPm87D6i81vebYc+b8IPL1ffc2AE1G8Zu5vcGL9ouqRN/o54dW8UGoYV/V0uBDI XMZFxA+oqYYCNNJzMrVgZ/cgpqpzsfY= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=lei0AyrP; spf=pass (imf16.hostedemail.com: domain of chrisl@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=chrisl@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 324A7A41DBA for ; Wed, 4 Dec 2024 21:13:09 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 20716C4CEE2 for ; Wed, 4 Dec 2024 21:15:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1733346901; bh=PP9riDGVMaArGyx4h4AGn8j7GwEl3x6Pboo0zhieNfQ=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=lei0AyrPlhHCJUTcmm2voR+SBkIAKLqlw36/7W6E2tB31MjAqSkttEQgZVdTgqH4O jShtr7ARbdctpMJZfbty13X9Q0LXc0fWQ6rETrP29RCQ6hcH0jiGG93pKGXWCnrMa2 rT5Y5XSC5Y3ACjnQq009AkJ29qmFD08z24WOltPviNr7CO3iMpbb2LcoU8vDzkj/rq cHWhZh8LdWbxdZ8mNRLmQ4Xn0qSP+4Q6Ki9GRvyK8yWXlnQEoFLLlw7DSk1bJUn79G 0AYsZwBX05i7Nuhn5/AyVOBn9ycPwenDaRaUGqdx1nCASltD9+tzcFRJHv1PpXhTAU WQ5MS3kqXyvXw== Received: by mail-yb1-f173.google.com with SMTP id 3f1490d57ef6-e3882273bdfso337157276.0 for ; Wed, 04 Dec 2024 13:15:01 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCXvQz9gRoCZdAPofq+9/rth4RFxQbjLClzT+w2be/dmvLor/0FtkUtVUXkRbDGKNixVyxBBILWPHw==@kvack.org X-Gm-Message-State: AOJu0YyRxkwBqBTRDarOmR29PH7Szp+e06osBQxjHQ6tPFeAy4U0bIXo niJjM7r59hHm1IEV1v6SZ+dYsrwYZW+Z0NFIYsJ5Bw4xsGUjt+iYq6bDbJ882/WbIXndgdsLdXY F+1xy9RdjfZ09VpZLaJnaK/D6Q0AaMasNAHO3xQ== X-Google-Smtp-Source: AGHT+IFbN/oBxB+gjk0+oluQjXjzSKrlvFaCzGobo+WMAjvS4w0CB48XlOf7h96U5Tdton4cm+nax9QC+aHH7L8ciNA= X-Received: by 2002:a05:690c:6e13:b0:6ee:a89e:af3b with SMTP id 00721157ae682-6efad16fb51mr112617817b3.9.1733346900383; Wed, 04 Dec 2024 13:15:00 -0800 (PST) MIME-Version: 1.0 References: <20241202184154.19321-1-ryncsn@gmail.com> <20241202184154.19321-4-ryncsn@gmail.com> In-Reply-To: From: Chris Li Date: Wed, 4 Dec 2024 13:14:49 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 3/4] mm/swap_cgroup: simplify swap cgroup definitions To: Yosry Ahmed Cc: Kairui Song , linux-mm@kvack.org, Andrew Morton , Hugh Dickins , "Huang, Ying" , Roman Gushchin , Shakeel Butt , Johannes Weiner , Barry Song , Michal Hocko , linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 0004E180017 X-Rspam-User: X-Rspamd-Server: rspam07 X-Stat-Signature: j931egz5greizx55trw8bz36up6cf4qh X-HE-Tag: 1733346886-436714 X-HE-Meta: U2FsdGVkX1+qmPUbM09d1oTK2d+BQCF6cRXlwnzsPv1Bo5NDBO+5uYOcug+DIA+ZUi131fEDv4GllznP+T50Ghoey/ZWZkwJT3f3TB4EppXzuPLF4hyQmCP+VBa7T3NOt3x8uXvaWjwMmVzqp1VYFQymKL6ZLGx7u8tJAZY85U6HBaWstDmtNkFAFrqDEmrY+HQYD0vB9GTDXbITQ1Do/vXApE3d3jsVkQUtekkmYR0ahWaneYtUcTPS2ohQJqlkQHLrYzYs4EcCzlzpCWmO/NGzkwOqkN+KHg3X/WxnDmi+wyIlIqVjauhvP/30WGeQuUEZ1Gp2aKuj3t47dYMcv0jFM2WuZphuLUSg1wHKGYlbbi4zPsw9JVtbUDNoOujYQAu6VfsybGieZkiuGWrgtVaNH3JmTkqZUx72TalblBrb+42h/Iu/VdLIppzYnIUl7wSYyLeceFX0xHZHTNpxGIBg/l/p4JtuTJJivTfBwBVvSJn4hfel1hfcoqiaZZZad71her8HeIv94dFMxEo3n8DXJdmMytMETN2PaiftMxOd6969cAwDBLMK8n9Ml7lyYxaWlvJvvnfabkp/L1dL/nl6X/DaF4DjxvZ/ZZ2BeUyV9WLi29JdaFxyhy4PAOfRhKRXG+bs732VD579UCSe5J2cfn8WE1ur9RYjCJzrrnuGJGlVhwgXY3XrS7MqZgzeKSdvfaRLbqwsmjWkzNoCplgBIC6Hq/Bxt3wiRpHxwk/+JZQTihFkXGFMLxip8lszIUMpCvzZodgMC4zHn6l0NqJlXl5Xou0QgqT5NSjZ/3WqFQr5epvxZsXn63hfwQgk6E/b8KXJbPl8WDS+TB0Z2c+/gj5EeXPR3J0Xq5jWpzOLmT7Wt1NGz9Qayg6cCJ11emhLBDjCkGqU5SQyXGY0OZD0Bc42B65k8Jre8e+6zyprG4QJ5dSak2wqoF1XqqD1wgnv/IpFKN6M0W9LrBr uB9udkaI uwKY8yXWoGfb4d4hepRrddpm5kWCNTrDUSBHXRUx8/7rDPa6fLeQyq5jKpsW8sFc8Ur4W3TcaEY2Un4QRa4DcsD/24FySN/z4WxoOJ4bxrzsT39J4AL5xwtuoAj0IPgxlhm7ju/mV97gd2PxCvFaeGqcw8ntbVZgnvYuD/WuAmyH8Py98GPefUvk+Xy6d4kSK804fHEuk5BAoJYV7ohqHXKdtIF9uMowm5XVsFOO9LoYnOHGHCFhhbUhqhL88wTdZbMqk/oQsZs4AtIrxxO7BJBmGZBpuTvxKsp6F+npUNd1ey/FKvHxSHwT9Hh3TPpLB2dIVddIthjOUPKc0x5wcEF8vTP0VsLIJTwYl4eALgYf0usyvGc5K7CfxDbUOmBM0SEq5e+fQtooUig/w3g3hw1fq9g== 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: List-Subscribe: List-Unsubscribe: On Mon, Dec 2, 2024 at 11:26=E2=80=AFAM Yosry Ahmed = wrote: > > On Mon, Dec 2, 2024 at 10:42=E2=80=AFAM Kairui Song wr= ote: > > > > From: Kairui Song > > > > Remove the intermediate struct swap_cgroup, it just a unsigned short > > wrapper, simplify the code. > > Did you actually remove the struct? It doesn't seem like it. Right, I did not see the struct get removed either. Anyway I will wait for the V2 to review again. Chris > > > > > Also zero the map on initialization to prevent unexpected behaviour as > > swap cgroup helpers are suppose to return 0 on error. > > All the callers lookup the id of an already swapped out page, so it > should never be uninitialized. Maybe we should WARN if the result of > the lookup is 0 in this case? > > > > > Signed-off-by: Kairui Song > > --- > > mm/swap_cgroup.c | 45 +++++++++++++++++++-------------------------- > > 1 file changed, 19 insertions(+), 26 deletions(-) > > > > diff --git a/mm/swap_cgroup.c b/mm/swap_cgroup.c > > index 1770b076f6b7..a76afdc3666a 100644 > > --- a/mm/swap_cgroup.c > > +++ b/mm/swap_cgroup.c > > @@ -12,14 +12,12 @@ struct swap_cgroup { > > }; > > > > struct swap_cgroup_ctrl { > > - struct swap_cgroup *map; > > + unsigned short *map; > > spinlock_t lock; > > }; > > > > static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES]; > > > > -#define SC_PER_PAGE (PAGE_SIZE/sizeof(struct swap_cgroup)) > > - > > /* > > * SwapCgroup implements "lookup" and "exchange" operations. > > * In typical usage, this swap_cgroup is accessed via memcg's charge/u= ncharge > > @@ -33,18 +31,6 @@ static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_= SWAPFILES]; > > * > > * TODO: we can push these buffers out to HIGHMEM. > > */ > > -static struct swap_cgroup *lookup_swap_cgroup(swp_entry_t ent, > > - struct swap_cgroup_ctrl **ctrlp= ) > > -{ > > - pgoff_t offset =3D swp_offset(ent); > > - struct swap_cgroup_ctrl *ctrl; > > - > > - ctrl =3D &swap_cgroup_ctrl[swp_type(ent)]; > > - if (ctrlp) > > - *ctrlp =3D ctrl; > > - return &ctrl->map[offset]; > > -} > > - > > /** > > * swap_cgroup_record - record mem_cgroup for a set of swap entries > > * @ent: the first swap entry to be recorded into > > @@ -58,20 +44,21 @@ unsigned short swap_cgroup_record(swp_entry_t ent, = unsigned short id, > > unsigned int nr_ents) > > { > > struct swap_cgroup_ctrl *ctrl; > > - struct swap_cgroup *sc; > > + unsigned short *map; > > unsigned short old; > > unsigned long flags; > > pgoff_t offset =3D swp_offset(ent); > > pgoff_t end =3D offset + nr_ents; > > > > - sc =3D lookup_swap_cgroup(ent, &ctrl); > > + ctrl =3D &swap_cgroup_ctrl[swp_type(ent)]; > > + map =3D ctrl->map; > > > > spin_lock_irqsave(&ctrl->lock, flags); > > - old =3D sc->id; > > - for (; offset < end; offset++, sc++) { > > - VM_BUG_ON(sc->id !=3D old); > > - sc->id =3D id; > > - } > > + old =3D map[offset]; > > + do { > > + VM_BUG_ON(map[offset] !=3D old); > > + map[offset] =3D id; > > + } while (++offset !=3D end); > > Why did you change the for loop here? > > > spin_unlock_irqrestore(&ctrl->lock, flags); > > > > return old; > > @@ -85,20 +72,26 @@ unsigned short swap_cgroup_record(swp_entry_t ent, = unsigned short id, > > */ > > unsigned short lookup_swap_cgroup_id(swp_entry_t ent) > > { > > + struct swap_cgroup_ctrl *ctrl; > > + > > if (mem_cgroup_disabled()) > > return 0; > > - return lookup_swap_cgroup(ent, NULL)->id; > > + > > + ctrl =3D &swap_cgroup_ctrl[swp_type(ent)]; > > + pgoff_t offset =3D swp_offset(ent); > > + > > + return READ_ONCE(ctrl->map[offset]); > > The READ_ONCE() does not exist today in lookup_swap_cgroup(). Why is it n= eeded? > > > } > > > > int swap_cgroup_swapon(int type, unsigned long max_pages) > > { > > - struct swap_cgroup *map; > > + void *map; > > struct swap_cgroup_ctrl *ctrl; > > > > if (mem_cgroup_disabled()) > > return 0; > > > > - map =3D vcalloc(max_pages, sizeof(struct swap_cgroup)); > > + map =3D vzalloc(max_pages * sizeof(unsigned short)); > > if (!map) > > goto nomem; > > > > @@ -117,7 +110,7 @@ int swap_cgroup_swapon(int type, unsigned long max_= pages) > > > > void swap_cgroup_swapoff(int type) > > { > > - struct swap_cgroup *map; > > + void *map; > > Why void? > > > struct swap_cgroup_ctrl *ctrl; > > > > if (mem_cgroup_disabled()) > > -- > > 2.47.0 > >