Message-ID: <DM6PR04MB65757E84F80B654689FBAE05FC6A9@DM6PR04MB6575.namprd04.prod.outlook.com>
Date:   Wed, 17 Mar 2021 15:46:52 +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 v5 06/10] scsi: ufshpb: Add hpb dev reset response

> >> >> >>
> >> >> >> Just curious, directly doing below things inside ufshpb_rsp_upiu()
> >> >> >> does
> >> >> >> not
> >> >> >> seem a problem to me, does this really deserve a separate work?
> >> >> > I don't know, I never even consider of doing this.
> >> >> > The active region list may contain up to few thousands of regions -
> >> >> > It is not rare to see configurations that covers the entire device.
> >> >> >
> >> >>
> >> >> Yes, true, it can be a huge list. But what does the ops
> >> >> "HPB_RSP_DEV_RESET"
> >> >> really mean? The specs says "Device reset HPB Regions information",
> >> >> but
> >> >> I
> >> >> don't know what is really happening. Could you please elaborate?
> >> > It means that the device informs the host that the L2P cache is no
> >> > longer valid.
> >> > The spec doesn't say what to do in that case.
> >>
> >> Then it means that all the clean (without DIRTY flag set) HPB entries
> >> (ppns)
> >> in active rgns in host memory side may not be valid to the device
> >> anymore.
> >> Please correct me if I am wrong.
> >>
> >> > We thought that in host mode, it make sense to update all the active
> >> > regions.
> >>
> >> But current logic does not set the state of the sub-regions (in active
> >> regions) to
> >> INVALID, it only marks all active regions as UPDATE.
> >>
> >> Although one of subsequent read cmds shall put the sub-region back to
> >> activate_list,
> >> ufshpb_test_ppn_dirty() can still return false, thus these read cmds
> >> still think the
> >> ppns are valid and they shall move forward to send HPB Write Buffer
> >> (buffer id = 0x2,
> >> in case of HPB2.0) and HPB Read cmds.
> >>
> >> HPB Read cmds with invalid ppns will be treated as normal Read cmds by
> >> device as the
> >> specs says, but what would happen to HPB Write Buffer cmds (buffer id
> >> =
> >> 0x2, in case
> >> of HPB2.0) with invalid ppns? Can this be a real problem?
> > No need to control the ppn dirty / invalid state for this case.
> > The device send device reset so it is aware that all the L2P cache is
> > invalid.
> > Any HPB_READ is treated like normal READ10.
> >
> > Only once HPB-READ-BUFFER is completed,
> > the device will relate back to the physical address.
> 
> What about HPB-WRITE-BUFFER (buffer id = 0x2) cmds?
Same.
Oper 0x2 is a relative simple case.
The device is expected to manage some versioning framework not to be "fooled" by erroneous ppn.
There are some more challenging races that the device should meet.

Thanks,
Avri
> 
> Thanks,
> Can Guo.
> 
> >
> >>
> >> >
> >> > I think I will go with your suggestion.
> >> > Effectively, in host mode, since it is deactivating "cold" regions,
> >> > the lru list is kept relatively small, and contains only "hot" regions.
> >>
> >> hmm... I don't really have a idea on this, please go with whatever you
> >> and Daejun think is fine here.
> > I will take your advice and remove the worker.
> >
> >
> > Thanks,
> > Avri
> >
> >>
> >> Thanks,
> >> Can Guo.
> >>
> >> >
> >> > Thanks,
> >> > Avri
> >> >
> >> >>
> >> >> Thanks,
> >> >> Can Guo.
> >> >>
> >> >> > But yes, I can do that.
> >> >> > Better to get ack from Daejun first.
> >> >> >
> >> >> > Thanks,
> >> >> > Avri
> >> >> >
> >> >> >>
> >> >> >> Thanks,
> >> >> >> Can Guo.
> >> >> >>
> >> >> >> > +{
> >> >> >> > +     struct ufshpb_lu *hpb;
> >> >> >> > +     struct victim_select_info *lru_info;
> >> >> >> > +     struct ufshpb_region *rgn;
> >> >> >> > +     unsigned long flags;
> >> >> >> > +
> >> >> >> > +     hpb = container_of(work, struct ufshpb_lu,
> >> ufshpb_lun_reset_work);
> >> >> >> > +
> >> >> >> > +     lru_info = &hpb->lru_info;
> >> >> >> > +
> >> >> >> > +     spin_lock_irqsave(&hpb->rgn_state_lock, flags);
> >> >> >> > +
> >> >> >> > +     list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn)
> >> >> >> > +             set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags);
> >> >> >> > +
> >> >> >> > +     spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
> >> >> >> > +}
> >> >> >> > +
> >> >> >> >  static void ufshpb_normalization_work_handler(struct work_struct
> >> >> >> > *work)
> >> >> >> >  {
> >> >> >> >       struct ufshpb_lu *hpb;
> >> >> >> > @@ -1798,6 +1832,8 @@ static int ufshpb_alloc_region_tbl(struct
> >> >> >> > ufs_hba *hba, struct ufshpb_lu *hpb)
> >> >> >> >               } else {
> >> >> >> >                       rgn->rgn_state = HPB_RGN_INACTIVE;
> >> >> >> >               }
> >> >> >> > +
> >> >> >> > +             rgn->rgn_flags = 0;
> >> >> >> >       }
> >> >> >> >
> >> >> >> >       return 0;
> >> >> >> > @@ -2012,9 +2048,12 @@ static int ufshpb_lu_hpb_init(struct
> >> ufs_hba
> >> >> >> > *hba, struct ufshpb_lu *hpb)
> >> >> >> >       INIT_LIST_HEAD(&hpb->list_hpb_lu);
> >> >> >> >
> >> >> >> >       INIT_WORK(&hpb->map_work, ufshpb_map_work_handler);
> >> >> >> > -     if (hpb->is_hcm)
> >> >> >> > +     if (hpb->is_hcm) {
> >> >> >> >               INIT_WORK(&hpb->ufshpb_normalization_work,
> >> >> >> >                         ufshpb_normalization_work_handler);
> >> >> >> > +             INIT_WORK(&hpb->ufshpb_lun_reset_work,
> >> >> >> > +                       ufshpb_reset_work_handler);
> >> >> >> > +     }
> >> >> >> >
> >> >> >> >       hpb->map_req_cache =
> kmem_cache_create("ufshpb_req_cache",
> >> >> >> >                         sizeof(struct ufshpb_req), 0, 0, NULL);
> >> >> >> > @@ -2114,8 +2153,10 @@ static void
> ufshpb_discard_rsp_lists(struct
> >> >> >> > ufshpb_lu *hpb)
> >> >> >> >
> >> >> >> >  static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb)
> >> >> >> >  {
> >> >> >> > -     if (hpb->is_hcm)
> >> >> >> > +     if (hpb->is_hcm) {
> >> >> >> > +             cancel_work_sync(&hpb->ufshpb_lun_reset_work);
> >> >> >> >               cancel_work_sync(&hpb->ufshpb_normalization_work);
> >> >> >> > +     }
> >> >> >> >       cancel_work_sync(&hpb->map_work);
> >> >> >> >  }
> >> >> >> >
> >> >> >> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> >> >> >> > index 84598a317897..37c1b0ea0c0a 100644
> >> >> >> > --- a/drivers/scsi/ufs/ufshpb.h
> >> >> >> > +++ b/drivers/scsi/ufs/ufshpb.h
> >> >> >> > @@ -121,6 +121,7 @@ struct ufshpb_region {
> >> >> >> >       struct list_head list_lru_rgn;
> >> >> >> >       unsigned long rgn_flags;
> >> >> >> >  #define RGN_FLAG_DIRTY 0
> >> >> >> > +#define RGN_FLAG_UPDATE 1
> >> >> >> >
> >> >> >> >       /* region reads - for host mode */
> >> >> >> >       spinlock_t rgn_lock;
> >> >> >> > @@ -217,6 +218,7 @@ struct ufshpb_lu {
> >> >> >> >       /* for selecting victim */
> >> >> >> >       struct victim_select_info lru_info;
> >> >> >> >       struct work_struct ufshpb_normalization_work;
> >> >> >> > +     struct work_struct ufshpb_lun_reset_work;
> >> >> >> >
> >> >> >> >       /* pinned region information */
> >> >> >> >       u32 lu_pinned_start;

Powered by blists - more mailing lists