Re: [RFC PATCH v3 0/5] scsi: ufs: Add Host Performance Booster Support

[Bean Huo]

On Tue, 2020-06-30 at 06:39 +0000, Avri Altman wrote:
> Hi,
>  
> > 
> > Hi Bean,
> > > On Mon, 2020-06-29 at 15:15 +0900, Daejun Park wrote:
> > > > > Seems you intentionally ignored to give you comments on my
> > > > > suggestion.
> > > > > let me provide the reason.
> > > > 
> > > > Sorry! I replied to your comment (
> > > > https://protect2.fireeye.com/url?k=be575021-e3854728-be56db6e-
> > 
> > 0cc47a31cdf8-
> > 6c7d0e1e42762b92&q=1&u=https%3A%2F%2Flkml.org%2Flkml%2F2020%2F6%
> > 2F15%2F1492),
> > > > but you didn't reply on that. I thought you agreed because you
> > > > didn't
> > > > send
> > > > any more comments.
> > > > 
> > > > 
> > > > > Before submitting your next version patch, please check your
> > > > > L2P
> > > > > mapping HPB reqeust submission logical algorithem. I have did
> > > > 
> > > > We are also reviewing the code that you submitted before.
> > > > It seems to be a performance improvement as it sends a map
> > > > request
> > > > directly.
> > > > 
> > > > > performance comparison testing on 4KB, there are about 13%
> > > > > performance
> > > > > drop. Also the hit count is lower. I don't know if this is
> > > > > related
> > > > > to
> > > > 
> > > > It is interesting that there is actually a performance
> > > > improvement.
> > > > Could you share the test environment, please? However, I think
> > > > stability is
> > > > important to HPB driver. We have tested our method with the
> > > > real
> > > > products and
> > > > the HPB 1.0 driver is based on that.
> > > 
> > > I just run fio benchmark tool with --rw=randread, --bs=4kb, --
> > > size=8G/10G/64G/100G. and see what performance diff with the
> > > direct
> > > submission approach.
> > 
> > Thanks!
> > 
> > > > After this patch, your approach can be done as an incremental
> > > > patch?
> > > > I would
> > > > like to test the patch that you submitted and verify it.
> > > > 
> > > > > your current work queue scheduling, since you didn't add the
> > > > > timer
> > > > > for
> > > > > each HPB request.
> > > 
> > > Taking into consideration of the HPB 2.0, can we submit the HPB
> > > write
> > > request to the SCSI layer? if not, it will be a direct submission
> > > way.
> > > why not directly use direct way? or maybe you have a more
> > > advisable
> > > approach to work around this. would you please share with us.
> > > appreciate.
> > 
> > I am considering a direct submission way for the next version.
> > We will implement the write buffer command of HPB 2.0, after
> > patching HPB
> > 1.0.
> > 
> > As for the direct submission of HPB releated command including HPB
> > write
> > buffer, I think we'd better discuss the right approach in depth
> > before
> > moving on to the next step.
> 
> I vote to stay with the current implementation because:
> 1) Bean is probably right about 2.0, but it's out of scope for now - 
>     there is a long way to go before we'll need to worry about it
> 2) For now, we should focus on the functional flows. 
>     Performance issues, should such issues indeed exists, can be
> dealt with  later.  And,
> 3) The current code base is running in production for more than 3
> years now.
>      I am not so eager to dump a robust, well debugged code unless it
> absolutely necessary.
> 
> Thanks,
> Avri
> 
> 
Hi Avri
Thanks, appreciate you shared your position on this topic. I don't know
how I can convince you to change your opinion.
Let me try.

1. HPB 2.0 is not out of scope.
HPB 1.0 only supports 4KB read length, which is useless. I don't know
if there will be users who want to use HPB driver only supports 4KB
chunk size. I think, we all know that some smartphone vendors have
already use HPB 2.0, even HPB 2.0 has not been released yet. you
mentioned this in your before emails. HPB 1.0 is just a
transition(limited) version, we need to think about the HPB 2.0 support
when we develop the HPB 1.0 driver.
To say the least, if we don't think about HPB 2.0 support, and just
focus HPB 1.0, in the end, after HPB 2.0 releasing, we need to return
original point, re-do lots thing, why we cannot fix it now and think
one step further.

2. The major goal of the HPB feature is to improve random read
performance, and HPB device mode implementing flow is now already very
clear enough. I don't know what the functional flows you mentioned.
if it is HPB host mode, no, this is another big topic, I think we'd
better not add in current driver until we all have a final approach.

3. Regarding the Daejun's HPB driver used age, I can't easily jump to a
conclusion. But for sure, before he disclosed his HPB driver and
submitted to the community, he did lots of changes and deletions. That
means it still needs lots of tests.


I didn't mean to disrupt Daejun's patch upstreaming. If Daejun can
consider HPB 2.0 support while developing HPB 1.0 patch, that is super.
Thus we can quickly add HPB 2.0 support once HPB 2.0 Spec released.
Think about that who is now using HPB 1.0?

Thanks,
Bean