Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1470768 > unrolled thread

Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result pointer

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result pointer Andy Lutomirski <luto@amacapital.net> - 2016-08-26 16:40 +0200
    Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result         pointer Christoph Hellwig <hch@lst.de> - 2016-08-26 16:50 +0200
      Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result  pointer Keith Busch <keith.busch@intel.com> - 2016-08-26 17:10 +0200

#1470768 — Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result pointer

On Aug 25, 2016 4:20 PM, "Jens Axboe" <axboe@fb.com> wrote:
>
> On 08/25/2016 01:54 AM, Andy Lutomirski wrote:
>>
>> On Thu, Aug 25, 2016 at 12:38 AM, Christoph Hellwig <hch@lst.de> wrote:
>>>
>>> Ooops, yes.
>>>
>>> Are you looking into new nvme_set_features users?  Another thing
>>> we need to tackle is either replacing dma_addr argument with a
>>> a real kernel pointer (or just kill it until users show up)
>>
>>
>> I am, and I have a patch to do the former (and to add a length
>> argument).  But that's not -stable material.
>>
>> While I have your attention: the new use is to enable APST (power
>> saving).  In theory, it seems like I should integrate with dev_pm_qos
>> so that the standard interface for setting a latency limit will work,
>> but, on brief inspection, there are literally no drivers in the entire
>> tree that do this.  Am I missing something?  My current draft patch
>> just adds a sysfs attribute.  (It saves a *lot* of power on my laptop,
>> so supporting APST is worth doing.)
>
>
> Care to send out what you have? I'd be interested in seeing how much I
> can save on my laptop, haven't played with APST yet.

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/log/?h=nvme/power

There are some todos:

 - Default to a nonzero latency (e.g. 7ms?  My SSD needs 5.5ms for max
power saving.)  To test it, write something like 7000000 to
apst_max_latency_ns.

 - Add a real changelog.

 - Optionally add a sysfs binfile or other interface to allow
uploading an entire custom table.

 - Consider *deleting* the SCSI translation layer's power saving code.
It looks almost entirely bogus to me.  It has an off-by-one in its
NPSS handling, it hardcodes power state indices which is total BS, it
ignores the distinction between operational and non-operational states
(which I think matters for non-APST usage).  It also seems likely to
be that it's never been used, since it's one of the formerly
crashy-looking set_features users.

You can inspect what it's doing with something like:

# nvme get-feature -f 0x0c -H -s 0 /dev/nvme0

if you have nvme-cli installed.

--Andy

[toc] | [next] | [standalone]

#1470778 — Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result pointer

On Fri, Aug 26, 2016 at 07:31:33AM -0700, Andy Lutomirski wrote:
>  - Consider *deleting* the SCSI translation layer's power saving code.
> It looks almost entirely bogus to me.  It has an off-by-one in its
> NPSS handling, it hardcodes power state indices which is total BS, it
> ignores the distinction between operational and non-operational states
> (which I think matters for non-APST usage).  It also seems likely to
> be that it's never been used, since it's one of the formerly
> crashy-looking set_features users.

Please go ahead and send a patch to delete it.  Adding the whole SCSI
layer was a mistake to start with, and it's always been horribly buggy.
Until I started running the libiscsi testsuite even fairly normal I/O
commands were a sure way to crash it, and crazy things like PM are
almost guaranteed to a) not actually be used by real application and
b) horrible buggy (as you've already noticed)

[toc] | [prev] | [next] | [standalone]

#1470800 — Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result pointer

On Fri, Aug 26, 2016 at 04:35:57PM +0200, Christoph Hellwig wrote:
> On Fri, Aug 26, 2016 at 07:31:33AM -0700, Andy Lutomirski wrote:
> >  - Consider *deleting* the SCSI translation layer's power saving code.
> > It looks almost entirely bogus to me.  It has an off-by-one in its
> > NPSS handling, it hardcodes power state indices which is total BS, it
> > ignores the distinction between operational and non-operational states
> > (which I think matters for non-APST usage).  It also seems likely to
> > be that it's never been used, since it's one of the formerly
> > crashy-looking set_features users.
> 
> Please go ahead and send a patch to delete it.  Adding the whole SCSI
> layer was a mistake to start with, and it's always been horribly buggy.
> Until I started running the libiscsi testsuite even fairly normal I/O
> commands were a sure way to crash it, and crazy things like PM are
> almost guaranteed to a) not actually be used by real application and
> b) horrible buggy (as you've already noticed)

Ack. If no distros or tools rely on the the SCSI crutch anymore, then
by all means, let's delete it. It's been disabled default for a while
now, and I think/hope everyone we care about has since migrated to
nvme awareness.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web