mbox series

[v3,0/5] mbuf dynamic field expansion

Message ID cover.1663767715.git.sthotton@marvell.com (mailing list archive)
Headers
Series mbuf dynamic field expansion |

Message

Shijith Thotton Sept. 21, 2022, 1:56 p.m. UTC
  This is a continuation of the discussions[1] to add mbuf physical address field to dynamic field.
Previous version was to add PA field to dynamic field area based on the EAL IOVA mode option. It was
deemed unsafe as some components could still use the PA field without checking IOVA mode and there
are drivers which need PA to work. One suggestion was to make the IOVA mode check at compile time so
that drivers which need PA can be disabled during build. This series adds this new meson build
options. Second patch adds mbuf PA field to dynamic field on such builds. Last two patches enable
Marvell cnxk PMDs and software PMDs in IOVA as VA build as they work without PA field.

1. https://inbox.dpdk.org/dev/57d2ab7fff672716d37ba4078e2e3bb2db126607.1656605763.git.sthotton@marvell.com/.

v3:
 * Cleared use of buf_iova from cnxk PMD.

v2:
 * Used RTE_IOVA_AS_VA instread of rte_is_iova_as_va_build().
 * Moved mbuf next pointer to first cacheline if RTE_IOVA_AS_VA = 1.

Shijith Thotton (5):
  build: add meson option to configure IOVA mode as VA
  mbuf: add second dynamic field member for VA only build
  lib: move mbuf next pointer to first cache line
  drivers: mark Marvell cnxk PMDs work with IOVA as VA
  drivers: mark software PMDs work with IOVA as VA

 app/test-bbdev/test_bbdev_perf.c         |  2 +-
 app/test-crypto-perf/cperf_test_common.c |  5 +--
 app/test/test_bpf.c                      |  2 +-
 app/test/test_dmadev.c                   | 33 ++++++--------
 app/test/test_mbuf.c                     | 12 +++---
 app/test/test_pcapng.c                   |  2 +-
 config/arm/meson.build                   |  8 +++-
 config/meson.build                       |  1 +
 drivers/common/cnxk/meson.build          |  1 +
 drivers/crypto/armv8/meson.build         |  1 +
 drivers/crypto/cnxk/cn10k_ipsec_la_ops.h |  4 +-
 drivers/crypto/cnxk/cn9k_ipsec_la_ops.h  |  2 +-
 drivers/crypto/cnxk/meson.build          |  2 +
 drivers/crypto/ipsec_mb/meson.build      |  1 +
 drivers/crypto/null/meson.build          |  1 +
 drivers/crypto/openssl/meson.build       |  1 +
 drivers/dma/cnxk/meson.build             |  1 +
 drivers/dma/skeleton/meson.build         |  1 +
 drivers/event/cnxk/meson.build           |  1 +
 drivers/event/dsw/meson.build            |  1 +
 drivers/event/opdl/meson.build           |  1 +
 drivers/event/skeleton/meson.build       |  1 +
 drivers/event/sw/meson.build             |  1 +
 drivers/mempool/bucket/meson.build       |  1 +
 drivers/mempool/cnxk/meson.build         |  1 +
 drivers/mempool/ring/meson.build         |  1 +
 drivers/mempool/stack/meson.build        |  1 +
 drivers/meson.build                      |  6 +++
 drivers/net/af_packet/meson.build        |  1 +
 drivers/net/af_xdp/meson.build           |  2 +
 drivers/net/bonding/meson.build          |  1 +
 drivers/net/cnxk/cn10k_tx.h              | 55 +++++++-----------------
 drivers/net/cnxk/cn9k_tx.h               | 55 +++++++-----------------
 drivers/net/cnxk/cnxk_ethdev.h           |  1 -
 drivers/net/cnxk/meson.build             |  1 +
 drivers/net/failsafe/meson.build         |  1 +
 drivers/net/memif/meson.build            |  1 +
 drivers/net/null/meson.build             |  1 +
 drivers/net/pcap/meson.build             |  1 +
 drivers/net/ring/meson.build             |  1 +
 drivers/net/tap/meson.build              |  1 +
 drivers/raw/cnxk_bphy/meson.build        |  1 +
 drivers/raw/cnxk_gpio/meson.build        |  1 +
 drivers/raw/skeleton/meson.build         |  1 +
 lib/eal/linux/eal.c                      |  7 +++
 lib/mbuf/rte_mbuf.c                      |  8 ++--
 lib/mbuf/rte_mbuf.h                      | 17 +++++---
 lib/mbuf/rte_mbuf_core.h                 | 55 ++++++++++++++++++------
 lib/mbuf/rte_mbuf_dyn.c                  |  2 +
 lib/meson.build                          |  3 ++
 lib/vhost/vhost.h                        |  2 +-
 lib/vhost/vhost_crypto.c                 | 54 +++++++++++++++++------
 meson_options.txt                        |  2 +
 53 files changed, 220 insertions(+), 150 deletions(-)
  

Comments

Shijith Thotton Sept. 28, 2022, 5:41 a.m. UTC | #1
>
>This is a continuation of the discussions[1] to add mbuf physical address field to
>dynamic field.
>Previous version was to add PA field to dynamic field area based on the EAL IOVA
>mode option. It was
>deemed unsafe as some components could still use the PA field without checking
>IOVA mode and there
>are drivers which need PA to work. One suggestion was to make the IOVA mode
>check at compile time so
>that drivers which need PA can be disabled during build. This series adds this new
>meson build
>options. Second patch adds mbuf PA field to dynamic field on such builds. Last two
>patches enable
>Marvell cnxk PMDs and software PMDs in IOVA as VA build as they work without
>PA field.
>
>1.
>https://inbox.dpdk.org/dev/57d2ab7fff672716d37ba4078e2e3bb2db126607.16566
>05763.git.sthotton@marvell.com/.
>
>v3:
> * Cleared use of buf_iova from cnxk PMD.
>
>v2:
> * Used RTE_IOVA_AS_VA instread of rte_is_iova_as_va_build().
> * Moved mbuf next pointer to first cacheline if RTE_IOVA_AS_VA = 1.
>
>Shijith Thotton (5):
>  build: add meson option to configure IOVA mode as VA
>  mbuf: add second dynamic field member for VA only build
>  lib: move mbuf next pointer to first cache line
>  drivers: mark Marvell cnxk PMDs work with IOVA as VA
>  drivers: mark software PMDs work with IOVA as VA
>

Hi All,

Please comment if any changes are needed on the series.
Right now, there is 1 ack from Morten.

Thanks,
Shijith
  
Olivier Matz Sept. 28, 2022, 12:52 p.m. UTC | #2
Hi Shijith,

On Wed, Sep 21, 2022 at 07:26:16PM +0530, Shijith Thotton wrote:
> This is a continuation of the discussions[1] to add mbuf physical address field to dynamic field.
> Previous version was to add PA field to dynamic field area based on the EAL IOVA mode option. It was
> deemed unsafe as some components could still use the PA field without checking IOVA mode and there
> are drivers which need PA to work. One suggestion was to make the IOVA mode check at compile time so
> that drivers which need PA can be disabled during build. This series adds this new meson build
> options. Second patch adds mbuf PA field to dynamic field on such builds. Last two patches enable
> Marvell cnxk PMDs and software PMDs in IOVA as VA build as they work without PA field.

Thank you for this patchset.

To be honnest, initially I was really reserved to remove the use of
buf_iova for some specific platforms.

But what made me change my mind is that the removal if buf_iova will
likely happen in the long-term future. It looks there is a consensus on
this. I think your patchset is a good way to prepare this transition.

What is missing, I think, is a good description of the problem you are
solving:

- more space for dynamic mbuf fields -> why? can you give more detail about
  this need?
- increase performance -> you previously said that it was not your point,
  but if we move the next field into the first cache line, I think this
  has to be highlighted. Out of curiosity, did you made measurements?

I'm sending separate comments as replies to the patches.

Olivier


> 
> 1. https://inbox.dpdk.org/dev/57d2ab7fff672716d37ba4078e2e3bb2db126607.1656605763.git.sthotton@marvell.com/.
> 
> v3:
>  * Cleared use of buf_iova from cnxk PMD.
> 
> v2:
>  * Used RTE_IOVA_AS_VA instread of rte_is_iova_as_va_build().
>  * Moved mbuf next pointer to first cacheline if RTE_IOVA_AS_VA = 1.
> 
> Shijith Thotton (5):
>   build: add meson option to configure IOVA mode as VA
>   mbuf: add second dynamic field member for VA only build
>   lib: move mbuf next pointer to first cache line
>   drivers: mark Marvell cnxk PMDs work with IOVA as VA
>   drivers: mark software PMDs work with IOVA as VA
> 
>  app/test-bbdev/test_bbdev_perf.c         |  2 +-
>  app/test-crypto-perf/cperf_test_common.c |  5 +--
>  app/test/test_bpf.c                      |  2 +-
>  app/test/test_dmadev.c                   | 33 ++++++--------
>  app/test/test_mbuf.c                     | 12 +++---
>  app/test/test_pcapng.c                   |  2 +-
>  config/arm/meson.build                   |  8 +++-
>  config/meson.build                       |  1 +
>  drivers/common/cnxk/meson.build          |  1 +
>  drivers/crypto/armv8/meson.build         |  1 +
>  drivers/crypto/cnxk/cn10k_ipsec_la_ops.h |  4 +-
>  drivers/crypto/cnxk/cn9k_ipsec_la_ops.h  |  2 +-
>  drivers/crypto/cnxk/meson.build          |  2 +
>  drivers/crypto/ipsec_mb/meson.build      |  1 +
>  drivers/crypto/null/meson.build          |  1 +
>  drivers/crypto/openssl/meson.build       |  1 +
>  drivers/dma/cnxk/meson.build             |  1 +
>  drivers/dma/skeleton/meson.build         |  1 +
>  drivers/event/cnxk/meson.build           |  1 +
>  drivers/event/dsw/meson.build            |  1 +
>  drivers/event/opdl/meson.build           |  1 +
>  drivers/event/skeleton/meson.build       |  1 +
>  drivers/event/sw/meson.build             |  1 +
>  drivers/mempool/bucket/meson.build       |  1 +
>  drivers/mempool/cnxk/meson.build         |  1 +
>  drivers/mempool/ring/meson.build         |  1 +
>  drivers/mempool/stack/meson.build        |  1 +
>  drivers/meson.build                      |  6 +++
>  drivers/net/af_packet/meson.build        |  1 +
>  drivers/net/af_xdp/meson.build           |  2 +
>  drivers/net/bonding/meson.build          |  1 +
>  drivers/net/cnxk/cn10k_tx.h              | 55 +++++++-----------------
>  drivers/net/cnxk/cn9k_tx.h               | 55 +++++++-----------------
>  drivers/net/cnxk/cnxk_ethdev.h           |  1 -
>  drivers/net/cnxk/meson.build             |  1 +
>  drivers/net/failsafe/meson.build         |  1 +
>  drivers/net/memif/meson.build            |  1 +
>  drivers/net/null/meson.build             |  1 +
>  drivers/net/pcap/meson.build             |  1 +
>  drivers/net/ring/meson.build             |  1 +
>  drivers/net/tap/meson.build              |  1 +
>  drivers/raw/cnxk_bphy/meson.build        |  1 +
>  drivers/raw/cnxk_gpio/meson.build        |  1 +
>  drivers/raw/skeleton/meson.build         |  1 +
>  lib/eal/linux/eal.c                      |  7 +++
>  lib/mbuf/rte_mbuf.c                      |  8 ++--
>  lib/mbuf/rte_mbuf.h                      | 17 +++++---
>  lib/mbuf/rte_mbuf_core.h                 | 55 ++++++++++++++++++------
>  lib/mbuf/rte_mbuf_dyn.c                  |  2 +
>  lib/meson.build                          |  3 ++
>  lib/vhost/vhost.h                        |  2 +-
>  lib/vhost/vhost_crypto.c                 | 54 +++++++++++++++++------
>  meson_options.txt                        |  2 +
>  53 files changed, 220 insertions(+), 150 deletions(-)
> 
> -- 
> 2.25.1
>
  
Shijith Thotton Sept. 29, 2022, 4:51 a.m. UTC | #3
Hi Olivier,

Thanks for the review.

>On Wed, Sep 21, 2022 at 07:26:16PM +0530, Shijith Thotton wrote:
>> This is a continuation of the discussions[1] to add mbuf physical address field to
>dynamic field.
>> Previous version was to add PA field to dynamic field area based on the EAL
>IOVA mode option. It was
>> deemed unsafe as some components could still use the PA field without
>checking IOVA mode and there
>> are drivers which need PA to work. One suggestion was to make the IOVA mode
>check at compile time so
>> that drivers which need PA can be disabled during build. This series adds this
>new meson build
>> options. Second patch adds mbuf PA field to dynamic field on such builds. Last
>two patches enable
>> Marvell cnxk PMDs and software PMDs in IOVA as VA build as they work without
>PA field.
>
>Thank you for this patchset.
>
>To be honnest, initially I was really reserved to remove the use of
>buf_iova for some specific platforms.
>
>But what made me change my mind is that the removal if buf_iova will
>likely happen in the long-term future. It looks there is a consensus on
>this. I think your patchset is a good way to prepare this transition.
>
>What is missing, I think, is a good description of the problem you are
>solving:
>
>- more space for dynamic mbuf fields -> why? can you give more detail about
>  this need?
 
Idea was to let app/lib use an additional 8-bytes of dynamic area.

>- increase performance -> you previously said that it was not your point,
>  but if we move the next field into the first cache line, I think this
>  has to be highlighted. Out of curiosity, did you made measurements?
>

I'm yet to do it. I will update, once I have the numbers.

>>
>> 1. https://urldefense.proofpoint.com/v2/url?u=https-
>3A__inbox.dpdk.org_dev_57d2ab7fff672716d37ba4078e2e3bb2db126607.1656605
>763.git.sthotton-
>40marvell.com_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=G9w4KsPaQLACBf
>GCL35PtiRH996yqJDxAZwrWegU2qQ&m=O9JeIb0lfExyVnC8dV3WUADowh165KkS
>3s9JrmAjLwj8Uw5Iyb0tqSQ9YvQWpbIc&s=DaHEYGwUqUmAFmQ9Jkj8jGnOS4aw8
>iZ8Tcww-jPTdFE&e=  .
>>
>> v3:
>>  * Cleared use of buf_iova from cnxk PMD.
>>
>> v2:
>>  * Used RTE_IOVA_AS_VA instread of rte_is_iova_as_va_build().
>>  * Moved mbuf next pointer to first cacheline if RTE_IOVA_AS_VA = 1.
>>
>> Shijith Thotton (5):
>>   build: add meson option to configure IOVA mode as VA
>>   mbuf: add second dynamic field member for VA only build
>>   lib: move mbuf next pointer to first cache line
>>   drivers: mark Marvell cnxk PMDs work with IOVA as VA
>>   drivers: mark software PMDs work with IOVA as VA
>>
>>  app/test-bbdev/test_bbdev_perf.c         |  2 +-
>>  app/test-crypto-perf/cperf_test_common.c |  5 +--
>>  app/test/test_bpf.c                      |  2 +-
>>  app/test/test_dmadev.c                   | 33 ++++++--------
>>  app/test/test_mbuf.c                     | 12 +++---
>>  app/test/test_pcapng.c                   |  2 +-
>>  config/arm/meson.build                   |  8 +++-
>>  config/meson.build                       |  1 +
>>  drivers/common/cnxk/meson.build          |  1 +
>>  drivers/crypto/armv8/meson.build         |  1 +
>>  drivers/crypto/cnxk/cn10k_ipsec_la_ops.h |  4 +-
>>  drivers/crypto/cnxk/cn9k_ipsec_la_ops.h  |  2 +-
>>  drivers/crypto/cnxk/meson.build          |  2 +
>>  drivers/crypto/ipsec_mb/meson.build      |  1 +
>>  drivers/crypto/null/meson.build          |  1 +
>>  drivers/crypto/openssl/meson.build       |  1 +
>>  drivers/dma/cnxk/meson.build             |  1 +
>>  drivers/dma/skeleton/meson.build         |  1 +
>>  drivers/event/cnxk/meson.build           |  1 +
>>  drivers/event/dsw/meson.build            |  1 +
>>  drivers/event/opdl/meson.build           |  1 +
>>  drivers/event/skeleton/meson.build       |  1 +
>>  drivers/event/sw/meson.build             |  1 +
>>  drivers/mempool/bucket/meson.build       |  1 +
>>  drivers/mempool/cnxk/meson.build         |  1 +
>>  drivers/mempool/ring/meson.build         |  1 +
>>  drivers/mempool/stack/meson.build        |  1 +
>>  drivers/meson.build                      |  6 +++
>>  drivers/net/af_packet/meson.build        |  1 +
>>  drivers/net/af_xdp/meson.build           |  2 +
>>  drivers/net/bonding/meson.build          |  1 +
>>  drivers/net/cnxk/cn10k_tx.h              | 55 +++++++-----------------
>>  drivers/net/cnxk/cn9k_tx.h               | 55 +++++++-----------------
>>  drivers/net/cnxk/cnxk_ethdev.h           |  1 -
>>  drivers/net/cnxk/meson.build             |  1 +
>>  drivers/net/failsafe/meson.build         |  1 +
>>  drivers/net/memif/meson.build            |  1 +
>>  drivers/net/null/meson.build             |  1 +
>>  drivers/net/pcap/meson.build             |  1 +
>>  drivers/net/ring/meson.build             |  1 +
>>  drivers/net/tap/meson.build              |  1 +
>>  drivers/raw/cnxk_bphy/meson.build        |  1 +
>>  drivers/raw/cnxk_gpio/meson.build        |  1 +
>>  drivers/raw/skeleton/meson.build         |  1 +
>>  lib/eal/linux/eal.c                      |  7 +++
>>  lib/mbuf/rte_mbuf.c                      |  8 ++--
>>  lib/mbuf/rte_mbuf.h                      | 17 +++++---
>>  lib/mbuf/rte_mbuf_core.h                 | 55 ++++++++++++++++++------
>>  lib/mbuf/rte_mbuf_dyn.c                  |  2 +
>>  lib/meson.build                          |  3 ++
>>  lib/vhost/vhost.h                        |  2 +-
>>  lib/vhost/vhost_crypto.c                 | 54 +++++++++++++++++------
>>  meson_options.txt                        |  2 +
>>  53 files changed, 220 insertions(+), 150 deletions(-)
>>
>> --
>> 2.25.1
>>
  
Thomas Monjalon Oct. 7, 2022, 1:50 p.m. UTC | #4
21/09/2022 15:56, Shijith Thotton:
> This is a continuation of the discussions[1] to add mbuf physical address field to dynamic field.
> Previous version was to add PA field to dynamic field area based on the EAL IOVA mode option. It was
> deemed unsafe as some components could still use the PA field without checking IOVA mode and there
> are drivers which need PA to work. One suggestion was to make the IOVA mode check at compile time so
> that drivers which need PA can be disabled during build. This series adds this new meson build
> options. Second patch adds mbuf PA field to dynamic field on such builds. Last two patches enable
> Marvell cnxk PMDs and software PMDs in IOVA as VA build as they work without PA field.

Shijith, in case it was not clear,
we can accept this change only in -rc1 closing today,
and we didn't receive the expected v4 yet.
  
Shijith Thotton Oct. 7, 2022, 7:35 p.m. UTC | #5
>> This is a continuation of the discussions[1] to add mbuf physical address field to
>dynamic field.
>> Previous version was to add PA field to dynamic field area based on the EAL
>IOVA mode option. It was
>> deemed unsafe as some components could still use the PA field without
>checking IOVA mode and there
>> are drivers which need PA to work. One suggestion was to make the IOVA mode
>check at compile time so
>> that drivers which need PA can be disabled during build. This series adds this
>new meson build
>> options. Second patch adds mbuf PA field to dynamic field on such builds. Last
>two patches enable
>> Marvell cnxk PMDs and software PMDs in IOVA as VA build as they work without
>PA field.
>
>Shijith, in case it was not clear,
>we can accept this change only in -rc1 closing today,
>and we didn't receive the expected v4 yet.
>

Hi Thomas,

Sorry for the delay, I was not aware of this deadline. I have posted v4 now.
https://patchwork.dpdk.org/project/dpdk/list/?series=25039