| Message ID | 20210302132503.224670-6-avri.altman@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Host control mode to HPB | expand |
On 2021-03-02 21:24, Avri Altman wrote: > I host mode, the host is expected to send HPB-WRITE-BUFFER with In host mode, > buffer-id = 0x1 when it inactivates a region. > > Use the map-requests pool as there is no point in assigning a > designated cache for umap-requests. > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > --- > drivers/scsi/ufs/ufshpb.c | 14 ++++++++++++++ > drivers/scsi/ufs/ufshpb.h | 1 + > 2 files changed, 15 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c > index 6f4fd22eaf2f..0744feb4d484 100644 > --- a/drivers/scsi/ufs/ufshpb.c > +++ b/drivers/scsi/ufs/ufshpb.c > @@ -907,6 +907,7 @@ static int ufshpb_execute_umap_req(struct ufshpb_lu > *hpb, > > blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_umap_req_compl_fn); > > + hpb->stats.umap_req_cnt++; > return 0; > } > > @@ -1103,6 +1104,12 @@ static int ufshpb_issue_umap_req(struct > ufshpb_lu *hpb, > return -EAGAIN; > } > > +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, > + struct ufshpb_region *rgn) > +{ > + return ufshpb_issue_umap_req(hpb, rgn); > +} > + > static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) > { > return ufshpb_issue_umap_req(hpb, NULL); > @@ -1115,6 +1122,10 @@ static void __ufshpb_evict_region(struct > ufshpb_lu *hpb, > struct ufshpb_subregion *srgn; > int srgn_idx; > > + No need of this blank line. Regards, Can Guo. > + if (hpb->is_hcm && ufshpb_issue_umap_single_req(hpb, rgn)) > + return; > + > lru_info = &hpb->lru_info; > > dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", > rgn->rgn_idx); > @@ -1855,6 +1866,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); > ufshpb_sysfs_attr_show_func(rb_active_cnt); > ufshpb_sysfs_attr_show_func(rb_inactive_cnt); > ufshpb_sysfs_attr_show_func(map_req_cnt); > +ufshpb_sysfs_attr_show_func(umap_req_cnt); > > static struct attribute *hpb_dev_stat_attrs[] = { > &dev_attr_hit_cnt.attr, > @@ -1863,6 +1875,7 @@ static struct attribute *hpb_dev_stat_attrs[] = { > &dev_attr_rb_active_cnt.attr, > &dev_attr_rb_inactive_cnt.attr, > &dev_attr_map_req_cnt.attr, > + &dev_attr_umap_req_cnt.attr, > NULL, > }; > > @@ -1978,6 +1991,7 @@ static void ufshpb_stat_init(struct ufshpb_lu > *hpb) > hpb->stats.rb_active_cnt = 0; > hpb->stats.rb_inactive_cnt = 0; > hpb->stats.map_req_cnt = 0; > + hpb->stats.umap_req_cnt = 0; > } > > static void ufshpb_param_init(struct ufshpb_lu *hpb) > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > index bd4308010466..84598a317897 100644 > --- a/drivers/scsi/ufs/ufshpb.h > +++ b/drivers/scsi/ufs/ufshpb.h > @@ -186,6 +186,7 @@ struct ufshpb_stats { > u64 rb_inactive_cnt; > u64 map_req_cnt; > u64 pre_req_cnt; > + u64 umap_req_cnt; > }; > > struct ufshpb_lu {
> > On 2021-03-02 21:24, Avri Altman wrote: > > I host mode, the host is expected to send HPB-WRITE-BUFFER with > > In host mode, Done. > > static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) > > { > > return ufshpb_issue_umap_req(hpb, NULL); > > @@ -1115,6 +1122,10 @@ static void __ufshpb_evict_region(struct > > ufshpb_lu *hpb, > > struct ufshpb_subregion *srgn; > > int srgn_idx; > > > > + > > No need of this blank line. Done. Thanks, Avri > > Regards, > Can Guo.
On 2021-03-02 21:24, Avri Altman wrote: > I host mode, the host is expected to send HPB-WRITE-BUFFER with > buffer-id = 0x1 when it inactivates a region. > > Use the map-requests pool as there is no point in assigning a > designated cache for umap-requests. > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > --- > drivers/scsi/ufs/ufshpb.c | 14 ++++++++++++++ > drivers/scsi/ufs/ufshpb.h | 1 + > 2 files changed, 15 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c > index 6f4fd22eaf2f..0744feb4d484 100644 > --- a/drivers/scsi/ufs/ufshpb.c > +++ b/drivers/scsi/ufs/ufshpb.c > @@ -907,6 +907,7 @@ static int ufshpb_execute_umap_req(struct ufshpb_lu > *hpb, > > blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_umap_req_compl_fn); > > + hpb->stats.umap_req_cnt++; > return 0; > } > > @@ -1103,6 +1104,12 @@ static int ufshpb_issue_umap_req(struct > ufshpb_lu *hpb, > return -EAGAIN; > } > > +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, > + struct ufshpb_region *rgn) > +{ > + return ufshpb_issue_umap_req(hpb, rgn); > +} > + > static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) > { > return ufshpb_issue_umap_req(hpb, NULL); > @@ -1115,6 +1122,10 @@ static void __ufshpb_evict_region(struct > ufshpb_lu *hpb, > struct ufshpb_subregion *srgn; > int srgn_idx; > > + > + if (hpb->is_hcm && ufshpb_issue_umap_single_req(hpb, rgn)) __ufshpb_evict_region() is called with rgn_state_lock held and IRQ disabled, when ufshpb_issue_umap_single_req() invokes blk_execute_rq_nowait(), below warning shall pop up every time, fix it? void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk, struct request *rq, int at_head, rq_end_io_fn *done) { WARN_ON(irqs_disabled()); ... Thanks. Can Guo. > + return; > + > lru_info = &hpb->lru_info; > > dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", > rgn->rgn_idx); > @@ -1855,6 +1866,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); > ufshpb_sysfs_attr_show_func(rb_active_cnt); > ufshpb_sysfs_attr_show_func(rb_inactive_cnt); > ufshpb_sysfs_attr_show_func(map_req_cnt); > +ufshpb_sysfs_attr_show_func(umap_req_cnt); > > static struct attribute *hpb_dev_stat_attrs[] = { > &dev_attr_hit_cnt.attr, > @@ -1863,6 +1875,7 @@ static struct attribute *hpb_dev_stat_attrs[] = { > &dev_attr_rb_active_cnt.attr, > &dev_attr_rb_inactive_cnt.attr, > &dev_attr_map_req_cnt.attr, > + &dev_attr_umap_req_cnt.attr, > NULL, > }; > > @@ -1978,6 +1991,7 @@ static void ufshpb_stat_init(struct ufshpb_lu > *hpb) > hpb->stats.rb_active_cnt = 0; > hpb->stats.rb_inactive_cnt = 0; > hpb->stats.map_req_cnt = 0; > + hpb->stats.umap_req_cnt = 0; > } > > static void ufshpb_param_init(struct ufshpb_lu *hpb) > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > index bd4308010466..84598a317897 100644 > --- a/drivers/scsi/ufs/ufshpb.h > +++ b/drivers/scsi/ufs/ufshpb.h > @@ -186,6 +186,7 @@ struct ufshpb_stats { > u64 rb_inactive_cnt; > u64 map_req_cnt; > u64 pre_req_cnt; > + u64 umap_req_cnt; > }; > > struct ufshpb_lu {
On 2021-03-15 12:02, Can Guo wrote: > On 2021-03-02 21:24, Avri Altman wrote: >> I host mode, the host is expected to send HPB-WRITE-BUFFER with >> buffer-id = 0x1 when it inactivates a region. >> >> Use the map-requests pool as there is no point in assigning a >> designated cache for umap-requests. >> >> Signed-off-by: Avri Altman <avri.altman@wdc.com> >> --- >> drivers/scsi/ufs/ufshpb.c | 14 ++++++++++++++ >> drivers/scsi/ufs/ufshpb.h | 1 + >> 2 files changed, 15 insertions(+) >> >> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c >> index 6f4fd22eaf2f..0744feb4d484 100644 >> --- a/drivers/scsi/ufs/ufshpb.c >> +++ b/drivers/scsi/ufs/ufshpb.c >> @@ -907,6 +907,7 @@ static int ufshpb_execute_umap_req(struct >> ufshpb_lu *hpb, >> >> blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_umap_req_compl_fn); >> >> + hpb->stats.umap_req_cnt++; >> return 0; >> } >> >> @@ -1103,6 +1104,12 @@ static int ufshpb_issue_umap_req(struct >> ufshpb_lu *hpb, >> return -EAGAIN; >> } >> >> +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, >> + struct ufshpb_region *rgn) >> +{ >> + return ufshpb_issue_umap_req(hpb, rgn); >> +} >> + >> static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) >> { >> return ufshpb_issue_umap_req(hpb, NULL); >> @@ -1115,6 +1122,10 @@ static void __ufshpb_evict_region(struct >> ufshpb_lu *hpb, >> struct ufshpb_subregion *srgn; >> int srgn_idx; >> >> + >> + if (hpb->is_hcm && ufshpb_issue_umap_single_req(hpb, rgn)) > > __ufshpb_evict_region() is called with rgn_state_lock held and IRQ > disabled, > when ufshpb_issue_umap_single_req() invokes blk_execute_rq_nowait(), > below > warning shall pop up every time, fix it? > > void blk_execute_rq_nowait(struct request_queue *q, struct gendisk > *bd_disk, > struct request *rq, int at_head, > rq_end_io_fn *done) > { > WARN_ON(irqs_disabled()); > ... > Moreover, since we are here with rgn_state_lock held and IRQ disabled, in ufshpb_get_req(), rq = kmem_cache_alloc(hpb->map_req_cache, GFP_KERNEL) has the GFP_KERNEL flag, scheduling while atomic??? Can Guo. > Thanks. > Can Guo. > >> + return; >> + >> lru_info = &hpb->lru_info; >> >> dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", >> rgn->rgn_idx); >> @@ -1855,6 +1866,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); >> ufshpb_sysfs_attr_show_func(rb_active_cnt); >> ufshpb_sysfs_attr_show_func(rb_inactive_cnt); >> ufshpb_sysfs_attr_show_func(map_req_cnt); >> +ufshpb_sysfs_attr_show_func(umap_req_cnt); >> >> static struct attribute *hpb_dev_stat_attrs[] = { >> &dev_attr_hit_cnt.attr, >> @@ -1863,6 +1875,7 @@ static struct attribute *hpb_dev_stat_attrs[] = >> { >> &dev_attr_rb_active_cnt.attr, >> &dev_attr_rb_inactive_cnt.attr, >> &dev_attr_map_req_cnt.attr, >> + &dev_attr_umap_req_cnt.attr, >> NULL, >> }; >> >> @@ -1978,6 +1991,7 @@ static void ufshpb_stat_init(struct ufshpb_lu >> *hpb) >> hpb->stats.rb_active_cnt = 0; >> hpb->stats.rb_inactive_cnt = 0; >> hpb->stats.map_req_cnt = 0; >> + hpb->stats.umap_req_cnt = 0; >> } >> >> static void ufshpb_param_init(struct ufshpb_lu *hpb) >> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h >> index bd4308010466..84598a317897 100644 >> --- a/drivers/scsi/ufs/ufshpb.h >> +++ b/drivers/scsi/ufs/ufshpb.h >> @@ -186,6 +186,7 @@ struct ufshpb_stats { >> u64 rb_inactive_cnt; >> u64 map_req_cnt; >> u64 pre_req_cnt; >> + u64 umap_req_cnt; >> }; >> >> struct ufshpb_lu {
> >> --- > >> drivers/scsi/ufs/ufshpb.c | 14 ++++++++++++++ > >> drivers/scsi/ufs/ufshpb.h | 1 + > >> 2 files changed, 15 insertions(+) > >> > >> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c > >> index 6f4fd22eaf2f..0744feb4d484 100644 > >> --- a/drivers/scsi/ufs/ufshpb.c > >> +++ b/drivers/scsi/ufs/ufshpb.c > >> @@ -907,6 +907,7 @@ static int ufshpb_execute_umap_req(struct > >> ufshpb_lu *hpb, > >> > >> blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_umap_req_compl_fn); > >> > >> + hpb->stats.umap_req_cnt++; > >> return 0; > >> } > >> > >> @@ -1103,6 +1104,12 @@ static int ufshpb_issue_umap_req(struct > >> ufshpb_lu *hpb, > >> return -EAGAIN; > >> } > >> > >> +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, > >> + struct ufshpb_region *rgn) > >> +{ > >> + return ufshpb_issue_umap_req(hpb, rgn); > >> +} > >> + > >> static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) > >> { > >> return ufshpb_issue_umap_req(hpb, NULL); > >> @@ -1115,6 +1122,10 @@ static void __ufshpb_evict_region(struct > >> ufshpb_lu *hpb, > >> struct ufshpb_subregion *srgn; > >> int srgn_idx; > >> > >> + > >> + if (hpb->is_hcm && ufshpb_issue_umap_single_req(hpb, rgn)) > > > > __ufshpb_evict_region() is called with rgn_state_lock held and IRQ > > disabled, > > when ufshpb_issue_umap_single_req() invokes blk_execute_rq_nowait(), > > below > > warning shall pop up every time, fix it? > > > > void blk_execute_rq_nowait(struct request_queue *q, struct gendisk > > *bd_disk, > > struct request *rq, int at_head, > > rq_end_io_fn *done) > > { > > WARN_ON(irqs_disabled()); > > ... > > > > Moreover, since we are here with rgn_state_lock held and IRQ disabled, > in ufshpb_get_req(), rq = kmem_cache_alloc(hpb->map_req_cache, > GFP_KERNEL) > has the GFP_KERNEL flag, scheduling while atomic??? I think your comment applies to ufshpb_issue_umap_all_req as well, Which is called from slave_configure/scsi_add_lun. Since the host-mode series is utilizing the framework laid by the device-mode, Maybe you can add this comment to Daejun's last version? Thanks, Avri > > Can Guo. > > > Thanks. > > Can Guo. > > > >> + return; > >> + > >> lru_info = &hpb->lru_info; > >> > >> dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", > >> rgn->rgn_idx); > >> @@ -1855,6 +1866,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); > >> ufshpb_sysfs_attr_show_func(rb_active_cnt); > >> ufshpb_sysfs_attr_show_func(rb_inactive_cnt); > >> ufshpb_sysfs_attr_show_func(map_req_cnt); > >> +ufshpb_sysfs_attr_show_func(umap_req_cnt); > >> > >> static struct attribute *hpb_dev_stat_attrs[] = { > >> &dev_attr_hit_cnt.attr, > >> @@ -1863,6 +1875,7 @@ static struct attribute *hpb_dev_stat_attrs[] = > >> { > >> &dev_attr_rb_active_cnt.attr, > >> &dev_attr_rb_inactive_cnt.attr, > >> &dev_attr_map_req_cnt.attr, > >> + &dev_attr_umap_req_cnt.attr, > >> NULL, > >> }; > >> > >> @@ -1978,6 +1991,7 @@ static void ufshpb_stat_init(struct ufshpb_lu > >> *hpb) > >> hpb->stats.rb_active_cnt = 0; > >> hpb->stats.rb_inactive_cnt = 0; > >> hpb->stats.map_req_cnt = 0; > >> + hpb->stats.umap_req_cnt = 0; > >> } > >> > >> static void ufshpb_param_init(struct ufshpb_lu *hpb) > >> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > >> index bd4308010466..84598a317897 100644 > >> --- a/drivers/scsi/ufs/ufshpb.h > >> +++ b/drivers/scsi/ufs/ufshpb.h > >> @@ -186,6 +186,7 @@ struct ufshpb_stats { > >> u64 rb_inactive_cnt; > >> u64 map_req_cnt; > >> u64 pre_req_cnt; > >> + u64 umap_req_cnt; > >> }; > >> > >> struct ufshpb_lu {
On 2021-03-16 16:30, Avri Altman wrote: >> >> --- >> >> drivers/scsi/ufs/ufshpb.c | 14 ++++++++++++++ >> >> drivers/scsi/ufs/ufshpb.h | 1 + >> >> 2 files changed, 15 insertions(+) >> >> >> >> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c >> >> index 6f4fd22eaf2f..0744feb4d484 100644 >> >> --- a/drivers/scsi/ufs/ufshpb.c >> >> +++ b/drivers/scsi/ufs/ufshpb.c >> >> @@ -907,6 +907,7 @@ static int ufshpb_execute_umap_req(struct >> >> ufshpb_lu *hpb, >> >> >> >> blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_umap_req_compl_fn); >> >> >> >> + hpb->stats.umap_req_cnt++; >> >> return 0; >> >> } >> >> >> >> @@ -1103,6 +1104,12 @@ static int ufshpb_issue_umap_req(struct >> >> ufshpb_lu *hpb, >> >> return -EAGAIN; >> >> } >> >> >> >> +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, >> >> + struct ufshpb_region *rgn) >> >> +{ >> >> + return ufshpb_issue_umap_req(hpb, rgn); >> >> +} >> >> + >> >> static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) >> >> { >> >> return ufshpb_issue_umap_req(hpb, NULL); >> >> @@ -1115,6 +1122,10 @@ static void __ufshpb_evict_region(struct >> >> ufshpb_lu *hpb, >> >> struct ufshpb_subregion *srgn; >> >> int srgn_idx; >> >> >> >> + >> >> + if (hpb->is_hcm && ufshpb_issue_umap_single_req(hpb, rgn)) >> > >> > __ufshpb_evict_region() is called with rgn_state_lock held and IRQ >> > disabled, >> > when ufshpb_issue_umap_single_req() invokes blk_execute_rq_nowait(), >> > below >> > warning shall pop up every time, fix it? >> > >> > void blk_execute_rq_nowait(struct request_queue *q, struct gendisk >> > *bd_disk, >> > struct request *rq, int at_head, >> > rq_end_io_fn *done) >> > { >> > WARN_ON(irqs_disabled()); >> > ... >> > >> >> Moreover, since we are here with rgn_state_lock held and IRQ disabled, >> in ufshpb_get_req(), rq = kmem_cache_alloc(hpb->map_req_cache, >> GFP_KERNEL) >> has the GFP_KERNEL flag, scheduling while atomic??? > I think your comment applies to ufshpb_issue_umap_all_req as well, > Which is called from slave_configure/scsi_add_lun. ufshpb_issue_umap_all_req() is not called from atomic contexts, so ufshpb_issue_umap_all_req() is fine. Thanks, Can Guo. > > Since the host-mode series is utilizing the framework laid by the > device-mode, > Maybe you can add this comment to Daejun's last version? > > Thanks, > Avri > >> >> Can Guo. >> >> > Thanks. >> > Can Guo. >> > >> >> + return; >> >> + >> >> lru_info = &hpb->lru_info; >> >> >> >> dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", >> >> rgn->rgn_idx); >> >> @@ -1855,6 +1866,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); >> >> ufshpb_sysfs_attr_show_func(rb_active_cnt); >> >> ufshpb_sysfs_attr_show_func(rb_inactive_cnt); >> >> ufshpb_sysfs_attr_show_func(map_req_cnt); >> >> +ufshpb_sysfs_attr_show_func(umap_req_cnt); >> >> >> >> static struct attribute *hpb_dev_stat_attrs[] = { >> >> &dev_attr_hit_cnt.attr, >> >> @@ -1863,6 +1875,7 @@ static struct attribute *hpb_dev_stat_attrs[] = >> >> { >> >> &dev_attr_rb_active_cnt.attr, >> >> &dev_attr_rb_inactive_cnt.attr, >> >> &dev_attr_map_req_cnt.attr, >> >> + &dev_attr_umap_req_cnt.attr, >> >> NULL, >> >> }; >> >> >> >> @@ -1978,6 +1991,7 @@ static void ufshpb_stat_init(struct ufshpb_lu >> >> *hpb) >> >> hpb->stats.rb_active_cnt = 0; >> >> hpb->stats.rb_inactive_cnt = 0; >> >> hpb->stats.map_req_cnt = 0; >> >> + hpb->stats.umap_req_cnt = 0; >> >> } >> >> >> >> static void ufshpb_param_init(struct ufshpb_lu *hpb) >> >> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h >> >> index bd4308010466..84598a317897 100644 >> >> --- a/drivers/scsi/ufs/ufshpb.h >> >> +++ b/drivers/scsi/ufs/ufshpb.h >> >> @@ -186,6 +186,7 @@ struct ufshpb_stats { >> >> u64 rb_inactive_cnt; >> >> u64 map_req_cnt; >> >> u64 pre_req_cnt; >> >> + u64 umap_req_cnt; >> >> }; >> >> >> >> struct ufshpb_lu {
>> >> --- >> >> drivers/scsi/ufs/ufshpb.c | 14 ++++++++++++++ >> >> drivers/scsi/ufs/ufshpb.h | 1 + >> >> 2 files changed, 15 insertions(+) >> >> >> >> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c >> >> index 6f4fd22eaf2f..0744feb4d484 100644 >> >> --- a/drivers/scsi/ufs/ufshpb.c >> >> +++ b/drivers/scsi/ufs/ufshpb.c >> >> @@ -907,6 +907,7 @@ static int ufshpb_execute_umap_req(struct >> >> ufshpb_lu *hpb, >> >> >> >> blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_umap_req_compl_fn); >> >> >> >> + hpb->stats.umap_req_cnt++; >> >> return 0; >> >> } >> >> >> >> @@ -1103,6 +1104,12 @@ static int ufshpb_issue_umap_req(struct >> >> ufshpb_lu *hpb, >> >> return -EAGAIN; >> >> } >> >> >> >> +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, >> >> + struct ufshpb_region *rgn) >> >> +{ >> >> + return ufshpb_issue_umap_req(hpb, rgn); >> >> +} >> >> + >> >> static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) >> >> { >> >> return ufshpb_issue_umap_req(hpb, NULL); >> >> @@ -1115,6 +1122,10 @@ static void __ufshpb_evict_region(struct >> >> ufshpb_lu *hpb, >> >> struct ufshpb_subregion *srgn; >> >> int srgn_idx; >> >> >> >> + >> >> + if (hpb->is_hcm && ufshpb_issue_umap_single_req(hpb, rgn)) >> > >> > __ufshpb_evict_region() is called with rgn_state_lock held and IRQ >> > disabled, >> > when ufshpb_issue_umap_single_req() invokes blk_execute_rq_nowait(), >> > below >> > warning shall pop up every time, fix it? >> > >> > void blk_execute_rq_nowait(struct request_queue *q, struct gendisk >> > *bd_disk, >> > struct request *rq, int at_head, >> > rq_end_io_fn *done) >> > { >> > WARN_ON(irqs_disabled()); >> > ... >> > >> >> Moreover, since we are here with rgn_state_lock held and IRQ disabled, >> in ufshpb_get_req(), rq = kmem_cache_alloc(hpb->map_req_cache, >> GFP_KERNEL) >> has the GFP_KERNEL flag, scheduling while atomic??? >I think your comment applies to ufshpb_issue_umap_all_req as well, >Which is called from slave_configure/scsi_add_lun. > >Since the host-mode series is utilizing the framework laid by the device-mode, >Maybe you can add this comment to Daejun's last version? Hi Avri, Can Guo I think ufshpb_issue_umap_single_req() can be moved to end of ufshpb_evict_region(). Then we can avoid rgn_state_lock when it sends unmap command. Thanks, Daejun >Thanks, >Avri > >> >> Can Guo. >> >> > Thanks. >> > Can Guo. >> > >> >> + return; >> >> + >> >> lru_info = &hpb->lru_info; >> >> >> >> dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", >> >> rgn->rgn_idx); >> >> @@ -1855,6 +1866,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); >> >> ufshpb_sysfs_attr_show_func(rb_active_cnt); >> >> ufshpb_sysfs_attr_show_func(rb_inactive_cnt); >> >> ufshpb_sysfs_attr_show_func(map_req_cnt); >> >> +ufshpb_sysfs_attr_show_func(umap_req_cnt); >> >> >> >> static struct attribute *hpb_dev_stat_attrs[] = { >> >> &dev_attr_hit_cnt.attr, >> >> @@ -1863,6 +1875,7 @@ static struct attribute *hpb_dev_stat_attrs[] = >> >> { >> >> &dev_attr_rb_active_cnt.attr, >> >> &dev_attr_rb_inactive_cnt.attr, >> >> &dev_attr_map_req_cnt.attr, >> >> + &dev_attr_umap_req_cnt.attr, >> >> NULL, >> >> }; >> >> >> >> @@ -1978,6 +1991,7 @@ static void ufshpb_stat_init(struct ufshpb_lu >> >> *hpb) >> >> hpb->stats.rb_active_cnt = 0; >> >> hpb->stats.rb_inactive_cnt = 0; >> >> hpb->stats.map_req_cnt = 0; >> >> + hpb->stats.umap_req_cnt = 0; >> >> } >> >> >> >> static void ufshpb_param_init(struct ufshpb_lu *hpb) >> >> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h >> >> index bd4308010466..84598a317897 100644 >> >> --- a/drivers/scsi/ufs/ufshpb.h >> >> +++ b/drivers/scsi/ufs/ufshpb.h >> >> @@ -186,6 +186,7 @@ struct ufshpb_stats { >> >> u64 rb_inactive_cnt; >> >> u64 map_req_cnt; >> >> u64 pre_req_cnt; >> >> + u64 umap_req_cnt; >> >> }; >> >> >> >> struct ufshpb_lu { > > >
On 2021-03-17 10:28, Daejun Park wrote: >>> >> --- >>> >> drivers/scsi/ufs/ufshpb.c | 14 ++++++++++++++ >>> >> drivers/scsi/ufs/ufshpb.h | 1 + >>> >> 2 files changed, 15 insertions(+) >>> >> >>> >> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c >>> >> index 6f4fd22eaf2f..0744feb4d484 100644 >>> >> --- a/drivers/scsi/ufs/ufshpb.c >>> >> +++ b/drivers/scsi/ufs/ufshpb.c >>> >> @@ -907,6 +907,7 @@ static int ufshpb_execute_umap_req(struct >>> >> ufshpb_lu *hpb, >>> >> >>> >> blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_umap_req_compl_fn); >>> >> >>> >> + hpb->stats.umap_req_cnt++; >>> >> return 0; >>> >> } >>> >> >>> >> @@ -1103,6 +1104,12 @@ static int ufshpb_issue_umap_req(struct >>> >> ufshpb_lu *hpb, >>> >> return -EAGAIN; >>> >> } >>> >> >>> >> +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, >>> >> + struct ufshpb_region *rgn) >>> >> +{ >>> >> + return ufshpb_issue_umap_req(hpb, rgn); >>> >> +} >>> >> + >>> >> static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) >>> >> { >>> >> return ufshpb_issue_umap_req(hpb, NULL); >>> >> @@ -1115,6 +1122,10 @@ static void __ufshpb_evict_region(struct >>> >> ufshpb_lu *hpb, >>> >> struct ufshpb_subregion *srgn; >>> >> int srgn_idx; >>> >> >>> >> + >>> >> + if (hpb->is_hcm && ufshpb_issue_umap_single_req(hpb, rgn)) >>> > >>> > __ufshpb_evict_region() is called with rgn_state_lock held and IRQ >>> > disabled, >>> > when ufshpb_issue_umap_single_req() invokes blk_execute_rq_nowait(), >>> > below >>> > warning shall pop up every time, fix it? >>> > >>> > void blk_execute_rq_nowait(struct request_queue *q, struct gendisk >>> > *bd_disk, >>> > struct request *rq, int at_head, >>> > rq_end_io_fn *done) >>> > { >>> > WARN_ON(irqs_disabled()); >>> > ... >>> > >>> >>> Moreover, since we are here with rgn_state_lock held and IRQ >>> disabled, >>> in ufshpb_get_req(), rq = kmem_cache_alloc(hpb->map_req_cache, >>> GFP_KERNEL) >>> has the GFP_KERNEL flag, scheduling while atomic??? >> I think your comment applies to ufshpb_issue_umap_all_req as well, >> Which is called from slave_configure/scsi_add_lun. >> >> Since the host-mode series is utilizing the framework laid by the >> device-mode, >> Maybe you can add this comment to Daejun's last version? > > Hi Avri, Can Guo > > I think ufshpb_issue_umap_single_req() can be moved to end of > ufshpb_evict_region(). > Then we can avoid rgn_state_lock when it sends unmap command. I am not the expert here, please you two fix it. I am just reporting what can be wrong. Anyways, ufshpb_issue_umap_single_req() should not be called with rgn_state_lock held - think about below (another deadly) scenario. lock(rgn_state_lock) ufshpb_issue_umap_single_req() ufshpb_prep() lock(rgn_state_lock) <---------- recursive spin_lock BTW, @Daejun shouldn't we stop passthrough cmds from stepping into ufshpb_prep()? In current code, you are trying to use below check to block cmds other than write/discard/read, but a passthrough cmd can not be blocked by the check. if (!ufshpb_is_write_or_discard_cmd(cmd) && !ufshpb_is_read_cmd(cmd)) return 0; Thanks, Can Guo. > > Thanks, > Daejun > > >> Thanks, >> Avri >> >>> >>> Can Guo. >>> >>> > Thanks. >>> > Can Guo. >>> > >>> >> + return; >>> >> + >>> >> lru_info = &hpb->lru_info; >>> >> >>> >> dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", >>> >> rgn->rgn_idx); >>> >> @@ -1855,6 +1866,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); >>> >> ufshpb_sysfs_attr_show_func(rb_active_cnt); >>> >> ufshpb_sysfs_attr_show_func(rb_inactive_cnt); >>> >> ufshpb_sysfs_attr_show_func(map_req_cnt); >>> >> +ufshpb_sysfs_attr_show_func(umap_req_cnt); >>> >> >>> >> static struct attribute *hpb_dev_stat_attrs[] = { >>> >> &dev_attr_hit_cnt.attr, >>> >> @@ -1863,6 +1875,7 @@ static struct attribute *hpb_dev_stat_attrs[] = >>> >> { >>> >> &dev_attr_rb_active_cnt.attr, >>> >> &dev_attr_rb_inactive_cnt.attr, >>> >> &dev_attr_map_req_cnt.attr, >>> >> + &dev_attr_umap_req_cnt.attr, >>> >> NULL, >>> >> }; >>> >> >>> >> @@ -1978,6 +1991,7 @@ static void ufshpb_stat_init(struct ufshpb_lu >>> >> *hpb) >>> >> hpb->stats.rb_active_cnt = 0; >>> >> hpb->stats.rb_inactive_cnt = 0; >>> >> hpb->stats.map_req_cnt = 0; >>> >> + hpb->stats.umap_req_cnt = 0; >>> >> } >>> >> >>> >> static void ufshpb_param_init(struct ufshpb_lu *hpb) >>> >> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h >>> >> index bd4308010466..84598a317897 100644 >>> >> --- a/drivers/scsi/ufs/ufshpb.h >>> >> +++ b/drivers/scsi/ufs/ufshpb.h >>> >> @@ -186,6 +186,7 @@ struct ufshpb_stats { >>> >> u64 rb_inactive_cnt; >>> >> u64 map_req_cnt; >>> >> u64 pre_req_cnt; >>> >> + u64 umap_req_cnt; >>> >> }; >>> >> >>> >> struct ufshpb_lu { >> >> >>
>>>> >> --- >>>> >> drivers/scsi/ufs/ufshpb.c | 14 ++++++++++++++ >>>> >> drivers/scsi/ufs/ufshpb.h | 1 + >>>> >> 2 files changed, 15 insertions(+) >>>> >> >>>> >> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c >>>> >> index 6f4fd22eaf2f..0744feb4d484 100644 >>>> >> --- a/drivers/scsi/ufs/ufshpb.c >>>> >> +++ b/drivers/scsi/ufs/ufshpb.c >>>> >> @@ -907,6 +907,7 @@ static int ufshpb_execute_umap_req(struct >>>> >> ufshpb_lu *hpb, >>>> >> >>>> >> blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_umap_req_compl_fn); >>>> >> >>>> >> + hpb->stats.umap_req_cnt++; >>>> >> return 0; >>>> >> } >>>> >> >>>> >> @@ -1103,6 +1104,12 @@ static int ufshpb_issue_umap_req(struct >>>> >> ufshpb_lu *hpb, >>>> >> return -EAGAIN; >>>> >> } >>>> >> >>>> >> +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, >>>> >> + struct ufshpb_region *rgn) >>>> >> +{ >>>> >> + return ufshpb_issue_umap_req(hpb, rgn); >>>> >> +} >>>> >> + >>>> >> static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) >>>> >> { >>>> >> return ufshpb_issue_umap_req(hpb, NULL); >>>> >> @@ -1115,6 +1122,10 @@ static void __ufshpb_evict_region(struct >>>> >> ufshpb_lu *hpb, >>>> >> struct ufshpb_subregion *srgn; >>>> >> int srgn_idx; >>>> >> >>>> >> + >>>> >> + if (hpb->is_hcm && ufshpb_issue_umap_single_req(hpb, rgn)) >>>> > >>>> > __ufshpb_evict_region() is called with rgn_state_lock held and IRQ >>>> > disabled, >>>> > when ufshpb_issue_umap_single_req() invokes blk_execute_rq_nowait(), >>>> > below >>>> > warning shall pop up every time, fix it? >>>> > >>>> > void blk_execute_rq_nowait(struct request_queue *q, struct gendisk >>>> > *bd_disk, >>>> > struct request *rq, int at_head, >>>> > rq_end_io_fn *done) >>>> > { >>>> > WARN_ON(irqs_disabled()); >>>> > ... >>>> > >>>> >>>> Moreover, since we are here with rgn_state_lock held and IRQ >>>> disabled, >>>> in ufshpb_get_req(), rq = kmem_cache_alloc(hpb->map_req_cache, >>>> GFP_KERNEL) >>>> has the GFP_KERNEL flag, scheduling while atomic??? >>> I think your comment applies to ufshpb_issue_umap_all_req as well, >>> Which is called from slave_configure/scsi_add_lun. >>> >>> Since the host-mode series is utilizing the framework laid by the >>> device-mode, >>> Maybe you can add this comment to Daejun's last version? >> >> Hi Avri, Can Guo >> >> I think ufshpb_issue_umap_single_req() can be moved to end of >> ufshpb_evict_region(). >> Then we can avoid rgn_state_lock when it sends unmap command. > >I am not the expert here, please you two fix it. I am just reporting >what can be wrong. Anyways, ufshpb_issue_umap_single_req() should not >be called with rgn_state_lock held - think about below (another deadly) >scenario. > >lock(rgn_state_lock) > ufshpb_issue_umap_single_req() > ufshpb_prep() > lock(rgn_state_lock) <---------- recursive spin_lock > >BTW, @Daejun shouldn't we stop passthrough cmds from stepping >into ufshpb_prep()? In current code, you are trying to use below >check to block cmds other than write/discard/read, but a passthrough >cmd can not be blocked by the check. > > if (!ufshpb_is_write_or_discard_cmd(cmd) && > !ufshpb_is_read_cmd(cmd) ) > return 0; I found this problem too. I fixed it and submit next patch. if (blk_rq_is_scsi(cmd->request) || (!ufshpb_is_write_or_discard_cmd(cmd) && !ufshpb_is_read_cmd(cmd))) return 0; Thanks, Daejun >Thanks, >Can Guo. > >> >> Thanks, >> Daejun >> >> >>> Thanks, >>> Avri >>> >>>> >>>> Can Guo. >>>> >>>> > Thanks. >>>> > Can Guo. >>>> > >>>> >> + return; >>>> >> + >>>> >> lru_info = &hpb->lru_info; >>>> >> >>>> >> dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", >>>> >> rgn->rgn_idx); >>>> >> @@ -1855,6 +1866,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); >>>> >> ufshpb_sysfs_attr_show_func(rb_active_cnt); >>>> >> ufshpb_sysfs_attr_show_func(rb_inactive_cnt); >>>> >> ufshpb_sysfs_attr_show_func(map_req_cnt); >>>> >> +ufshpb_sysfs_attr_show_func(umap_req_cnt); >>>> >> >>>> >> static struct attribute *hpb_dev_stat_attrs[] = { >>>> >> &dev_attr_hit_cnt.attr, >>>> >> @@ -1863,6 +1875,7 @@ static struct attribute *hpb_dev_stat_attrs[] = >>>> >> { >>>> >> &dev_attr_rb_active_cnt.attr, >>>> >> &dev_attr_rb_inactive_cnt.attr, >>>> >> &dev_attr_map_req_cnt.attr, >>>> >> + &dev_attr_umap_req_cnt.attr, >>>> >> NULL, >>>> >> }; >>>> >> >>>> >> @@ -1978,6 +1991,7 @@ static void ufshpb_stat_init(struct ufshpb_lu >>>> >> *hpb) >>>> >> hpb->stats.rb_active_cnt = 0; >>>> >> hpb->stats.rb_inactive_cnt = 0; >>>> >> hpb->stats.map_req_cnt = 0; >>>> >> + hpb->stats.umap_req_cnt = 0; >>>> >> } >>>> >> >>>> >> static void ufshpb_param_init(struct ufshpb_lu *hpb) >>>> >> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h >>>> >> index bd4308010466..84598a317897 100644 >>>> >> --- a/drivers/scsi/ufs/ufshpb.h >>>> >> +++ b/drivers/scsi/ufs/ufshpb.h >>>> >> @@ -186,6 +186,7 @@ struct ufshpb_stats { >>>> >> u64 rb_inactive_cnt; >>>> >> u64 map_req_cnt; >>>> >> u64 pre_req_cnt; >>>> >> + u64 umap_req_cnt; >>>> >> }; >>>> >> >>>> >> struct ufshpb_lu { >>> >>> >>> > > >
On 2021-03-17 13:19, Daejun Park wrote: >>>>> >> --- >>>>> >> drivers/scsi/ufs/ufshpb.c | 14 ++++++++++++++ >>>>> >> drivers/scsi/ufs/ufshpb.h | 1 + >>>>> >> 2 files changed, 15 insertions(+) >>>>> >> >>>>> >> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c >>>>> >> index 6f4fd22eaf2f..0744feb4d484 100644 >>>>> >> --- a/drivers/scsi/ufs/ufshpb.c >>>>> >> +++ b/drivers/scsi/ufs/ufshpb.c >>>>> >> @@ -907,6 +907,7 @@ static int ufshpb_execute_umap_req(struct >>>>> >> ufshpb_lu *hpb, >>>>> >> >>>>> >> blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_umap_req_compl_fn); >>>>> >> >>>>> >> + hpb->stats.umap_req_cnt++; >>>>> >> return 0; >>>>> >> } >>>>> >> >>>>> >> @@ -1103,6 +1104,12 @@ static int ufshpb_issue_umap_req(struct >>>>> >> ufshpb_lu *hpb, >>>>> >> return -EAGAIN; >>>>> >> } >>>>> >> >>>>> >> +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, >>>>> >> + struct ufshpb_region *rgn) >>>>> >> +{ >>>>> >> + return ufshpb_issue_umap_req(hpb, rgn); >>>>> >> +} >>>>> >> + >>>>> >> static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) >>>>> >> { >>>>> >> return ufshpb_issue_umap_req(hpb, NULL); >>>>> >> @@ -1115,6 +1122,10 @@ static void __ufshpb_evict_region(struct >>>>> >> ufshpb_lu *hpb, >>>>> >> struct ufshpb_subregion *srgn; >>>>> >> int srgn_idx; >>>>> >> >>>>> >> + >>>>> >> + if (hpb->is_hcm && ufshpb_issue_umap_single_req(hpb, rgn)) >>>>> > >>>>> > __ufshpb_evict_region() is called with rgn_state_lock held and IRQ >>>>> > disabled, >>>>> > when ufshpb_issue_umap_single_req() invokes blk_execute_rq_nowait(), >>>>> > below >>>>> > warning shall pop up every time, fix it? >>>>> > >>>>> > void blk_execute_rq_nowait(struct request_queue *q, struct gendisk >>>>> > *bd_disk, >>>>> > struct request *rq, int at_head, >>>>> > rq_end_io_fn *done) >>>>> > { >>>>> > WARN_ON(irqs_disabled()); >>>>> > ... >>>>> > >>>>> >>>>> Moreover, since we are here with rgn_state_lock held and IRQ >>>>> disabled, >>>>> in ufshpb_get_req(), rq = kmem_cache_alloc(hpb->map_req_cache, >>>>> GFP_KERNEL) >>>>> has the GFP_KERNEL flag, scheduling while atomic??? >>>> I think your comment applies to ufshpb_issue_umap_all_req as well, >>>> Which is called from slave_configure/scsi_add_lun. >>>> >>>> Since the host-mode series is utilizing the framework laid by the >>>> device-mode, >>>> Maybe you can add this comment to Daejun's last version? >>> >>> Hi Avri, Can Guo >>> >>> I think ufshpb_issue_umap_single_req() can be moved to end of >>> ufshpb_evict_region(). >>> Then we can avoid rgn_state_lock when it sends unmap command. >> >> I am not the expert here, please you two fix it. I am just reporting >> what can be wrong. Anyways, ufshpb_issue_umap_single_req() should not >> be called with rgn_state_lock held - think about below (another >> deadly) >> scenario. >> >> lock(rgn_state_lock) >> ufshpb_issue_umap_single_req() >> ufshpb_prep() >> lock(rgn_state_lock) <---------- recursive spin_lock >> >> BTW, @Daejun shouldn't we stop passthrough cmds from stepping >> into ufshpb_prep()? In current code, you are trying to use below >> check to block cmds other than write/discard/read, but a passthrough >> cmd can not be blocked by the check. >> >> if (!ufshpb_is_write_or_discard_cmd(cmd) && >> !ufshpb_is_read_cmd(cmd) ) >> return 0; > > I found this problem too. I fixed it and submit next patch. You mean in V30, which has not been uploaded yet, right? Thanks, Can Guo. > > if (blk_rq_is_scsi(cmd->request) || > (!ufshpb_is_write_or_discard_cmd(cmd) && > !ufshpb_is_read_cmd(cmd))) > return 0; > > > Thanks, > Daejun > >> Thanks, >> Can Guo. >> >>> >>> Thanks, >>> Daejun >>> >>> >>>> Thanks, >>>> Avri >>>> >>>>> >>>>> Can Guo. >>>>> >>>>> > Thanks. >>>>> > Can Guo. >>>>> > >>>>> >> + return; >>>>> >> + >>>>> >> lru_info = &hpb->lru_info; >>>>> >> >>>>> >> dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", >>>>> >> rgn->rgn_idx); >>>>> >> @@ -1855,6 +1866,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); >>>>> >> ufshpb_sysfs_attr_show_func(rb_active_cnt); >>>>> >> ufshpb_sysfs_attr_show_func(rb_inactive_cnt); >>>>> >> ufshpb_sysfs_attr_show_func(map_req_cnt); >>>>> >> +ufshpb_sysfs_attr_show_func(umap_req_cnt); >>>>> >> >>>>> >> static struct attribute *hpb_dev_stat_attrs[] = { >>>>> >> &dev_attr_hit_cnt.attr, >>>>> >> @@ -1863,6 +1875,7 @@ static struct attribute *hpb_dev_stat_attrs[] = >>>>> >> { >>>>> >> &dev_attr_rb_active_cnt.attr, >>>>> >> &dev_attr_rb_inactive_cnt.attr, >>>>> >> &dev_attr_map_req_cnt.attr, >>>>> >> + &dev_attr_umap_req_cnt.attr, >>>>> >> NULL, >>>>> >> }; >>>>> >> >>>>> >> @@ -1978,6 +1991,7 @@ static void ufshpb_stat_init(struct ufshpb_lu >>>>> >> *hpb) >>>>> >> hpb->stats.rb_active_cnt = 0; >>>>> >> hpb->stats.rb_inactive_cnt = 0; >>>>> >> hpb->stats.map_req_cnt = 0; >>>>> >> + hpb->stats.umap_req_cnt = 0; >>>>> >> } >>>>> >> >>>>> >> static void ufshpb_param_init(struct ufshpb_lu *hpb) >>>>> >> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h >>>>> >> index bd4308010466..84598a317897 100644 >>>>> >> --- a/drivers/scsi/ufs/ufshpb.h >>>>> >> +++ b/drivers/scsi/ufs/ufshpb.h >>>>> >> @@ -186,6 +186,7 @@ struct ufshpb_stats { >>>>> >> u64 rb_inactive_cnt; >>>>> >> u64 map_req_cnt; >>>>> >> u64 pre_req_cnt; >>>>> >> + u64 umap_req_cnt; >>>>> >> }; >>>>> >> >>>>> >> struct ufshpb_lu { >>>> >>>> >>>> >> >> >>
>>>>>> >> --- >>>>>> >> drivers/scsi/ufs/ufshpb.c | 14 ++++++++++++++ >>>>>> >> drivers/scsi/ufs/ufshpb.h | 1 + >>>>>> >> 2 files changed, 15 insertions(+) >>>>>> >> >>>>>> >> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c >>>>>> >> index 6f4fd22eaf2f..0744feb4d484 100644 >>>>>> >> --- a/drivers/scsi/ufs/ufshpb.c >>>>>> >> +++ b/drivers/scsi/ufs/ufshpb.c >>>>>> >> @@ -907,6 +907,7 @@ static int ufshpb_execute_umap_req(struct >>>>>> >> ufshpb_lu *hpb, >>>>>> >> >>>>>> >> blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_umap_req_compl_fn); >>>>>> >> >>>>>> >> + hpb->stats.umap_req_cnt++; >>>>>> >> return 0; >>>>>> >> } >>>>>> >> >>>>>> >> @@ -1103,6 +1104,12 @@ static int ufshpb_issue_umap_req(struct >>>>>> >> ufshpb_lu *hpb, >>>>>> >> return -EAGAIN; >>>>>> >> } >>>>>> >> >>>>>> >> +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, >>>>>> >> + struct ufshpb_region *rgn) >>>>>> >> +{ >>>>>> >> + return ufshpb_issue_umap_req(hpb, rgn); >>>>>> >> +} >>>>>> >> + >>>>>> >> static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) >>>>>> >> { >>>>>> >> return ufshpb_issue_umap_req(hpb, NULL); >>>>>> >> @@ -1115,6 +1122,10 @@ static void __ufshpb_evict_region(struct >>>>>> >> ufshpb_lu *hpb, >>>>>> >> struct ufshpb_subregion *srgn; >>>>>> >> int srgn_idx; >>>>>> >> >>>>>> >> + >>>>>> >> + if (hpb->is_hcm && ufshpb_issue_umap_single_req(hpb, rgn)) >>>>>> > >>>>>> > __ufshpb_evict_region() is called with rgn_state_lock held and IRQ >>>>>> > disabled, >>>>>> > when ufshpb_issue_umap_single_req() invokes blk_execute_rq_nowait(), >>>>>> > below >>>>>> > warning shall pop up every time, fix it? >>>>>> > >>>>>> > void blk_execute_rq_nowait(struct request_queue *q, struct gendisk >>>>>> > *bd_disk, >>>>>> > struct request *rq, int at_head, >>>>>> > rq_end_io_fn *done) >>>>>> > { >>>>>> > WARN_ON(irqs_disabled()); >>>>>> > ... >>>>>> > >>>>>> >>>>>> Moreover, since we are here with rgn_state_lock held and IRQ >>>>>> disabled, >>>>>> in ufshpb_get_req(), rq = kmem_cache_alloc(hpb->map_req_cache, >>>>>> GFP_KERNEL) >>>>>> has the GFP_KERNEL flag, scheduling while atomic??? >>>>> I think your comment applies to ufshpb_issue_umap_all_req as well, >>>>> Which is called from slave_configure/scsi_add_lun. >>>>> >>>>> Since the host-mode series is utilizing the framework laid by the >>>>> device-mode, >>>>> Maybe you can add this comment to Daejun's last version? >>>> >>>> Hi Avri, Can Guo >>>> >>>> I think ufshpb_issue_umap_single_req() can be moved to end of >>>> ufshpb_evict_region(). >>>> Then we can avoid rgn_state_lock when it sends unmap command. >>> >>> I am not the expert here, please you two fix it. I am just reporting >>> what can be wrong. Anyways, ufshpb_issue_umap_single_req() should not >>> be called with rgn_state_lock held - think about below (another >>> deadly) >>> scenario. >>> >>> lock(rgn_state_lock) >>> ufshpb_issue_umap_single_req() >>> ufshpb_prep() >>> lock(rgn_state_lock) <---------- recursive spin_lock >>> >>> BTW, @Daejun shouldn't we stop passthrough cmds from stepping >>> into ufshpb_prep()? In current code, you are trying to use below >>> check to block cmds other than write/discard/read, but a passthrough >>> cmd can not be blocked by the check. >>> >>> if (!ufshpb_is_write_or_discard_cmd(cmd) && >>> !ufshpb_is_read_cmd(cmd) ) >>> return 0; >> >> I found this problem too. I fixed it and submit next patch. > >You mean in V30, which has not been uploaded yet, right? Yes, it is about v30. Thanks, Daejun >Thanks, >Can Guo. > >> >> if (blk_rq_is_scsi(cmd->request) || >> (!ufshpb_is_write_or_discard_cmd(cmd) && >> !ufshpb_is_read_cmd(cmd))) >> return 0; >> >> >> Thanks, >> Daejun >> >>> Thanks, >>> Can Guo. >>> >>>> >>>> Thanks, >>>> Daejun >>>> >>>> >>>>> Thanks, >>>>> Avri >>>>> >>>>>> >>>>>> Can Guo. >>>>>> >>>>>> > Thanks. >>>>>> > Can Guo. >>>>>> > >>>>>> >> + return; >>>>>> >> + >>>>>> >> lru_info = &hpb->lru_info; >>>>>> >> >>>>>> >> dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", >>>>>> >> rgn->rgn_idx); >>>>>> >> @@ -1855,6 +1866,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); >>>>>> >> ufshpb_sysfs_attr_show_func(rb_active_cnt); >>>>>> >> ufshpb_sysfs_attr_show_func(rb_inactive_cnt); >>>>>> >> ufshpb_sysfs_attr_show_func(map_req_cnt); >>>>>> >> +ufshpb_sysfs_attr_show_func(umap_req_cnt); >>>>>> >> >>>>>> >> static struct attribute *hpb_dev_stat_attrs[] = { >>>>>> >> &dev_attr_hit_cnt.attr, >>>>>> >> @@ -1863,6 +1875,7 @@ static struct attribute *hpb_dev_stat_attrs[] = >>>>>> >> { >>>>>> >> &dev_attr_rb_active_cnt.attr, >>>>>> >> &dev_attr_rb_inactive_cnt.attr, >>>>>> >> &dev_attr_map_req_cnt.attr, >>>>>> >> + &dev_attr_umap_req_cnt.attr, >>>>>> >> NULL, >>>>>> >> }; >>>>>> >> >>>>>> >> @@ -1978,6 +1991,7 @@ static void ufshpb_stat_init(struct ufshpb_lu >>>>>> >> *hpb) >>>>>> >> hpb->stats.rb_active_cnt = 0; >>>>>> >> hpb->stats.rb_inactive_cnt = 0; >>>>>> >> hpb->stats.map_req_cnt = 0; >>>>>> >> + hpb->stats.umap_req_cnt = 0; >>>>>> >> } >>>>>> >> >>>>>> >> static void ufshpb_param_init(struct ufshpb_lu *hpb) >>>>>> >> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h >>>>>> >> index bd4308010466..84598a317897 100644 >>>>>> >> --- a/drivers/scsi/ufs/ufshpb.h >>>>>> >> +++ b/drivers/scsi/ufs/ufshpb.h >>>>>> >> @@ -186,6 +186,7 @@ struct ufshpb_stats { >>>>>> >> u64 rb_inactive_cnt; >>>>>> >> u64 map_req_cnt; >>>>>> >> u64 pre_req_cnt; >>>>>> >> + u64 umap_req_cnt; >>>>>> >> }; >>>>>> >> >>>>>> >> struct ufshpb_lu { >>>>> >>>>> >>>>> >>> >>> >>> > > >
> On 2021-03-17 10:28, Daejun Park wrote: > >>> >> --- > >>> >> drivers/scsi/ufs/ufshpb.c | 14 ++++++++++++++ > >>> >> drivers/scsi/ufs/ufshpb.h | 1 + > >>> >> 2 files changed, 15 insertions(+) > >>> >> > >>> >> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c > >>> >> index 6f4fd22eaf2f..0744feb4d484 100644 > >>> >> --- a/drivers/scsi/ufs/ufshpb.c > >>> >> +++ b/drivers/scsi/ufs/ufshpb.c > >>> >> @@ -907,6 +907,7 @@ static int ufshpb_execute_umap_req(struct > >>> >> ufshpb_lu *hpb, > >>> >> > >>> >> blk_execute_rq_nowait(q, NULL, req, 1, > ufshpb_umap_req_compl_fn); > >>> >> > >>> >> + hpb->stats.umap_req_cnt++; > >>> >> return 0; > >>> >> } > >>> >> > >>> >> @@ -1103,6 +1104,12 @@ static int ufshpb_issue_umap_req(struct > >>> >> ufshpb_lu *hpb, > >>> >> return -EAGAIN; > >>> >> } > >>> >> > >>> >> +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, > >>> >> + struct ufshpb_region *rgn) > >>> >> +{ > >>> >> + return ufshpb_issue_umap_req(hpb, rgn); > >>> >> +} > >>> >> + > >>> >> static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) > >>> >> { > >>> >> return ufshpb_issue_umap_req(hpb, NULL); > >>> >> @@ -1115,6 +1122,10 @@ static void __ufshpb_evict_region(struct > >>> >> ufshpb_lu *hpb, > >>> >> struct ufshpb_subregion *srgn; > >>> >> int srgn_idx; > >>> >> > >>> >> + > >>> >> + if (hpb->is_hcm && ufshpb_issue_umap_single_req(hpb, rgn)) > >>> > > >>> > __ufshpb_evict_region() is called with rgn_state_lock held and IRQ > >>> > disabled, > >>> > when ufshpb_issue_umap_single_req() invokes > blk_execute_rq_nowait(), > >>> > below > >>> > warning shall pop up every time, fix it? > >>> > > >>> > void blk_execute_rq_nowait(struct request_queue *q, struct gendisk > >>> > *bd_disk, > >>> > struct request *rq, int at_head, > >>> > rq_end_io_fn *done) > >>> > { > >>> > WARN_ON(irqs_disabled()); > >>> > ... > >>> > > >>> > >>> Moreover, since we are here with rgn_state_lock held and IRQ > >>> disabled, > >>> in ufshpb_get_req(), rq = kmem_cache_alloc(hpb->map_req_cache, > >>> GFP_KERNEL) > >>> has the GFP_KERNEL flag, scheduling while atomic??? > >> I think your comment applies to ufshpb_issue_umap_all_req as well, > >> Which is called from slave_configure/scsi_add_lun. > >> > >> Since the host-mode series is utilizing the framework laid by the > >> device-mode, > >> Maybe you can add this comment to Daejun's last version? > > > > Hi Avri, Can Guo > > > > I think ufshpb_issue_umap_single_req() can be moved to end of > > ufshpb_evict_region(). > > Then we can avoid rgn_state_lock when it sends unmap command. > > I am not the expert here, please you two fix it. I am just reporting > what can be wrong. Anyways, ufshpb_issue_umap_single_req() should not > be called with rgn_state_lock held - think about below (another deadly) > scenario. > > lock(rgn_state_lock) > ufshpb_issue_umap_single_req() > ufshpb_prep() > lock(rgn_state_lock) <---------- recursive spin_lock Will fix. Will wait for Daejun's v30 to see if anything applies to unmap_single. Thanks, Avri > > BTW, @Daejun shouldn't we stop passthrough cmds from stepping > into ufshpb_prep()? In current code, you are trying to use below > check to block cmds other than write/discard/read, but a passthrough > cmd can not be blocked by the check. > > if (!ufshpb_is_write_or_discard_cmd(cmd) && > !ufshpb_is_read_cmd(cmd)) > return 0; > > Thanks, > Can Guo. > > > > > Thanks, > > Daejun > > > > > >> Thanks, > >> Avri > >> > >>> > >>> Can Guo. > >>> > >>> > Thanks. > >>> > Can Guo. > >>> > > >>> >> + return; > >>> >> + > >>> >> lru_info = &hpb->lru_info; > >>> >> > >>> >> dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", > >>> >> rgn->rgn_idx); > >>> >> @@ -1855,6 +1866,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); > >>> >> ufshpb_sysfs_attr_show_func(rb_active_cnt); > >>> >> ufshpb_sysfs_attr_show_func(rb_inactive_cnt); > >>> >> ufshpb_sysfs_attr_show_func(map_req_cnt); > >>> >> +ufshpb_sysfs_attr_show_func(umap_req_cnt); > >>> >> > >>> >> static struct attribute *hpb_dev_stat_attrs[] = { > >>> >> &dev_attr_hit_cnt.attr, > >>> >> @@ -1863,6 +1875,7 @@ static struct attribute *hpb_dev_stat_attrs[] > = > >>> >> { > >>> >> &dev_attr_rb_active_cnt.attr, > >>> >> &dev_attr_rb_inactive_cnt.attr, > >>> >> &dev_attr_map_req_cnt.attr, > >>> >> + &dev_attr_umap_req_cnt.attr, > >>> >> NULL, > >>> >> }; > >>> >> > >>> >> @@ -1978,6 +1991,7 @@ static void ufshpb_stat_init(struct ufshpb_lu > >>> >> *hpb) > >>> >> hpb->stats.rb_active_cnt = 0; > >>> >> hpb->stats.rb_inactive_cnt = 0; > >>> >> hpb->stats.map_req_cnt = 0; > >>> >> + hpb->stats.umap_req_cnt = 0; > >>> >> } > >>> >> > >>> >> static void ufshpb_param_init(struct ufshpb_lu *hpb) > >>> >> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > >>> >> index bd4308010466..84598a317897 100644 > >>> >> --- a/drivers/scsi/ufs/ufshpb.h > >>> >> +++ b/drivers/scsi/ufs/ufshpb.h > >>> >> @@ -186,6 +186,7 @@ struct ufshpb_stats { > >>> >> u64 rb_inactive_cnt; > >>> >> u64 map_req_cnt; > >>> >> u64 pre_req_cnt; > >>> >> + u64 umap_req_cnt; > >>> >> }; > >>> >> > >>> >> struct ufshpb_lu { > >> > >> > >>
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c index 6f4fd22eaf2f..0744feb4d484 100644 --- a/drivers/scsi/ufs/ufshpb.c +++ b/drivers/scsi/ufs/ufshpb.c @@ -907,6 +907,7 @@ static int ufshpb_execute_umap_req(struct ufshpb_lu *hpb, blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_umap_req_compl_fn); + hpb->stats.umap_req_cnt++; return 0; } @@ -1103,6 +1104,12 @@ static int ufshpb_issue_umap_req(struct ufshpb_lu *hpb, return -EAGAIN; } +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, + struct ufshpb_region *rgn) +{ + return ufshpb_issue_umap_req(hpb, rgn); +} + static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) { return ufshpb_issue_umap_req(hpb, NULL); @@ -1115,6 +1122,10 @@ static void __ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_subregion *srgn; int srgn_idx; + + if (hpb->is_hcm && ufshpb_issue_umap_single_req(hpb, rgn)) + return; + lru_info = &hpb->lru_info; dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", rgn->rgn_idx); @@ -1855,6 +1866,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt); ufshpb_sysfs_attr_show_func(rb_active_cnt); ufshpb_sysfs_attr_show_func(rb_inactive_cnt); ufshpb_sysfs_attr_show_func(map_req_cnt); +ufshpb_sysfs_attr_show_func(umap_req_cnt); static struct attribute *hpb_dev_stat_attrs[] = { &dev_attr_hit_cnt.attr, @@ -1863,6 +1875,7 @@ static struct attribute *hpb_dev_stat_attrs[] = { &dev_attr_rb_active_cnt.attr, &dev_attr_rb_inactive_cnt.attr, &dev_attr_map_req_cnt.attr, + &dev_attr_umap_req_cnt.attr, NULL, }; @@ -1978,6 +1991,7 @@ static void ufshpb_stat_init(struct ufshpb_lu *hpb) hpb->stats.rb_active_cnt = 0; hpb->stats.rb_inactive_cnt = 0; hpb->stats.map_req_cnt = 0; + hpb->stats.umap_req_cnt = 0; } static void ufshpb_param_init(struct ufshpb_lu *hpb) diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h index bd4308010466..84598a317897 100644 --- a/drivers/scsi/ufs/ufshpb.h +++ b/drivers/scsi/ufs/ufshpb.h @@ -186,6 +186,7 @@ struct ufshpb_stats { u64 rb_inactive_cnt; u64 map_req_cnt; u64 pre_req_cnt; + u64 umap_req_cnt; }; struct ufshpb_lu {
I host mode, the host is expected to send HPB-WRITE-BUFFER with buffer-id = 0x1 when it inactivates a region. Use the map-requests pool as there is no point in assigning a designated cache for umap-requests. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/scsi/ufs/ufshpb.c | 14 ++++++++++++++ drivers/scsi/ufs/ufshpb.h | 1 + 2 files changed, 15 insertions(+)