| 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
| ||
|
Date: Tue, 09 Jun 2020 09:52:56 +0900
From: Daejun Park <daejun7.park@...sung.com>
To: Avri Altman <Avri.Altman@....com>,
Daejun Park <daejun7.park@...sung.com>,
ALIM AKHTAR <alim.akhtar@...sung.com>,
"jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
"martin.petersen@...cle.com" <martin.petersen@...cle.com>,
"asutoshd@...eaurora.org" <asutoshd@...eaurora.org>,
"beanhuo@...ron.com" <beanhuo@...ron.com>,
"stanley.chu@...iatek.com" <stanley.chu@...iatek.com>,
"cang@...eaurora.org" <cang@...eaurora.org>,
"bvanassche@....org" <bvanassche@....org>,
"tomas.winkler@...el.com" <tomas.winkler@...el.com>
CC: "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Sang-yoon Oh <sangyoon.oh@...sung.com>,
Sung-Jun Park <sungjun07.park@...sung.com>,
yongmyung lee <ymhungry.lee@...sung.com>,
Jinyoung CHOI <j-young.choi@...sung.com>,
Adel Choi <adel.choi@...sung.com>,
BoRam Shin <boram.shin@...sung.com>
Subject: RE: [RFC PATCH 4/5] scsi: ufs: L2P map management for HPB read
> > 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@...sung.com>
> > ---
> > + 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
Powered by blists - more mailing lists