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;
IronPort-SDR: m+bGFo1yKtUTcTB3MZAv7XKs+/CZDsPCe9GtDUtNYDAgXpp/g96p0xE8ht7FOT39kTzpudJ0wU
 NEP5vYAokYEXZD2kMMQoQPDthjfSJgmBIqtFRFowE343SnJ5LIwwpt103pvv3+epuMNaRnCs5C
 kodOPCRfauYnXJ3HNuzZLXpgTmeCxwT4sNFZ2NX4gFy44KhtUke9rVV8lmVUylxfSE4oYwJyG5
 tqXDQyTgsx3xKi/luteCXrwBH6o+hrEgAYRHDCIaxPrVaimw/rGHeLFDABkS19x691C/MDqtDE
 Wjw=
From:   Avri Altman <Avri.Altman@wdc.com>
To:     Can Guo <cang@codeaurora.org>
CC:     "James E . J . Bottomley" <jejb@linux.vnet.ibm.com>,
        "Martin K . Petersen" <martin.petersen@oracle.com>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        Bart Van Assche <bvanassche@acm.org>,
        yongmyung lee <ymhungry.lee@samsung.com>,
        Daejun Park <daejun7.park@samsung.com>,
        "alim.akhtar@samsung.com" <alim.akhtar@samsung.com>,
        "asutoshd@codeaurora.org" <asutoshd@codeaurora.org>,
        Zang Leigang <zangleigang@hisilicon.com>,
        Avi Shchislowski <Avi.Shchislowski@wdc.com>,
        Bean Huo <beanhuo@micron.com>,
        "stanley.chu@mediatek.com" <stanley.chu@mediatek.com>
Subject: RE: [PATCH v6 04/10] scsi: ufshpb: Make eviction depends on region's
 reads
Thread-Topic: [PATCH v6 04/10] scsi: ufshpb: Make eviction depends on region's
 reads
Thread-Index: AQHXHvMB0UyOx5FZfEKuIKqQy+zc/qqUMx2AgAABiHA=
Date:   Thu, 25 Mar 2021 05:56:40 +0000
Message-ID: <DM6PR04MB6575CC008E355F965A1886E7FC629@DM6PR04MB6575.namprd04.prod.outlook.com>
References: <20210322081044.62003-1-avri.altman@wdc.com>
 <20210322081044.62003-5-avri.altman@wdc.com>
 <b06c0bcc3ea51ab7d6b8e5fb46ed6bdb@codeaurora.org>
In-Reply-To: <b06c0bcc3ea51ab7d6b8e5fb46ed6bdb@codeaurora.org>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 074175d7-4a83-4c0c-0b3b-08d8ef52c2b7
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Mar 2021 05:56:40.4464
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 0mWC1MrZ8HEJ3dCU/9AZUj6577syGdsb3MbWfnONlogFkaqDS/UZriUN3CtUVoAuFL79xYgah0GWswV7htyKVg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR04MB5017
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

>=20
> On 2021-03-22 16:10, Avri Altman wrote:
> > In host mode, eviction is considered an extreme measure.
> > verify that the entering region has enough reads, and the exiting
> > region has much less reads.
> >
> > Signed-off-by: Avri Altman <avri.altman@wdc.com>
> > ---
> >  drivers/scsi/ufs/ufshpb.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> > index a1519cbb4ce0..5e757220d66a 100644
> > --- a/drivers/scsi/ufs/ufshpb.c
> > +++ b/drivers/scsi/ufs/ufshpb.c
> > @@ -17,6 +17,7 @@
> >  #include "../sd.h"
> >
> >  #define ACTIVATION_THRESHOLD 8 /* 8 IOs */
> > +#define EVICTION_THRESHOLD (ACTIVATION_THRESHOLD << 5) /* 256 IOs
> */
> >
> >  /* memory management */
> >  static struct kmem_cache *ufshpb_mctx_cache;
> > @@ -1047,6 +1048,13 @@ static struct ufshpb_region
> > *ufshpb_victim_lru_info(struct ufshpb_lu *hpb)
> >               if (ufshpb_check_srgns_issue_state(hpb, rgn))
> >                       continue;
> >
> > +             /*
> > +              * in host control mode, verify that the exiting region
> > +              * has less reads
> > +              */
> > +             if (hpb->is_hcm && rgn->reads > (EVICTION_THRESHOLD >> 1)=
)
> > +                     continue;
> > +
> >               victim_rgn =3D rgn;
> >               break;
> >       }
> > @@ -1219,7 +1227,7 @@ static int ufshpb_issue_map_req(struct ufshpb_lu
> > *hpb,
> >
> >  static int ufshpb_add_region(struct ufshpb_lu *hpb, struct
> > ufshpb_region *rgn)
> >  {
> > -     struct ufshpb_region *victim_rgn;
> > +     struct ufshpb_region *victim_rgn =3D NULL;
> >       struct victim_select_info *lru_info =3D &hpb->lru_info;
> >       unsigned long flags;
> >       int ret =3D 0;
> > @@ -1246,7 +1254,15 @@ static int ufshpb_add_region(struct ufshpb_lu
> > *hpb, struct ufshpb_region *rgn)
> >                        * It is okay to evict the least recently used re=
gion,
> >                        * because the device could detect this region
> >                        * by not issuing HPB_READ
> > +                      *
> > +                      * in host control mode, verify that the entering
> > +                      * region has enough reads
> >                        */
> > +                     if (hpb->is_hcm && rgn->reads < EVICTION_THRESHOL=
D) {
> > +                             ret =3D -EACCES;
> > +                             goto out;
> > +                     }
> > +
>=20
> I cannot understand the logic behind this. A rgn which host chooses to
> activate,
> is in INACTIVE state now, if its rgn->reads < 256, then don't activate
> it.
> Could you please elaborate?
I am re-citing the commit log:
"In host mode, eviction is considered an extreme measure.
verify that the entering region has enough reads, and the exiting
region has much less reads."

Here comes to play the reads counter as a comparative index.
Max-active-regions has crossed, and to activate a region, you need to evict=
 another region.
But the activation threshold is relatively low, how do you know that you wi=
ll benefit more,
From the new region, than from the one you choose to evict?

Not to arbitrarily evict the "first" (LRU) region, like in device mode, we =
are looking for a solid
Reason for the new region to enter, and for the existing region to leave.
Otherwise, you will find yourself entering and existing the same region ove=
r and over,
Just threshing the active-list creating an unnecessary overhead by keep sen=
ding map requests.
For example, say the entering region has 4 reads, but the LRU region has 20=
0, and its reads keeps coming.
Is it the "correct" decision to evict a 200-reads region for a 4-reads regi=
on?
If you indeed evict this 200-reads region, you will evict another to put it=
 right back,
Over and over.

On the other hand, we are not hanging-on to "cold" regions, and inactivate =
them if there are no recent
Reads to that region - see the patch with the "Cold" timeout.

I agree that this can be elaborate to a more sophisticated policies - which=
 we tried.
For now, let's go with the simplest one - use thresholds for both the enter=
ing and exiting regions.

Thanks,
Avri
>=20
> Thanks,
> Can Guo.
>=20
> >                       victim_rgn =3D ufshpb_victim_lru_info(hpb);
> >                       if (!victim_rgn) {
> >                               dev_warn(&hpb->sdev_ufs_lu->sdev_dev,