On Fri, 2019-06-21 at 09:57 -0300, Jason Gunthorpe wrote:
> On Thu, Jun 20, 2019 at 04:30:03PM -0400, Doug Ledford wrote:
> > 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.
>
> It is because I wasn't applying those patches :)
Ok, well, I promise if they send a patch that adds an msleep in the
middle of the send routine, I'll reject that. But this one is on the
module remove path (or so the subject says). I'm not really concerned
about it here.
> > 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
>
> Most of our drivers have 0 sleep loops (excluding the hfi/qib
> uglyness), so 9 is aleady absurd for a 2010 era driver.
Yeah, but they get away from that with a workqueue/waitq type setup.
That's fine for a running module, but is a bit messy when doing a module
unload where you have to protect against deleting the workqueue/waitq
until after you are done using it, so again, I'm fine with a sleep here
instead.
--
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
