public inbox for linux-scsi@vger.kernel.org
 help / color / mirror / Atom feed
* RE: [PATCH v37 3/4] scsi: ufs: Prepare HPB read for cached sub-region
@ 2021-06-13 23:40 정요한(JOUNG YOHAN) Mobile SE
  2021-06-14  6:48 ` Avri Altman
  0 siblings, 1 reply; 6+ messages in thread
From: 정요한(JOUNG YOHAN) Mobile SE @ 2021-06-13 23:40 UTC (permalink / raw)
  To: avri.altman@wdc.com
  Cc: alim.akhtar@samsung.com, asutoshd@codeaurora.org,
	bvanassche@acm.org, cang@codeaurora.org, d_hyun.kwon@samsung.com,
	daejun7.park@samsung.com, gregkh@linuxfoundation.org,
	huobean@gmail.com, j-young.choi@samsung.com,
	jaemyung.lee@samsung.com,
	최재영(CHOI JAE YOUNG) Mobile SE,
	javier.gonz@samsung.com, jejb@linux.ibm.com,
	jh.i.park@samsung.com, jieon.seol@samsung.com,
	keosung.park@samsung.com, linux-kernel@vger.kernel.org,
	linux-scsi@vger.kernel.org, martin.petersen@oracle.com,
	stanley.chu@mediatek.com, sungjun07.park@samsung.com,
	정요한(JOUNG YOHAN) Mobile SE

> > >+      /*
> > >+       * If the region state is active, mctx must be allocated.
> > >+       * In this case, check whether the region is evicted or
> > >+       * mctx allcation fail.
> > >+       */
> > >+      if (unlikely(!srgn->mctx)) {
> > >+              dev_err(&hpb->sdev_ufs_lu->sdev_dev,
> > >+                      "no mctx in region %d subregion %d.\n",
> > >+                      srgn->rgn_idx, srgn->srgn_idx);
> > >+              return true;
> > >+      }
> > >+
> > >+      if ((srgn_offset + cnt) > bitmap_len)
> > >+              bit_len = bitmap_len - srgn_offset;
> > >+      else
> > >+              bit_len = cnt;
> > >+
> > >+      if (find_next_bit(srgn->mctx->ppn_dirty, bitmap_len,
> > >+                        srgn_offset) < bit_len + srgn_offset)
> > >+              return true;
> > >+
> > 
> > It seems unnecessary to search through bitmap_len
> > How about searching by transfer size?
> > 
> > if (find_next_bit(srgn->mctx->ppn_dirty,
> >               bit_len + srgn_offset, srgn_offset) < bit_len + srgn_offset)
> Isn't bit_len should be used for size, and not bit_len + srgn_offset ?

then find_next_bit checks from start to bit_len.
find_next_bit stops checking if start is greater than bit_len.
it does not check for dirty as transfer_size.

Thanks
Yohan

> 
> Thanks,
> Avri
> 
> > 
> > Thanks
> > Yohan

^ permalink raw reply	[flat|nested] 6+ messages in thread
* RE: [PATCH v37 3/4] scsi: ufs: Prepare HPB read for cached sub-region
@ 2021-06-14  7:44 정요한(JOUNG YOHAN) Mobile SE
  0 siblings, 0 replies; 6+ messages in thread
From: 정요한(JOUNG YOHAN) Mobile SE @ 2021-06-14  7:44 UTC (permalink / raw)
  To: avri.altman@wdc.com
  Cc: alim.akhtar@samsung.com, asutoshd@codeaurora.org,
	bvanassche@acm.org, cang@codeaurora.org, d_hyun.kwon@samsung.com,
	daejun7.park@samsung.com, gregkh@linuxfoundation.org,
	huobean@gmail.com, j-young.choi@samsung.com,
	jaemyung.lee@samsung.com,
	최재영(CHOI JAE YOUNG) Mobile SE,
	javier.gonz@samsung.com, jejb@linux.ibm.com,
	jh.i.park@samsung.com, jieon.seol@samsung.com,
	keosung.park@samsung.com, linux-kernel@vger.kernel.org,
	linux-scsi@vger.kernel.org, martin.petersen@oracle.com,
	stanley.chu@mediatek.com, sungjun07.park@samsung.com,
	정요한(JOUNG YOHAN) Mobile SE

> > > > >+      /*
> > > > >+       * If the region state is active, mctx must be allocated.
> > > > >+       * In this case, check whether the region is evicted or
> > > > >+       * mctx allcation fail.
> > > > >+       */
> > > > >+      if (unlikely(!srgn->mctx)) {
> > > > >+              dev_err(&hpb->sdev_ufs_lu->sdev_dev,
> > > > >+                      "no mctx in region %d subregion %d.\n",
> > > > >+                      srgn->rgn_idx, srgn->srgn_idx);
> > > > >+              return true;
> > > > >+      }
> > > > >+
> > > > >+      if ((srgn_offset + cnt) > bitmap_len)
> > > > >+              bit_len = bitmap_len - srgn_offset;
> > > > >+      else
> > > > >+              bit_len = cnt;
> > > > >+
> > > > >+      if (find_next_bit(srgn->mctx->ppn_dirty, bitmap_len,
> > > > >+                        srgn_offset) < bit_len + srgn_offset)
> > > > >+              return true;
> > > > >+
> > > >
> > > > It seems unnecessary to search through bitmap_len
> > > > How about searching by transfer size?
> > > >
> > > > if (find_next_bit(srgn->mctx->ppn_dirty,
> > > >               bit_len + srgn_offset, srgn_offset) < bit_len + srgn_offset)
> > > Isn't bit_len should be used for size, and not bit_len + srgn_offset ?
> > 
> > then find_next_bit checks from start to bit_len.
> > find_next_bit stops checking if start is greater than bit_len.
> > it does not check for dirty as transfer_size.
> Right. Size (nbits in _find_next_bit) practically means @end - Confusing...
> Either way, Is this tad optimization worth another spin in your opinion?

Worst case, we have to search every time up to the bitmap_len size. 
Even though, when bit_len is 1, only 1 bit needs to be checked
it can increase the effect, depending on the subregion size. 

Thanks
Yohan
> 
> Thanks,
> Avri
> 
> > 
> > Thanks
> > Yohan
> > 
> > >
> > > Thanks,
> > > Avri
> > >
> > > >
> > > > Thanks
> > > > Yohan

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v37 3/4] scsi: ufs: Prepare HPB read for cached sub-region
@ 2021-06-11  5:07 정요한(JOUNG YOHAN) Mobile SE
  2021-06-13 10:20 ` Avri Altman
  0 siblings, 1 reply; 6+ messages in thread
From: 정요한(JOUNG YOHAN) Mobile SE @ 2021-06-11  5:07 UTC (permalink / raw)
  To: daejun7.park@samsung.com
  Cc: alim.akhtar@samsung.com, asutoshd@codeaurora.org,
	avri.altman@wdc.com, bvanassche@acm.org, cang@codeaurora.org,
	d_hyun.kwon@samsung.com, Greg KH, huobean@gmail.com,
	j-young.choi@samsung.com, jaemyung.lee@samsung.com,
	javier.gonz@samsung.com, jejb@linux.ibm.com,
	jh.i.park@samsung.com, jieon.seol@samsung.com,
	keosung.park@samsung.com, linux-kernel@vger.kernel.org,
	linux-scsi@vger.kernel.org, martin.petersen@oracle.com,
	stanley.chu@mediatek.com, sungjun07.park@samsung.com,
	최재영(CHOI JAE YOUNG) Mobile SE

>+	/*
>+	 * If the region state is active, mctx must be allocated.
>+	 * In this case, check whether the region is evicted or
>+	 * mctx allcation fail.
>+	 */
>+	if (unlikely(!srgn->mctx)) {
>+		dev_err(&hpb->sdev_ufs_lu->sdev_dev,
>+			"no mctx in region %d subregion %d.\n",
>+			srgn->rgn_idx, srgn->srgn_idx);
>+		return true;
>+	}
>+
>+	if ((srgn_offset + cnt) > bitmap_len)
>+		bit_len = bitmap_len - srgn_offset;
>+	else
>+		bit_len = cnt;
>+
>+	if (find_next_bit(srgn->mctx->ppn_dirty, bitmap_len,
>+			  srgn_offset) < bit_len + srgn_offset)
>+		return true;
>+

It seems unnecessary to search through bitmap_len
How about searching by transfer size?

if (find_next_bit(srgn->mctx->ppn_dirty,
              bit_len + srgn_offset, srgn_offset) < bit_len + srgn_offset)

Thanks
Yohan

^ permalink raw reply	[flat|nested] 6+ messages in thread
[parent not found: <CGME20210611022142epcms2p374ea5b82cfe122de69a7fefe27edf856@epcms2p2>]

end of thread, other threads:[~2021-06-14  7:44 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-06-13 23:40 [PATCH v37 3/4] scsi: ufs: Prepare HPB read for cached sub-region 정요한(JOUNG YOHAN) Mobile SE
2021-06-14  6:48 ` Avri Altman
  -- strict thread matches above, loose matches on Subject: below --
2021-06-14  7:44 정요한(JOUNG YOHAN) Mobile SE
2021-06-11  5:07 정요한(JOUNG YOHAN) Mobile SE
2021-06-13 10:20 ` Avri Altman
     [not found] <CGME20210611022142epcms2p374ea5b82cfe122de69a7fefe27edf856@epcms2p2>
2021-06-11  2:21 ` [PATCH v37 0/4] scsi: ufs: Add Host Performance Booster Support Daejun Park
2021-06-11  2:23   ` [PATCH v37 3/4] scsi: ufs: Prepare HPB read for cached sub-region Daejun Park

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox