Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <984834578beba18fa5b0196a0d9e4327dc22cf73.camel@kernel.org>
Subject: Re: [PATCH V3 19/30] x86/sgx: Free up EPC pages directly to support
 large page ranges
From:   Jarkko Sakkinen <jarkko@kernel.org>
To:     Dave Hansen <dave.hansen@intel.com>,
        Reinette Chatre <reinette.chatre@intel.com>
Cc:     dave.hansen@linux.intel.com, tglx@linutronix.de, bp@alien8.de,
        luto@kernel.org, mingo@redhat.com, linux-sgx@vger.kernel.org,
        x86@kernel.org, seanjc@google.com, kai.huang@intel.com,
        cathy.zhang@intel.com, cedric.xing@intel.com,
        haitao.huang@intel.com, mark.shanahan@intel.com, hpa@zytor.com,
        linux-kernel@vger.kernel.org
Date:   Wed, 06 Apr 2022 09:35:39 +0300
In-Reply-To: <f8d92685-27dd-e7ef-ce0d-100a285aec4b@intel.com>
References: <cover.1648847675.git.reinette.chatre@intel.com>
         <b3a17e1ce7bcd14db3c28903e8a97f094998ae74.1648847675.git.reinette.chatre@intel.com>
         <Ykvrk4hYvBEnNOOl@kernel.org>
         <e118f4c6-7216-15f3-2bda-3a5851b0bcb1@intel.com>
         <f8d92685-27dd-e7ef-ce0d-100a285aec4b@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.42.4 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2022-04-05 at 10:25 -0700, Dave Hansen wrote:
> On 4/5/22 10:13, Reinette Chatre wrote:
> > > > +void sgx_direct_reclaim(void)
> > > > +{
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (sgx_should_reclaim(S=
GX_NR_LOW_PAGES))
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0sgx_reclaim_pages();
> > > > +}
> > > Please, instead open code this to both locations - not enough redunda=
ncy
> > > to be worth of new function. Causes only unnecessary cross-referencin=
g
> > > when maintaining. Otherwise, I agree with the idea.
> > >=20
> > hmmm, that means the heart of the reclaimer (sgx_reclaim_pages()) would=
 be
> > made available for direct use from everywhere in the driver. I will loo=
k into this.
>=20
> I like the change.=C2=A0 It's not about reducing code redundancy, it's ab=
out
> *describing* what the code does.=C2=A0 Each location could have:
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* Enter direct SGX recla=
im: */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (sgx_should_reclaim(SG=
X_NR_LOW_PAGES))
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0sgx_reclaim_pages();
>=20
> Or, it could just be:
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0sgx_direct_reclaim();
>=20
> Which also provides a logical choke point to add comments, like:
>=20
> /*
> =C2=A0* sgx_direct_reclaim() should be called in locations where SGX
> =C2=A0* memory resources might be low and might be needed in order
> =C2=A0* to make forward progress.
> =C2=A0*/
> void sgx_direct_reclaim(void)
> {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0...

Maybe cutting hairs but could it be "sgx_reclaim_direct"? Rationale
is easier grepping of reclaimer functions, e.g. when tracing.

BR, Jarkko