mbox series

[0/5] optimize some data structure in nvme

Message ID cover.1682941568.git.christophe.jaillet@wanadoo.fr
Headers
Series optimize some data structure in nvme |

Message

Christophe JAILLET May 1, 2023, 12:40 p.m. UTC 
  This serie is a proposal to slighly optimize the memory needed for some
structures used in nvme.

This follows the discussion in [1].

Honnestly, I'm not convinced that this serie really brings semething.
Because of the way memory alocation works, and its over-allocation to try to
avoid memory fragmentation, some limited gains are most of the time useless.

It could still help:
   - many holes in structure can, at some point, have its size reach a threshold
     (this is specially true if such structures are allocated with kcalloc() or
     kmalloc_array())
   - it can save some space in some other structures if embedded in them
   - it can save a few cycles if the structure is memcpy()'ed or zeroed, for
     example
   - can reduce cache usage

With that in mind, patch 3 is a win, patch 4 is likely a win, the other ones are
much more theorical.

The changes are really limited, so even if the gain is marginal, maybe it still
makes sense to merge them.

Each patch gives the layout generated by pahole before and after the patch.

[1]: https://lore.kernel.org/all/67a9e53e-4ac9-7ba8-9713-96c1dfe1e341@nvidia.com/

Christophe JAILLET (5):
  nvmet: Reorder fields in 'struct nvmet_sq'
  nvmet: Reorder fields in 'struct nvme_ctrl'
  nvmet: Reorder fields in 'struct nvmf_ctrl_options'
  nvmet: Reorder fields in 'struct nvme_dhchap_queue_context'
  nvmet: Reorder fields in 'struct nvmefc_fcp_req'

 drivers/nvme/host/auth.c       |  6 +++---
 drivers/nvme/host/fabrics.h    |  8 ++++----
 drivers/nvme/host/nvme.h       |  6 +++---
 drivers/nvme/target/nvmet.h    |  4 ++--
 include/linux/nvme-fc-driver.h | 10 +++++-----
 5 files changed, 17 insertions(+), 17 deletions(-)

Comments

Sagi Grimberg May 1, 2023, 1:57 p.m. UTC | #1 
> This serie is a proposal to slighly optimize the memory needed for some
> structures used in nvme.
> 
> This follows the discussion in [1].
> 
> Honnestly, I'm not convinced that this serie really brings semething.
> Because of the way memory alocation works, and its over-allocation to try to
> avoid memory fragmentation, some limited gains are most of the time useless.
> 
> It could still help:
>     - many holes in structure can, at some point, have its size reach a threshold
>       (this is specially true if such structures are allocated with kcalloc() or
>       kmalloc_array())
>     - it can save some space in some other structures if embedded in them
>     - it can save a few cycles if the structure is memcpy()'ed or zeroed, for
>       example
>     - can reduce cache usage
> 
> With that in mind, patch 3 is a win, patch 4 is likely a win, the other ones are
> much more theorical.
> 
> The changes are really limited, so even if the gain is marginal, maybe it still
> makes sense to merge them.

Don't see why not, given they make do the structures smaller.

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Christoph Hellwig May 12, 2023, 8:26 p.m. UTC | #2 
Thanks,

the whole series looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Chaitanya Kulkarni May 13, 2023, 1:01 a.m. UTC | #3 
On 5/1/23 05:40, Christophe JAILLET wrote:
> This serie is a proposal to slighly optimize the memory needed for some
> structures used in nvme.
>
> This follows the discussion in [1].
>
> Honnestly, I'm not convinced that this serie really brings semething.
> Because of the way memory alocation works, and its over-allocation to try to
> avoid memory fragmentation, some limited gains are most of the time useless.
>
> It could still help:
>     - many holes in structure can, at some point, have its size reach a threshold
>       (this is specially true if such structures are allocated with kcalloc() or
>       kmalloc_array())
>     - it can save some space in some other structures if embedded in them
>     - it can save a few cycles if the structure is memcpy()'ed or zeroed, for
>       example
>     - can reduce cache usage
>
> With that in mind, patch 3 is a win, patch 4 is likely a win, the other ones are
> much more theorical.
>
> The changes are really limited, so even if the gain is marginal, maybe it still
> makes sense to merge them.
>
> Each patch gives the layout generated by pahole before and after the patch.
>
> [1]: https://lore.kernel.org/all/67a9e53e-4ac9-7ba8-9713-96c1dfe1e341@nvidia.com/
>
> Christophe JAILLET (5):
>    nvmet: Reorder fields in 'struct nvmet_sq'
>    nvmet: Reorder fields in 'struct nvme_ctrl'
>    nvmet: Reorder fields in 'struct nvmf_ctrl_options'
>    nvmet: Reorder fields in 'struct nvme_dhchap_queue_context'
>    nvmet: Reorder fields in 'struct nvmefc_fcp_req'
>
>   drivers/nvme/host/auth.c       |  6 +++---
>   drivers/nvme/host/fabrics.h    |  8 ++++----
>   drivers/nvme/host/nvme.h       |  6 +++---
>   drivers/nvme/target/nvmet.h    |  4 ++--
>   include/linux/nvme-fc-driver.h | 10 +++++-----
>   5 files changed, 17 insertions(+), 17 deletions(-)
>

thanks a lot for doing this and following the comment on the other patch.

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck
Keith Busch May 15, 2023, 5:13 p.m. UTC | #4 
Thanks, applied to nvme-6.5.