nvme-5.3 ssd performance regression

nvme-5.3 ssd performance regression

[Benjamin Herrenschmidt]

Hi !

Something I noticed while testing those patches for the Apple drives.

With 5.2.0 (and those patches):

[benh at mini ~]$ sudo hdparm -tf /dev/nvme0n1

/dev/nvme0n1:
 HDIO_DRIVE_CMD(identify) failed: Inappropriate ioctl for device
 Timing buffered disk reads: 5960 MB in  3.00 seconds = 1986.22 MB/sec

(The numbers are reasonably stable accross multiple runs)

With nvme-5.3 (and those patches & the ctrl->opts NULL fix)

[benh at mini linux]$ sudo hdparm -tf /dev/nvme0n1

/dev/nvme0n1:
 HDIO_DRIVE_CMD(identify) failed: Inappropriate ioctl for device
 Timing buffered disk reads: 4520 MB in  3.00 seconds = 1506.63 MB/sec
[benh at mini linux]$ sudo hdparm -tf /dev/nvme0n1

Here too, the numbers are quite stable during a given boot but
interestingly they seem to change from boot to boot, I also observed
1700 MB/s for example. It's a rather major regression.

The .config is identical in both cases (an x86_64-defconfig with a
small tweak or two, I removed more drivers mostly and made nvme a
module)

In both cases the io scheduler is mq-deadline. The device has a single
IO queue.

Cheers,
Ben.

nvme-5.3 ssd performance regression

[Ming Lei]

On Tue, Jul 16, 2019 at 9:05 AM Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> Hi !
>
> Something I noticed while testing those patches for the Apple drives.
>
> With 5.2.0 (and those patches):
>
> [benh at mini ~]$ sudo hdparm -tf /dev/nvme0n1
>
> /dev/nvme0n1:
>  HDIO_DRIVE_CMD(identify) failed: Inappropriate ioctl for device
>  Timing buffered disk reads: 5960 MB in  3.00 seconds = 1986.22 MB/sec
>
> (The numbers are reasonably stable accross multiple runs)
>
> With nvme-5.3 (and those patches & the ctrl->opts NULL fix)
>
> [benh at mini linux]$ sudo hdparm -tf /dev/nvme0n1
>
> /dev/nvme0n1:
>  HDIO_DRIVE_CMD(identify) failed: Inappropriate ioctl for device
>  Timing buffered disk reads: 4520 MB in  3.00 seconds = 1506.63 MB/sec
> [benh at mini linux]$ sudo hdparm -tf /dev/nvme0n1
>
> Here too, the numbers are quite stable during a given boot but
> interestingly they seem to change from boot to boot, I also observed
> 1700 MB/s for example. It's a rather major regression.
>
> The .config is identical in both cases (an x86_64-defconfig with a
> small tweak or two, I removed more drivers mostly and made nvme a
> module)
>
> In both cases the io scheduler is mq-deadline. The device has a single
> IO queue.

It should be caused by the following commit, and that patch uses
single mapping size to limit max hw sectors, and looks that way
is wrong. For example, on qemu, max_hw_sectors is decreased to
512. You can try to revert the patch and see if it makes a difference.

I feel we might need the max segment size limit too.

commit 7637de311bd2124b298a072852448b940d8a34b9
Author: Christoph Hellwig <hch at lst.de>
Date:   Wed Jul 3 09:54:44 2019 -0700

    nvme-pci: limit max_hw_sectors based on the DMA max mapping size



Thanks,
Ming Lei

nvme-5.3 ssd performance regression

[Christoph Hellwig]

On Tue, Jul 16, 2019@03:12:06PM +0800, Ming Lei wrote:
> It should be caused by the following commit, and that patch uses
> single mapping size to limit max hw sectors, and looks that way
> is wrong. For example, on qemu, max_hw_sectors is decreased to
> 512. You can try to revert the patch and see if it makes a difference.
> 
> I feel we might need the max segment size limit too.

No, with swiotlb it really is the whole request size that ?s limited
by the swiotlb buffer size.  Similar for potential iommus where
again it is the whole thing.

But looking at the implementation of dma_direct_max_mapping_size I
think we need to relax it - it currently limits the size as soon
as swiotlb is enabled, and not just if we need it for a given device.

We have some open coded versions of those checks elsewhere, let me
cook up a series to sort that mess out.

Ben, in then meantime can you try a revert of the commit Ming identified?
That is obviously not the proper fix, but it would help validating the
hypothesis.

nvme-5.3 ssd performance regression

[Benjamin Herrenschmidt]

On Tue, 2019-07-16@11:36 +0200, Christoph Hellwig wrote:
> On Tue, Jul 16, 2019@03:12:06PM +0800, Ming Lei wrote:
> > It should be caused by the following commit, and that patch uses
> > single mapping size to limit max hw sectors, and looks that way
> > is wrong. For example, on qemu, max_hw_sectors is decreased to
> > 512. You can try to revert the patch and see if it makes a difference.
> > 
> > I feel we might need the max segment size limit too.
> 
> No, with swiotlb it really is the whole request size that ?s limited
> by the swiotlb buffer size.  Similar for potential iommus where
> again it is the whole thing.
> 
> But looking at the implementation of dma_direct_max_mapping_size I
> think we need to relax it - it currently limits the size as soon
> as swiotlb is enabled, and not just if we need it for a given device.
> 
> We have some open coded versions of those checks elsewhere, let me
> cook up a series to sort that mess out.
> 
> Ben, in then meantime can you try a revert of the commit Ming identified?
> That is obviously not the proper fix, but it would help validating the
> hypothesis.

Sure I was going to do it anyways. I screwed up my remote access
tonight so it will have to wait for tomorrow though.

Cheers,
Ben.

nvme-5.3 ssd performance regression

[Ming Lei]

On Tue, Jul 16, 2019@11:36:17AM +0200, Christoph Hellwig wrote:
> On Tue, Jul 16, 2019@03:12:06PM +0800, Ming Lei wrote:
> > It should be caused by the following commit, and that patch uses
> > single mapping size to limit max hw sectors, and looks that way
> > is wrong. For example, on qemu, max_hw_sectors is decreased to
> > 512. You can try to revert the patch and see if it makes a difference.
> > 
> > I feel we might need the max segment size limit too.
> 
> No, with swiotlb it really is the whole request size that ?s limited
> by the swiotlb buffer size.  Similar for potential iommus where
> again it is the whole thing.

Documentation/DMA-API.txt:

		size_t
		dma_max_mapping_size(struct device *dev);
	
	Returns the maximum size of a mapping for the device. The size parameter
	of the mapping functions like dma_map_single(), dma_map_page() and
	others should not be larger than the returned value.

And dma_map_single() & dma_map_page() is usually for mapping single
element of SGL, instead of whole SGL.

Thanks,
Ming

nvme-5.3 ssd performance regression

[Christoph Hellwig]

On Tue, Jul 16, 2019@08:47:42PM +0800, Ming Lei wrote:
> > No, with swiotlb it really is the whole request size that ?s limited
> > by the swiotlb buffer size.  Similar for potential iommus where
> > again it is the whole thing.
> 
> Documentation/DMA-API.txt:
> 
> 		size_t
> 		dma_max_mapping_size(struct device *dev);
> 	
> 	Returns the maximum size of a mapping for the device. The size parameter
> 	of the mapping functions like dma_map_single(), dma_map_page() and
> 	others should not be larger than the returned value.
> 
> And dma_map_single() & dma_map_page() is usually for mapping single
> element of SGL, instead of whole SGL.

... and others.  And if you look at the actual swiotlb code it is
pretty clear that the whole request counts.  But I'll make sure to
make clarify the documentation when fixing the issue.

nvme-5.3 ssd performance regression

[Benjamin Herrenschmidt]

On Tue, 2019-07-16@15:12 +0800, Ming Lei wrote:
> I feel we might need the max segment size limit too.
> 
> commit 7637de311bd2124b298a072852448b940d8a34b9
> Author: Christoph Hellwig <hch at lst.de>
> Date:   Wed Jul 3 09:54:44 2019 -0700
> 
>     nvme-pci: limit max_hw_sectors based on the DMA max mapping size

Yes. Reverting this brings the speed back to 2GB/s

Cheers,
Ben.

nvme-5.3 ssd performance regression

[Christoph Hellwig]

On Wed, Jul 17, 2019@10:15:00AM +1000, Benjamin Herrenschmidt wrote:
> Yes. Reverting this brings the speed back to 2GB/s

Thanks.  Can you try this series as well:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-max-mapping-size-fix

nvme-5.3 ssd performance regression

[Benjamin Herrenschmidt]

On Wed, 2019-07-17@06:26 +0200, Christoph Hellwig wrote:
> On Wed, Jul 17, 2019 at 10:15:00AM +1000, Benjamin Herrenschmidt
> wrote:
> > Yes. Reverting this brings the speed back to 2GB/s
> 
> Thanks.  Can you try this series as well:
> 
> 
http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-max-mapping-size-fix

With or without

 "nvme-pci: limit max_hw_sectors based on the DMA max mapping size"

?

Ben.

nvme-5.3 ssd performance regression

[Christoph Hellwig]

On Wed, Jul 17, 2019@02:33:59PM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2019-07-17@06:26 +0200, Christoph Hellwig wrote:
> > On Wed, Jul 17, 2019 at 10:15:00AM +1000, Benjamin Herrenschmidt
> > wrote:
> > > Yes. Reverting this brings the speed back to 2GB/s
> > 
> > Thanks.  Can you try this series as well:
> > 
> > 
> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-max-mapping-size-fix
> 
> With or without
> 
>  "nvme-pci: limit max_hw_sectors based on the DMA max mapping size"
> 
> ?

With.  Just pull in the branch :)

nvme-5.3 ssd performance regression

[Benjamin Herrenschmidt]

On Wed, 2019-07-17@06:40 +0200, Christoph Hellwig wrote:


http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-max-mapping-size-fix
> > 
> > With or without
> > 
> >  "nvme-pci: limit max_hw_sectors based on the DMA max mapping size"
> > 
> > ?
> 
> With.  Just pull in the branch :)

Ah oops :-) Brain not engaged clearly ...

I'll get back to you when I get past the lockups caused by the Intel
graphic driver... Something is trying to do synchronous module loads
from async init calls and its' not happy.

Cheers,
Ben.

nvme-5.3 ssd performance regression

[Benjamin Herrenschmidt]

On Wed, 2019-07-17@06:26 +0200, Christoph Hellwig wrote:
> On Wed, Jul 17, 2019 at 10:15:00AM +1000, Benjamin Herrenschmidt
> wrote:
> > Yes. Reverting this brings the speed back to 2GB/s
> 
> Thanks.  Can you try this series as well:
> 
> 
http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-max-mapping-size-fix

This works. The performance is normal.

Cheers,
Ben.