ACK/cmnt: [SRU Xenial][PATCH 0/2] Improve our SAUCE for virtio-scsi reqs counter (fix CPU soft lockup)

[Mauricio Faria de Oliveira]

On Wed, Oct 17, 2018 at 11:59 AM Khaled Elmously
<khalid.elmously at canonical.com> wrote:
[snip]
> > >  - Maybe I'm misunderstanding what you mean by 'no-up'. I assumed it means "no operation", as in, no change in the resulting binary (e.g., whitespace changes, comment changes, casting etc.). Both patches seem to be "op" changes
> >
> > Hm, I meant 'no up-stream' (e.g., downstream/SAUCE patch that is not
> > expected to go upstream, as described in the StablePatchFormat wiki
> > page [3].
> >
>
> I had no idea that that's what "no-up" means :) Thanks

Heh, happens to everyone :-) Glad to help!

[snip]
> > >  - It seems strange to me that you'd have a loop that repeatedly decrements a counter because you know it will be incremented again when about to retry. I would think it makes more sense to increment the counter only once when queuing the command and decrement only once on success or "giving up" - though I assume you did it this way for a reason?
> >
> > I didn' t follow you on the loop scenario (I don't see a loop unless a
> > SCSI commands repeatedly fails to be kicked and gets requeued/resent
> > by the SCSI layer - is that it?)
>
> Yep, that's what I meant.

Okay. So, indeed, that scenario is possible and actually weird,
but that's not the common case nor hot path, fortunately
(so not much is wasted on these atomic inc/dec error path)

And yeah, unfortunately we had that 'reason' I mentioned in the previous email,
which didn't easily allow for changes in where the incrementing is done.
(If things were upstream the way they are downstram for us now,
I'd like to have sent something to simplify those things up, as you said.)

[snip]
> > I'll likely have a chat w/ Jay about it this week, but since the fix
> > was obvious and conservative,
> > I imagined it would be better to have it fixed on top for now, and
> > then refactored/re-fixed later).
>
> Fair enough - and thanks for clarifying in detail

Sure, thanks again for your careful review.

>
> >
> > [1] f1f609d8015e UBUNTU: SAUCE: (no-up) virtio-scsi: Fix race in target free
> > [2] a6c038a533f3 UBUNTU: SAUCE: (no-up) virtio-scsi: Increment reqs counter.
> >
> >
> > > None of the above points are major enough for me to NAK this patchset, but I wanted to bring them to your attention (in case you missed something).
> >
> > Sure, thanks for the careful review.
> > Hope this helps clarify things!
> >
> > > Acked-by: Khalid Elmously <khalid.elmously at canonical.com>
> > >
> >
> > [3] https://wiki.ubuntu.com/Kernel/Dev/StablePatchFormat
> >
> > --
> > Mauricio Faria de Oliveira



-- 
Mauricio Faria de Oliveira