[PATCH] nvme: use __iowrite64_copy for SQs in the CMB

[PATCH] nvme: use __iowrite64_copy for SQs in the CMB

[Saar Gross]

Use __iowrite64_copy() instead of memcpy_toio() when copying SQ entries
to a SQ that's contained in the CMB.
__iowrite64_copy() writes to the MMIO in 64-bit units, whenever
possible.

Signed-off-by: Saar Gross <saar at annapurnalabs.com>
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 0248d0e..0e603f0 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -289,7 +289,7 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
        u16 tail = nvmeq->sq_tail;

        if (nvmeq->sq_cmds_io)
-               memcpy_toio(&nvmeq->sq_cmds_io[tail], cmd, sizeof(*cmd));
+               __iowrite64_copy(&nvmeq->sq_cmds_io[tail], cmd,
sizeof(*cmd) / 8);
        else
                memcpy(&nvmeq->sq_cmds[tail], cmd, sizeof(*cmd));

-- 
2.7.4

[PATCH] nvme: use __iowrite64_copy for SQs in the CMB

[Saar Gross]

Hi everyone,

Any comments on this patch? I didn't receive any so far.


On Sun, Oct 23, 2016@10:30 PM, Saar Gross <saar@annapurnalabs.com> wrote:
> Use __iowrite64_copy() instead of memcpy_toio() when copying SQ entries
> to a SQ that's contained in the CMB.
> __iowrite64_copy() writes to the MMIO in 64-bit units, whenever
> possible.
>
> Signed-off-by: Saar Gross <saar at annapurnalabs.com>
> ---
>  drivers/nvme/host/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 0248d0e..0e603f0 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -289,7 +289,7 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
>         u16 tail = nvmeq->sq_tail;
>
>         if (nvmeq->sq_cmds_io)
> -               memcpy_toio(&nvmeq->sq_cmds_io[tail], cmd, sizeof(*cmd));
> +               __iowrite64_copy(&nvmeq->sq_cmds_io[tail], cmd,
> sizeof(*cmd) / 8);
>         else
>                 memcpy(&nvmeq->sq_cmds[tail], cmd, sizeof(*cmd));
>
> --
> 2.7.4

[PATCH] nvme: use __iowrite64_copy for SQs in the CMB

[Christoph Hellwig]

On Mon, Nov 21, 2016@10:54:40AM +0200, Saar Gross wrote:
> Hi everyone,
> 
> Any comments on this patch? I didn't receive any so far.

Please explain why we would want to apply the patch, e.g. without
it some devices may be broken, or that on a particular device you
get much better performance (including numbers and an explanation of the
setup).

[PATCH] nvme: use __iowrite64_copy for SQs in the CMB

[Stephen Bates]

>>
>> Any comments on this patch? I didn't receive any so far.
>>
>
> Please explain why we would want to apply the patch, e.g. without
> it some devices may be broken, or that on a particular device you get much
> better performance (including numbers and an explanation of the setup).
>

Some PCIe devices have requirements on the alignment of PCIe writes into
them. Would this patch help with some of those devices?

Stephen

[PATCH] nvme: use __iowrite64_copy for SQs in the CMB

[Saar Gross]

Sorry for taking so long to reply: yes, it might help with some PCIe
devices with weird alignment requirements.

The patch itself doesn't fix any broken devices (possibly other than
those PCIe devices mentioned above), it only replaces the current
implementation of memcpy_toio() to __iowrite64_copy() which is more
suited when working with WC buffers. We've tested the patch on a CMB
enabled device where we got the same performance as the original
implementation, which is expected as using __iowrite64_copy() should
really only make a difference with a very stressed system.

Thanks,
Saar


On Tue, Nov 22, 2016@4:43 AM, Stephen Bates <sbates@raithlin.com> wrote:
>
>>>
>>> Any comments on this patch? I didn't receive any so far.
>>>
>>
>> Please explain why we would want to apply the patch, e.g. without
>> it some devices may be broken, or that on a particular device you get much
>> better performance (including numbers and an explanation of the setup).
>>
>
> Some PCIe devices have requirements on the alignment of PCIe writes into
> them. Would this patch help with some of those devices?
>
> Stephen
>

[PATCH] nvme: use __iowrite64_copy for SQs in the CMB

[Saar Gross]

Hi again,

Any other comments?

Thanks,
Saar


On Wed, Nov 30, 2016@10:31 AM, Saar Gross <saar@annapurnalabs.com> wrote:
> Sorry for taking so long to reply: yes, it might help with some PCIe
> devices with weird alignment requirements.
>
> The patch itself doesn't fix any broken devices (possibly other than
> those PCIe devices mentioned above), it only replaces the current
> implementation of memcpy_toio() to __iowrite64_copy() which is more
> suited when working with WC buffers. We've tested the patch on a CMB
> enabled device where we got the same performance as the original
> implementation, which is expected as using __iowrite64_copy() should
> really only make a difference with a very stressed system.
>
> Thanks,
> Saar
>
>
> On Tue, Nov 22, 2016@4:43 AM, Stephen Bates <sbates@raithlin.com> wrote:
>>
>>>>
>>>> Any comments on this patch? I didn't receive any so far.
>>>>
>>>
>>> Please explain why we would want to apply the patch, e.g. without
>>> it some devices may be broken, or that on a particular device you get much
>>> better performance (including numbers and an explanation of the setup).
>>>
>>
>> Some PCIe devices have requirements on the alignment of PCIe writes into
>> them. Would this patch help with some of those devices?
>>
>> Stephen
>>

[PATCH] nvme: use __iowrite64_copy for SQs in the CMB

[Christoph Hellwig]

On Mon, Dec 19, 2016@02:18:30PM +0200, Saar Gross wrote:
> Hi again,
> 
> Any other comments?

I still can't see a good explanation on why we'd want/need the patch.

[PATCH] nvme: use __iowrite64_copy for SQs in the CMB

[Saar Gross]

As already mentioned: the current implementation uses memcpy() which
is generic in nature, whereas the new implementation uses explicit
64-bit writes when possible.
What else are you looking for?

Thanks,
Saar


On Wed, Dec 21, 2016@12:22 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Dec 19, 2016@02:18:30PM +0200, Saar Gross wrote:
>> Hi again,
>>
>> Any other comments?
>
> I still can't see a good explanation on why we'd want/need the patch.

[PATCH] nvme: use __iowrite64_copy for SQs in the CMB

[Christoph Hellwig]

On Wed, Jan 04, 2017@09:22:34AM +0200, Saar Gross wrote:
> As already mentioned: the current implementation uses memcpy() which
> is generic in nature, whereas the new implementation uses explicit
> 64-bit writes when possible.
> What else are you looking for?

Any explanation of why this would benefit the driver in practive.
You've now explained the mechanics of your change in detail, but still
haven't shown in numbers or bugs fixed.

Note that we currently don't use 64-bit MMIO writes in the NVMe driver
at all, as we work around buggy Apple controllers.  This would be the
first one, and while I'm not against it at all, I'd rather have a good
reason to make the change instead of just having a drive-by patch in
the tree.