Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Nitin Gupta <nigupta@nvidia.com>
To:     "pintu@codeaurora.org" <pintu@codeaurora.org>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "iamjoonsoo.kim@lge.com" <iamjoonsoo.kim@lge.com>,
        "sh_def@163.com" <sh_def@163.com>,
        "mateusznosek0@gmail.com" <mateusznosek0@gmail.com>,
        "bhe@redhat.com" <bhe@redhat.com>,
        "vbabka@suse.cz" <vbabka@suse.cz>,
        "yzaikin@google.com" <yzaikin@google.com>,
        "keescook@chromium.org" <keescook@chromium.org>,
        "mcgrof@kernel.org" <mcgrof@kernel.org>,
        "mgorman@techsingularity.net" <mgorman@techsingularity.net>,
        "pintu.ping@gmail.com" <pintu.ping@gmail.com>
Subject: RE: [PATCH] mm/compaction: remove unused variable
 sysctl_compact_memory
Thread-Topic: [PATCH] mm/compaction: remove unused variable
 sysctl_compact_memory
Thread-Index: AQHXD411mHVMaaygcESwzSP7eEOUu6pxIlMggAEy8oCAADvyQA==
Date:   Wed, 3 Mar 2021 18:14:50 +0000
Message-ID: <BYAPR12MB3416A388F9635A0F8799E49BD8989@BYAPR12MB3416.namprd12.prod.outlook.com>
References: <1614707773-10725-1-git-send-email-pintu@codeaurora.org>
 <BYAPR12MB3416C9FD5D10AFB930E1C023D8999@BYAPR12MB3416.namprd12.prod.outlook.com>
 <d3a2ef132c8deee8ced530367c81479c@codeaurora.org>
In-Reply-To: <d3a2ef132c8deee8ced530367c81479c@codeaurora.org>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 8aaf3035-6960-4e7c-594c-08d8de703c59
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Mar 2021 18:14:50.2147
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 5XvimWbCuzlzU7cYn2eyXliFLIP/p2OA6uRAY1BkZkFEvM7mv7OhV3ru5ZQhh2BDq+QEbqLGiKBqwuEQLrE2xQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB4177
X-OriginatorOrg: Nvidia.com
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1;
        t=1614795292; bh=yxHZTb1EMC0NqfNR3MSde4iuUoriIXG1U04URZaKJxk=;
        h=ARC-Seal:ARC-Message-Signature:ARC-Authentication-Results:From:To:
         CC:Subject:Thread-Topic:Thread-Index:Date:Message-ID:References:
         In-Reply-To:Accept-Language:Content-Language:X-MS-Has-Attach:
         X-MS-TNEF-Correlator:authentication-results:x-originating-ip:
         x-ms-publictraffictype:x-ms-office365-filtering-correlation-id:
         x-ms-traffictypediagnostic:x-microsoft-antispam-prvs:x-header:
         x-ms-oob-tlc-oobclassifiers:x-ms-exchange-senderadcheck:
         x-microsoft-antispam:x-microsoft-antispam-message-info:
         x-forefront-antispam-report:x-ms-exchange-antispam-messagedata:
         x-ms-exchange-transport-forked:Content-Type:
         Content-Transfer-Encoding:MIME-Version:
         X-MS-Exchange-CrossTenant-AuthAs:
         X-MS-Exchange-CrossTenant-AuthSource:
         X-MS-Exchange-CrossTenant-Network-Message-Id:
         X-MS-Exchange-CrossTenant-originalarrivaltime:
         X-MS-Exchange-CrossTenant-fromentityheader:
         X-MS-Exchange-CrossTenant-id:X-MS-Exchange-CrossTenant-mailboxtype:
         X-MS-Exchange-CrossTenant-userprincipalname:
         X-MS-Exchange-Transport-CrossTenantHeadersStamped:X-OriginatorOrg;
        b=B22Q8YasibF+GC3jNHTv5ro0ggEwN/tl206k97NvjK+WP2kcpDpi3+SG/lKGiHWhm
         qiRBx3x0Z638my66lSOaYf/ILd2kFZnD0TW4Yep33do4iaLNAMKlhUk3FMjQ7+Q+JS
         WitCrKJ+y9d8d0OXZ9KnuhgiKTSiSnk+TB+f//dZcRch309lI5gQT0sY0SOAwKVM4e
         xLDbxzK+LiJZy4+Or2JcXpIGcU/Kb0VogDNkVBCnHbSM8d9ITk91nt4YWPJy1vQZqQ
         2Ltt6NRXZRC6ED8xFrti4YHh15/dDndfJr2X9PO+dLBkUtU1efC1UKx9LMePUxVsVo
         xrWBouIty861A==
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: owner-linux-mm@kvack.org <owner-linux-mm@kvack.org> On Behalf
> Of pintu@codeaurora.org
> Sent: Wednesday, March 3, 2021 6:34 AM
> To: Nitin Gupta <nigupta@nvidia.com>
> Cc: linux-kernel@vger.kernel.org; akpm@linux-foundation.org; linux-
> mm@kvack.org; linux-fsdevel@vger.kernel.org; iamjoonsoo.kim@lge.com;
> sh_def@163.com; mateusznosek0@gmail.com; bhe@redhat.com;
> vbabka@suse.cz; yzaikin@google.com; keescook@chromium.org;
> mcgrof@kernel.org; mgorman@techsingularity.net; pintu.ping@gmail.com
> Subject: Re: [PATCH] mm/compaction: remove unused variable
> sysctl_compact_memory
>=20
> External email: Use caution opening links or attachments
>=20
>=20
> On 2021-03-03 01:48, Nitin Gupta wrote:
> >> -----Original Message-----
> >> From: pintu=3Dcodeaurora.org@mg.codeaurora.org
> >> <pintu=3Dcodeaurora.org@mg.codeaurora.org> On Behalf Of Pintu Kumar
> >> Sent: Tuesday, March 2, 2021 9:56 AM
> >> To: linux-kernel@vger.kernel.org; akpm@linux-foundation.org; linux-
> >> mm@kvack.org; linux-fsdevel@vger.kernel.org; pintu@codeaurora.org;
> >> iamjoonsoo.kim@lge.com; sh_def@163.com;
> mateusznosek0@gmail.com;
> >> bhe@redhat.com; Nitin Gupta <nigupta@nvidia.com>; vbabka@suse.cz;
> >> yzaikin@google.com; keescook@chromium.org; mcgrof@kernel.org;
> >> mgorman@techsingularity.net
> >> Cc: pintu.ping@gmail.com
> >> Subject: [PATCH] mm/compaction: remove unused variable
> >> sysctl_compact_memory
> >>
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> The sysctl_compact_memory is mostly unsed in mm/compaction.c It just
> >> acts as a place holder for sysctl.
> >>
> >> Thus we can remove it from here and move the declaration directly in
> >> kernel/sysctl.c itself.
> >> This will also eliminate the extern declaration from header file.
> >
> >
> > I prefer keeping the existing pattern of listing all compaction
> > related tunables together in compaction.h:
> >
> >       extern int sysctl_compact_memory;
> >       extern unsigned int sysctl_compaction_proactiveness;
> >       extern int sysctl_extfrag_threshold;
> >       extern int sysctl_compact_unevictable_allowed;
> >
>=20
> Thanks Nitin for your review.
> You mean, you just wanted to retain this extern declaration ?
> Any real benefit of keeping this declaration if not used elsewhere ?
>=20

I see that sysctl_compaction_handler() doesn't use the sysctl value at all.
So, we can get rid of it completely as Vlastimil suggested.

> >
> >> No functionality is broken or changed this way.
> >>
> >> Signed-off-by: Pintu Kumar <pintu@codeaurora.org>
> >> Signed-off-by: Pintu Agarwal <pintu.ping@gmail.com>
> >> ---
> >>  include/linux/compaction.h | 1 -
> >>  kernel/sysctl.c            | 1 +
> >>  mm/compaction.c            | 3 ---
> >>  3 files changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> >> index
> >> ed4070e..4221888 100644
> >> --- a/include/linux/compaction.h
> >> +++ b/include/linux/compaction.h
> >> @@ -81,7 +81,6 @@ static inline unsigned long compact_gap(unsigned
> >> int
> >> order)  }
> >>
> >>  #ifdef CONFIG_COMPACTION
> >> -extern int sysctl_compact_memory;
> >>  extern unsigned int sysctl_compaction_proactiveness;  extern int
> >> sysctl_compaction_handler(struct ctl_table *table, int write,
> >>                         void *buffer, size_t *length, loff_t *ppos);
> >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c index c9fbdd8..66aff21
> >> 100644
> >> --- a/kernel/sysctl.c
> >> +++ b/kernel/sysctl.c
> >> @@ -198,6 +198,7 @@ static int max_sched_tunable_scaling =3D
> >> SCHED_TUNABLESCALING_END-1;  #ifdef CONFIG_COMPACTION  static int
> >> min_extfrag_threshold;  static int max_extfrag_threshold =3D 1000;
> >> +static int sysctl_compact_memory;
> >>  #endif
> >>
> >>  #endif /* CONFIG_SYSCTL */
> >> diff --git a/mm/compaction.c b/mm/compaction.c index
> 190ccda..ede2886
> >> 100644
> >> --- a/mm/compaction.c
> >> +++ b/mm/compaction.c
> >> @@ -2650,9 +2650,6 @@ static void compact_nodes(void)
> >>                 compact_node(nid);
> >>  }
> >>
> >> -/* The written value is actually unused, all memory is compacted */
> >> -int sysctl_compact_memory;
> >> -
> >
> >
> > Please retain this comment for the tunable.
>=20
> Sorry, I could not understand.
> You mean to say just retain this last comment and only remove the
> variable ?
> Again any real benefit you see in retaining this even if its not used?
>=20
>=20

You are just moving declaration of sysctl_compact_memory from compaction.c
to sysctl.c. So, I wanted the comment "... all memory is compacted" to be r=
etained
with the sysctl variable. Since you are now getting rid of this variable co=
mpletely,
this comment goes away too.

Thanks,
Nitin