On Thu, 2019-06-20 at 17:05 -0300, Jason Gunthorpe wrote:
> On Thu, Jun 20, 2019 at 03:48:23PM -0400, Doug Ledford wrote:
>
> > It's an msleep() waiting for a hardware command to
> > complete. Waiting
> > synchronously for a command that has the purpose of stopping the
> > card's
> > operation does not sound like an incorrect locking or concurrency
> > model
> > to me. It sounds sane, albeit annoying.
>
> If it was the only sleep loop you might have a point, but it isn't,
> every other patch series lately seems to be adding more sleep
> loops. This sleep loop is already wrapping another sleep loop under
> __hns_roce_cmq_send() - which, for some reason, doesn't have an
> interrupt driven completion path.
>
> Nor is there any explanation why we need a sleep loop on top of a
> sleep loop, or why the command is allowed to fail or why retrying the
> failed command is even a good idea, or why it can't be properly
> interrupt driven!
>
> I'm frankly sick of it, maybe you should review HNS patches for a
> while..
Are you sure this hasn't changed over time and you didn't realize it?
I'm not seeing all the sleeps you are talking about. In fact, if I grep
for "sleep" in hw/hns/ I only find 9 instances: 5 in hns_roce_hw_v1 and
4 in hns_roce_hw_v2, so really only 5 at most as those two files are
just duplicates of each other for the different hardware. And even
then, when I checked on all the sleeps in hw_v2, they were all in init
or reset code paths, certainly none in send. And I didn't find any
include file wrappers that use sleeps. I get how over use of sleeps can
be a big issue, I guess I'm just having a hard time finding where it's
being abused as badly as you say. Of course, I may just not be looking
in the right place...
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: B826A3330E572FDD
Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Attachment:
signature.asc
Description: This is a digitally signed message part
