linux-kernel - RE: [PATCH v6 04/10] scsi: ufshpb: Make eviction depends on region's reads

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DM6PR04MB6575CC008E355F965A1886E7FC629@DM6PR04MB6575.namprd04.prod.outlook.com>
Date:   Thu, 25 Mar 2021 05:56:40 +0000
From:   Avri Altman <Avri.Altman@....com>
To:     Can Guo <cang@...eaurora.org>
CC:     "James E . J . Bottomley" <jejb@...ux.vnet.ibm.com>,
        "Martin K . Petersen" <martin.petersen@...cle.com>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        Bart Van Assche <bvanassche@....org>,
        yongmyung lee <ymhungry.lee@...sung.com>,
        Daejun Park <daejun7.park@...sung.com>,
        "alim.akhtar@...sung.com" <alim.akhtar@...sung.com>,
        "asutoshd@...eaurora.org" <asutoshd@...eaurora.org>,
        Zang Leigang <zangleigang@...ilicon.com>,
        Avi Shchislowski <Avi.Shchislowski@....com>,
        Bean Huo <beanhuo@...ron.com>,
        "stanley.chu@...iatek.com" <stanley.chu@...iatek.com>
Subject: RE: [PATCH v6 04/10] scsi: ufshpb: Make eviction depends on region's
 reads

> 
> 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@....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 = 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 = NULL;
> >       struct victim_select_info *lru_info = &hpb->lru_info;
> >       unsigned long flags;
> >       int ret = 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 region,
> >                        * 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_THRESHOLD) {
> > +                             ret = -EACCES;
> > +                             goto out;
> > +                     }
> > +
> 
> 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 will 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 over and over,
Just threshing the active-list creating an unnecessary overhead by keep sending map requests.
For example, say the entering region has 4 reads, but the LRU region has 200, and its reads keeps coming.
Is it the "correct" decision to evict a 200-reads region for a 4-reads region?
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 entering and exiting regions.

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