RE: [RFC PATCH 4/5] scsi: ufs: L2P map management for HPB read

[Daejun Park]

> > The data structure for map data request and L2P map uses mempool API,
> > minimizing allocation overhead while avoiding static allocation.
> Maybe one or two more sentences to explain the L2P framework:
> Each hpb lun maintains 2 "to-do" lists: 
>  - hpb->lh_inact_rgn - regions to be inactivated, and 
>  - hpb->lh_act_srgn - subregions to be activated
> Those lists are being checked on every resume and completion interrupt.
OK, I will add more description of L2P framework.

> > 
> > Signed-off-by: Daejun Park <daejun7.park@xxxxxxxxxxx>
> > ---
> > +       for (i = 0; i < hpb->pages_per_srgn; i++) {
> > +               mctx->m_page[i] = mempool_alloc(ufshpb_drv.ufshpb_page_pool,
> > +                                               GFP_KERNEL);
> > +               memset(page_address(mctx->m_page[i]), 0, PAGE_SIZE);
> Better move this memset after if (!mctx->m_page[i]).
> And maybe use clear_page instead?
OK, I will change the code.

> > +               if (!mctx->m_page[i]) {
> > +                       for (j = 0; j < i; j++)
> > +                               mempool_free(mctx->m_page[j],
> > +                                            ufshpb_drv.ufshpb_page_pool);
> > +                       goto release_ppn_dirty;
> > +               }
> > +static inline int ufshpb_add_region(struct ufshpb_lu *hpb,
> > +                                   struct ufshpb_region *rgn)
> > +{
> Maybe better describe what this function does - ufshpb_get_rgn_map_ctx ?
Yes, I think "ufshpb_get_rgn_map_ctx" is better name.

> > +       if (!list_empty(&rgn->list_lru_rgn)) {
> > +               if (ufshpb_check_issue_state_srgns(hpb, rgn)) {
> So if one of its subregions has inflight map request - you add it to the "starved" list?
> Why call it starved?
"starved list" was wrong name. I will change it to "postponed_evict_list".

> > +        * Since the region state change occurs only in the hpb task-work,
> > +        * the state of the region cannot HPB_RGN_INACTIVE at this point.
> > +        * The region state must be changed in the hpb task-work
> I think that you called this worker map_work?
Yes, "the hpb task-work" will be changed to the map_work.

> > +               spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
> > +               ret = ufshpb_add_region(hpb, rgn);
> If this is not an active region,
> Although the device indicated to activate a specific subregion, 
> You are activating all the subregions of that region.
> You should elaborate on that in your commit log,
> and explain why this is the correct activation course.
Yes, I'm going to change the code to activate only the subregions that are "activate state".

> get_unaligned instead of be16_to_cpu ?
Yes, I will change.

> > +
> > +       if (!data_seg_len) {
> data_seg_len should be DEV_DATA_SEG_LEN, and you should also check HPB_UPDATE_ALERT,
> which you might want to do here and not in ufshpb_may_field_valid
Yes, I will change.

> > +       switch (rsp_field->hpb_type) {
> > +       case HPB_RSP_REQ_REGION_UPDATE:
> > +               WARN_ON(data_seg_len != DEV_DATA_SEG_LEN);
> > +               ufshpb_rsp_req_region_update(hpb, rsp_field);
> > +               break;
> What about hpb dev reset - oper 0x2?
Yes, I will change.

> > +static void ufshpb_add_active_list(struct ufshpb_lu *hpb,
> > +                                  struct ufshpb_region *rgn,
> > +                                  struct ufshpb_subregion *srgn)
> > +{
> > +       if (!list_empty(&rgn->list_inact_rgn))
> > +               return;
> > +
> > +       if (!list_empty(&srgn->list_act_srgn)) {
> > +               list_move(&srgn->list_act_srgn, &hpb->lh_act_srgn);
> Why is this needed?
> Why updating this subregion position?
The "ufshpb_add_active_list()" is called from "ufshpb_run_active_subregion_list()" to retry activating subregion that failed to activate.
Therefore, it requeues the subregion to activate region list head.

> > @@ -195,8 +1047,15 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba
> > *hba, struct ufshpb_lu *hpb)
> >  release_srgn_table:
> >         for (i = 0; i < rgn_idx; i++) {
> >                 rgn = rgn_table + i;
> > -               if (rgn->srgn_tbl)
> > +               if (rgn->srgn_tbl) {
> > +                       for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt;
> > +                            srgn_idx++) {
> > +                               srgn = rgn->srgn_tbl + srgn_idx;
> > +                               if (srgn->mctx)
> How is it even possible that on init there is an active subregion?
> ufshpb_init_pinned_active_region does its own cleanup.
I will fix the duplicated cleanup codes.

> > +       hpb->m_page_cache = kmem_cache_create("ufshpb_m_page_cache",
> > +                         sizeof(struct page *) * hpb->pages_per_srgn,
> > +                         0, 0, NULL);
> What is the advantage in using an array of page pointers,
> Instead of a single pointer to pages_per_srgn?
To minimize memory fragmentation problem, I used pointer + single page rather than single array of pages. 

> > @@ -398,6 +1326,9 @@ static void ufshpb_resume(struct ufs_hba *hba)
> > 
> >                 dev_info(&hpb->hpb_lu_dev, "ufshpb resume");
> >                 ufshpb_set_state(hpb, HPB_PRESENT);
> > +               if (!ufshpb_is_empty_rsp_lists(hpb))
> > +                       queue_work(ufshpb_drv.ufshpb_wq, &hpb->map_work);
> Ahha - so you are using the ufs driver pm flows to poll your work queue.
> Why device recommendations isn't enough?
I don't understand this comment. The code resumes map_work that was stopped by PM during the map request.
Please explain your concerns.

Thanks,
Avri