nvme-pci: about page_size of DMA pool

nvme-pci: about page_size of DMA pool

[Minwoo Im]

Hi All,

It seems that _PAGE_SIZE_ and _dev->ctrl.page_size_ might be different.
For now, nvme_setup_prp_pools() attempts to create PRP page DMA pool
with PAGE_SIZE instead of dev->ctrl.page_size.

By the way, in nvme_pci_setup_prps(), PRP lists are built by
dev->ctrl.page_size like following code.

for (;;) {
         if (i == page_size >> 3) {
                  ^^^^^^^^^
             __le64 *old_prp_list = prp_list;
             prp_list = dma_pool_alloc(pool, GFP_ATOMIC, &prp_dma);

if dev->ctrl.page_size should be used as is, I guess DMA pool should be
created in dev->ctrl.page_size (But at that time of
nvme_setup_prp_pools(), dev->ctrl.page_size may not be set properly)
somehow instead of PAGE_SIZE.

Additionally, It seems that page_shift in nvme_enable_ctrl() is now
hard-coded to 12 which means dev->ctrl.page_size will always be 4096,
though.


Q1. Should dev->prp_page_pool be created with dev->ctrl.page_size
instead of PAGE_SIZE?

Q2. Is there any special reason why page_shift in nvme_enable_ctrl()
is hard-coded to 12, not PAGE_SHIFT?

Always welcome directives and comments. :)

Sincerely,
Minwoo Im

nvme-pci: about page_size of DMA pool

[Keith Busch]

On Sun, Feb 18, 2018@04:52:34PM +0900, Minwoo Im wrote:
> It seems that _PAGE_SIZE_ and _dev->ctrl.page_size_ might be different.
> For now, nvme_setup_prp_pools() attempts to create PRP page DMA pool
> with PAGE_SIZE instead of dev->ctrl.page_size.
>
> By the way, in nvme_pci_setup_prps(), PRP lists are built by
> dev->ctrl.page_size like following code.
> 
> for (;;) {
>         if (i == page_size >> 3) {
>                  ^^^^^^^^^
>             __le64 *old_prp_list = prp_list;
>             prp_list = dma_pool_alloc(pool, GFP_ATOMIC, &prp_dma);
> 
> if dev->ctrl.page_size should be used as is, I guess DMA pool should be
> created in dev->ctrl.page_size (But at that time of
> nvme_setup_prp_pools(), dev->ctrl.page_size may not be set properly)
> somehow instead of PAGE_SIZE.

Good point, but as long as we know it's hard-coded to 4k, the order
doesn't really matter.

> Additionally, It seems that page_shift in nvme_enable_ctrl() is now
> hard-coded to 12 which means dev->ctrl.page_size will always be 4096,
> though.
> 
> 
> Q1. Should dev->prp_page_pool be created with dev->ctrl.page_size
> instead of PAGE_SIZE?

Yeah, the current method looks like it may potentially be over-allocating
some memory for very large IO transfers. The size of the "large" pool
ought to be the same as ctrl.page_size.
 
> 
> Q2. Is there any special reason why page_shift in nvme_enable_ctrl()
> is hard-coded to 12, not PAGE_SHIFT?

Some CPU architectures have different alignment when comparing DMA mapped
addresses with the virual address, so we have to go to the lowest common
denominator. Previous discussion here:

  http://lists.infradead.org/pipermail/linux-nvme/2015-October/002893.html

nvme-pci: about page_size of DMA pool

[Minwoo Im]

On 02/21/2018 01:06 AM, Keith Busch wrote:
>> Q1. Should dev->prp_page_pool be created with dev->ctrl.page_size
>> instead of PAGE_SIZE?
> 
> Yeah, the current method looks like it may potentially be over-allocating
> some memory for very large IO transfers. The size of the "large" pool
> ought to be the same as ctrl.page_size.
>   
>>
>> Q2. Is there any special reason why page_shift in nvme_enable_ctrl()
>> is hard-coded to 12, not PAGE_SHIFT?
> 
> Some CPU architectures have different alignment when comparing DMA mapped
> addresses with the virual address, so we have to go to the lowest common
> denominator. Previous discussion here:
> 
>    http://lists.infradead.org/pipermail/linux-nvme/2015-October/002893.html
> 

Thanks for your kind response, Keith. :)