ethdev: introduce enable_driver_sdk to install driver headers

Introduce a meson option enable_driver_sdk when true installs internal driver headers for ethdev. this allows drivers that do not depend on stable api/abi to be built external to the dpdk source tree.

Signed-off-by Tyler Retzlaff <[email protected]>

---

it's still unclear to me if we should be renaming the headers:

   ethdev_driver.h -> rte_ethdev_driver.h
   ethdev_pci.h -> rte_ethdev_pci.h
   ethdev_vdev.h -> rte_ethdev_vdev.h

  lib/librte_ethdev/meson.build | 5 +++++
  meson_options.txt             | 2 ++
  2 files changed, 7 insertions(+)

lib/librte_ethdev/meson.build +0 -0

index c37b2e377..7ecdec6f0 100644

Quoted 22 lines · Tyler Retzlaff

Introduce a meson option enable_driver_sdk when true installs internal driver headers for ethdev. this allows drivers that do not depend on stable api/abi to be built external to the dpdk source tree.

Signed-off-by Tyler Retzlaff <[email protected]>

---

it's still unclear to me if we should be renaming the headers:

   ethdev_driver.h -> rte_ethdev_driver.h
   ethdev_pci.h -> rte_ethdev_pci.h
   ethdev_vdev.h -> rte_ethdev_vdev.h

  lib/librte_ethdev/meson.build | 5 +++++
  meson_options.txt             | 2 ++
  2 files changed, 7 insertions(+)

lib/librte_ethdev/meson.build +0 -0

index c37b2e377..7ecdec6f0 100644

lib/librte_ethdev/meson.build headers = files('rte_ethdev.h', · line 27

Quoted 8 lines

lib/librte_ethdev/meson.build +5 -0

@@ -20,6 +20,11 @@ headers = files('rte_ethdev.h',
      'rte_mtr_driver.h',
      'rte_tm.h',
      'rte_tm_driver.h')
+if get_option('enable_driver_sdk')
+headers += files('ethdev_driver.h',
+     'ethdev_pci.h',
+     'ethdev_vdev.h')
+endif

Instead of adding the "get_option('enable_driver_sdk')" checks to the modules, what about a more generic solution, like:

modules assign relevant headers into a new variable, let's say 'pmd_headers', and in a high level meson file, all 'pmd_headers' are installed if 'enable_driver_sdk' enabled?

Quoted 36 lines · Ferruh Yigit

Quoted 22 lines · Tyler Retzlaff

Introduce a meson option enable_driver_sdk when true installs internal driver headers for ethdev. this allows drivers that do not depend on stable api/abi to be built external to the dpdk source tree.

Signed-off-by Tyler Retzlaff <[email protected]>

---

it's still unclear to me if we should be renaming the headers:

   ethdev_driver.h -> rte_ethdev_driver.h
   ethdev_pci.h -> rte_ethdev_pci.h
   ethdev_vdev.h -> rte_ethdev_vdev.h

  lib/librte_ethdev/meson.build | 5 +++++
  meson_options.txt             | 2 ++
  2 files changed, 7 insertions(+)

lib/librte_ethdev/meson.build +0 -0

index c37b2e377..7ecdec6f0 100644

lib/librte_ethdev/meson.build headers = files('rte_ethdev.h', · line 27

Quoted 8 lines

lib/librte_ethdev/meson.build +5 -0

@@ -20,6 +20,11 @@ headers = files('rte_ethdev.h',
      'rte_mtr_driver.h',
      'rte_tm.h',
      'rte_tm_driver.h')
+if get_option('enable_driver_sdk')
+headers += files('ethdev_driver.h',
+     'ethdev_pci.h',
+     'ethdev_vdev.h')
+endif

Instead of adding the "get_option('enable_driver_sdk')" checks to the modules, what about a more generic solution, like:

Quoted 4 lines

modules assign relevant headers into a new variable, let's say 'pmd_headers', and in a high level meson file, all 'pmd_headers' are installed if 'enable_driver_sdk' enabled?

+1.
Just, I don't like "pmd_headers" as the list name.
It can be misunderstood as the list of pmd-specific headers (thinking of rte_pmd_i40e.h), that are exposed to applications.

Quoted 42 lines · Ferruh Yigit

Quoted 40 lines · Tyler Retzlaff

Quoted 22 lines

Introduce a meson option enable_driver_sdk when true installs internal driver headers for ethdev. this allows drivers that do not depend on stable api/abi to be built external to the dpdk source tree.

Signed-off-by Tyler Retzlaff <[email protected]>

---

it's still unclear to me if we should be renaming the headers:

   ethdev_driver.h -> rte_ethdev_driver.h
   ethdev_pci.h -> rte_ethdev_pci.h
   ethdev_vdev.h -> rte_ethdev_vdev.h

  lib/librte_ethdev/meson.build | 5 +++++
  meson_options.txt             | 2 ++
  2 files changed, 7 insertions(+)

lib/librte_ethdev/meson.build +0 -0

index c37b2e377..7ecdec6f0 100644

lib/librte_ethdev/meson.build headers = files('rte_ethdev.h', · line 27

Quoted 8 lines

lib/librte_ethdev/meson.build +5 -0

@@ -20,6 +20,11 @@ headers = files('rte_ethdev.h',
      'rte_mtr_driver.h',
      'rte_tm.h',
      'rte_tm_driver.h')
+if get_option('enable_driver_sdk')
+headers += files('ethdev_driver.h',
+     'ethdev_pci.h',
+     'ethdev_vdev.h')
+endif

Instead of adding the "get_option('enable_driver_sdk')" checks to the modules, what about a more generic solution, like:

modules assign relevant headers into a new variable, let's say 'pmd_headers', and in a high level meson file, all 'pmd_headers' are installed if 'enable_driver_sdk' enabled?

Quoted 5 lines

+1.
Just, I don't like "pmd_headers" as the list name.
It can be misunderstood as the list of pmd-specific headers (thinking of rte_pmd_i40e.h), that are exposed to applications.

Since the option is called "enable_driver_sdk" the variable name of "driver_sdk_headers" would be a good match. We should try and keep variable names and option names in sync as much as possible.

12/03/2021 16:34, Bruce Richardson:

Quoted 14 lines

Quoted 14 lines

Quoted 8 lines

Instead of adding the "get_option('enable_driver_sdk')" checks to the modules, what about a more generic solution, like:

modules assign relevant headers into a new variable, let's say 'pmd_headers', and in a high level meson file, all 'pmd_headers' are installed if 'enable_driver_sdk' enabled?

+1.
Just, I don't like "pmd_headers" as the list name.
It can be misunderstood as the list of pmd-specific headers (thinking of rte_pmd_i40e.h), that are exposed to applications.

Quoted 3 lines

Since the option is called "enable_driver_sdk" the variable name of "driver_sdk_headers" would be a good match. We should try and keep variable names and option names in sync as much as possible.

+1

Introduce a meson option enable_driver_sdk when true installs internal driver headers for ethdev. this allows drivers that do not depend on stable api/abi to be built external to the dpdk source tree.

Signed-off-by Tyler Retzlaff <[email protected]>

---
 lib/librte_ethdev/meson.build | 6 ++++++
 lib/meson.build               | 4 ++++
 meson_options.txt             | 2 ++
 3 files changed, 12 insertions(+)

Introduce a meson option enable_driver_sdk when true installs internal driver headers for ethdev. this allows drivers that do not depend on stable api/abi to be built external to the dpdk source tree.

Signed-off-by Tyler Retzlaff <[email protected]>

---
  lib/librte_ethdev/meson.build | 6 ++++++
  lib/meson.build               | 4 ++++
  meson_options.txt             | 2 ++
  3 files changed, 12 insertions(+)

lib/librte_ethdev/meson.build +6 -0

index c37b2e377..4353fa6b7 100644
@@ -20,8 +20,14 @@ headers = files('rte_ethdev.h',
  	'rte_mtr_driver.h',
  	'rte_tm.h',
  	'rte_tm_driver.h')
+
  indirect_headers += files(
  	'rte_ethdev_core.h',
  	'rte_eth_ctrl.h')
 
+driver_sdk_headers += files(
+	'ethdev_driver.h',
+	'ethdev_pci.h',
+	'ethdev_vdev.h')
+
  deps += ['net', 'kvargs', 'meter', 'telemetry']

lib/meson.build +4 -0

index 7712aa497..992ebdf63 100644
@@ -67,6 +67,7 @@ foreach l:libraries
  	sources = []
  	headers = []
  	indirect_headers = [] # public headers not directly included by apps
+	driver_sdk_headers = [] # public headers included by drivers
  	includes = []
  	cflags = default_cflags
  	objs = [] # other object files to link against, used e.g. for

@@ -105,6 +106,9 @@ foreach l:libraries
  		dpdk_conf.set('RTE_LIB_' + name.to_upper(), 1)
  		install_headers(headers)
  		install_headers(indirect_headers)
+		if get_option('enable_driver_sdk')
+			install_headers(driver_sdk_headers)
+		endif
  		dpdk_chkinc_headers += headers
 
  		libname = 'rte_' + name

meson_options.txt +0 -0

index 6eff62e47..857874a19 100644

Quoted 10 lines

lib/librte_ethdev/meson.build +1 -0

index c37b2e377..4353fa6b7 100644
@@ -20,8 +20,14 @@ headers = files('rte_ethdev.h',
 	'rte_mtr_driver.h',
 	'rte_tm.h',
 	'rte_tm_driver.h')
+
 indirect_headers += files(

lib/librte_ethdev/meson.build headers = files('rte_ethdev.h', · line 33

Quoted 8 lines

lib/librte_ethdev/meson.build +5 -0

 	'rte_ethdev_core.h',
 	'rte_eth_ctrl.h')
+driver_sdk_headers += files(
+	'ethdev_driver.h',
+	'ethdev_pci.h',
+	'ethdev_vdev.h')
+
 deps += ['net', 'kvargs', 'meter', 'telemetry']

Quoted 10 lines · Ferruh Yigit

Quoted 10 lines

lib/librte_ethdev/meson.build +1 -0

index c37b2e377..4353fa6b7 100644
@@ -20,8 +20,14 @@ headers = files('rte_ethdev.h',
  	'rte_mtr_driver.h',
  	'rte_tm.h',
  	'rte_tm_driver.h')
+
  indirect_headers += files(

Quoted 9 lines

lib/librte_ethdev/meson.build headers = files('rte_ethdev.h', · line 33

Quoted 8 lines

lib/librte_ethdev/meson.build +5 -0

  	'rte_ethdev_core.h',
  	'rte_eth_ctrl.h')
+driver_sdk_headers += files(
+	'ethdev_driver.h',
+	'ethdev_pci.h',
+	'ethdev_vdev.h')
+
  deps += ['net', 'kvargs', 'meter', 'telemetry']

i feel like i missed a reply here. but just to clarify only ethdev will be covered by this patch. inclusion of other driver headers was discussed off list (sorry) and it emerged that it would result in withdraw a number of driver api/abi that had not been marked as __rte_internal.

for driver api that were being exported as 'stable' a deprecation notice will need to be issued in order to make them part of the driver_sdk_headers. for that reason only ethdev is being made available under this option for now.

please ack/nack the patch as-is

Quoted 27 lines · Ferruh Yigit

Quoted 10 lines

Quoted 10 lines

lib/librte_ethdev/meson.build +1 -0

index c37b2e377..4353fa6b7 100644
@@ -20,8 +20,14 @@ headers = files('rte_ethdev.h',
  	'rte_mtr_driver.h',
  	'rte_tm.h',
  	'rte_tm_driver.h')
+
  indirect_headers += files(

Quoted 9 lines

lib/librte_ethdev/meson.build headers = files('rte_ethdev.h', · line 33

Quoted 8 lines

lib/librte_ethdev/meson.build +5 -0

  	'rte_ethdev_core.h',
  	'rte_eth_ctrl.h')
+driver_sdk_headers += files(
+	'ethdev_driver.h',
+	'ethdev_pci.h',
+	'ethdev_vdev.h')
+
  deps += ['net', 'kvargs', 'meter', 'telemetry']

i feel like i missed a reply here. but just to clarify only ethdev will be covered by this patch. inclusion of other driver headers was discussed off list (sorry) and it emerged that it would result in withdraw a number of driver api/abi that had not been marked as __rte_internal.

Quoted 7 lines

for driver api that were being exported as 'stable' a deprecation notice will need to be issued in order to make them part of the driver_sdk_headers. for that reason only ethdev is being made available under this option for now.

please ack/nack the patch as-is

I am OK the patch for the ethdev part, hence

Acked-by Ferruh Yigit <[email protected]>

But not sure how to manage the same problem for whole project, if install all headers in one patch, or add them gradually via separate patches by time ...

24/03/2021 12:27, Ferruh Yigit:

Quoted 3 lines

But not sure how to manage the same problem for whole project, if install all headers in one patch, or add them gradually via separate patches by time ...

We did a cleanup in ethdev but not in other driver classes. When the cleanup will be done gradually, the headers must move in this new category driver_sdk_headers.

Quoted 8 lines · Thomas Monjalon

24/03/2021 12:27, Ferruh Yigit:

Quoted 3 lines

But not sure how to manage the same problem for whole project, if install all headers in one patch, or add them gradually via separate patches by time ...

We did a cleanup in ethdev but not in other driver classes. When the cleanup will be done gradually, the headers must move in this new category driver_sdk_headers.

yes, some headers are not installed now. so they need only to have their api marked __rte_internal and installed (since there should be no external consumer as a function of not being installed)

the more difficult case is where headers were installed but the api were not marked __rte_internal and appear in the stable version.map. for those i guess deprecation notice has to be issued before marking as internal.

Quoted 14 lines · Tyler Retzlaff

Quoted 8 lines · Thomas Monjalon

24/03/2021 12:27, Ferruh Yigit:

Quoted 3 lines

But not sure how to manage the same problem for whole project, if install all headers in one patch, or add them gradually via separate patches by time ...

We did a cleanup in ethdev but not in other driver classes. When the cleanup will be done gradually, the headers must move in this new category driver_sdk_headers.

yes, some headers are not installed now. so they need only to have their api marked __rte_internal and installed (since there should be no external consumer as a function of not being installed)

Quoted 5 lines

the more difficult case is where headers were installed but the api were not marked __rte_internal and appear in the stable version.map. for those i guess deprecation notice has to be issued before marking as internal.

Are you referring to any specific APIs, can you share list of them?

Quoted 15 lines · Ferruh Yigit

Quoted 14 lines · Tyler Retzlaff

Quoted 8 lines · Thomas Monjalon

24/03/2021 12:27, Ferruh Yigit:

Quoted 3 lines

But not sure how to manage the same problem for whole project, if install all headers in one patch, or add them gradually via separate patches by time ...

We did a cleanup in ethdev but not in other driver classes. When the cleanup will be done gradually, the headers must move in this new category driver_sdk_headers.

yes, some headers are not installed now. so they need only to have their api marked __rte_internal and installed (since there should be no external consumer as a function of not being installed)

Quoted 7 lines

Quoted 5 lines

the more difficult case is where headers were installed but the api were not marked __rte_internal and appear in the stable version.map. for those i guess deprecation notice has to be issued before marking as internal.

Are you referring to any specific APIs, can you share list of them?

i can't remember the whole list but Thomas originally indicated the following candidate list.

     baseband/ -> librte_bbdev/rte_bbdev_pmd.h
     bus/ -> rte_bus.h
     common/ -> no interface
     crypto/ -> librte_cryptodev/rte_cryptodev_pmd.h
     event/ -> librte_eventdev/
     mempool/ -> librte_mempool/
     net/ -> librte_ethdev/
     raw/ -> librte_rawdev/rte_rawdev_pmd.h
     regex/ -> librte_regexdev/rte_regexdev_driver.h
     vdpa/ -> librte_vhost/rte_vdpa_dev.h

some of these headers are not published, some are.

Quoted 38 lines · Ferruh Yigit

Quoted 15 lines · Tyler Retzlaff

Quoted 14 lines · Thomas Monjalon

Quoted 8 lines

24/03/2021 12:27, Ferruh Yigit:

Quoted 3 lines

But not sure how to manage the same problem for whole project, if install all headers in one patch, or add them gradually via separate patches by time ...

We did a cleanup in ethdev but not in other driver classes. When the cleanup will be done gradually, the headers must move in this new category driver_sdk_headers.

yes, some headers are not installed now. so they need only to have their api marked __rte_internal and installed (since there should be no external consumer as a function of not being installed)

Quoted 7 lines

Quoted 5 lines

the more difficult case is where headers were installed but the api were not marked __rte_internal and appear in the stable version.map. for those i guess deprecation notice has to be issued before marking as internal.

Are you referring to any specific APIs, can you share list of them?

i can't remember the whole list but Thomas originally indicated the following candidate list.

     baseband/ -> librte_bbdev/rte_bbdev_pmd.h
     bus/ -> rte_bus.h
     common/ -> no interface
     crypto/ -> librte_cryptodev/rte_cryptodev_pmd.h
     event/ -> librte_eventdev/
     mempool/ -> librte_mempool/
     net/ -> librte_ethdev/
     raw/ -> librte_rawdev/rte_rawdev_pmd.h
     regex/ -> librte_regexdev/rte_regexdev_driver.h
     vdpa/ -> librte_vhost/rte_vdpa_dev.h

Quoted 2 lines

some of these headers are not published, some are.

These are public headers, so they should not have '__rte_internal' functions, are you saying some of public headers has internal functions that are presented as public APIs?

29/03/2021 11:43, Ferruh Yigit:

Quoted 42 lines · Tyler Retzlaff

Quoted 40 lines · Ferruh Yigit

Quoted 15 lines · Tyler Retzlaff

Quoted 14 lines · Thomas Monjalon

Quoted 8 lines

24/03/2021 12:27, Ferruh Yigit:

Quoted 3 lines

But not sure how to manage the same problem for whole project, if install all headers in one patch, or add them gradually via separate patches by time ...

We did a cleanup in ethdev but not in other driver classes. When the cleanup will be done gradually, the headers must move in this new category driver_sdk_headers.

yes, some headers are not installed now. so they need only to have their api marked __rte_internal and installed (since there should be no external consumer as a function of not being installed)

Quoted 7 lines

Quoted 5 lines

the more difficult case is where headers were installed but the api were not marked __rte_internal and appear in the stable version.map. for those i guess deprecation notice has to be issued before marking as internal.

Are you referring to any specific APIs, can you share list of them?

i can't remember the whole list but Thomas originally indicated the following candidate list.

      baseband/ -> librte_bbdev/rte_bbdev_pmd.h
      bus/ -> rte_bus.h
      common/ -> no interface
      crypto/ -> librte_cryptodev/rte_cryptodev_pmd.h
      event/ -> librte_eventdev/
      mempool/ -> librte_mempool/
      net/ -> librte_ethdev/
      raw/ -> librte_rawdev/rte_rawdev_pmd.h
      regex/ -> librte_regexdev/rte_regexdev_driver.h
      vdpa/ -> librte_vhost/rte_vdpa_dev.h

some of these headers are not published, some are.

Quoted 3 lines

These are public headers, so they should not have '__rte_internal' functions, are you saying some of public headers has internal functions that are presented as public APIs?

These are the headers for use by the drivers.
We should classify them as SDK headers, not API.

Quoted 43 lines

29/03/2021 11:43, Ferruh Yigit:

Quoted 42 lines · Tyler Retzlaff

Quoted 40 lines · Ferruh Yigit

Quoted 15 lines · Tyler Retzlaff

Quoted 14 lines · Thomas Monjalon

Quoted 8 lines

24/03/2021 12:27, Ferruh Yigit:

Quoted 3 lines

But not sure how to manage the same problem for whole project, if install all headers in one patch, or add them gradually via separate patches by time ...

We did a cleanup in ethdev but not in other driver classes. When the cleanup will be done gradually, the headers must move in this new category driver_sdk_headers.

yes, some headers are not installed now. so they need only to have their api marked __rte_internal and installed (since there should be no external consumer as a function of not being installed)

Quoted 7 lines

Quoted 5 lines

the more difficult case is where headers were installed but the api were not marked __rte_internal and appear in the stable version.map. for those i guess deprecation notice has to be issued before marking as internal.

Are you referring to any specific APIs, can you share list of them?

i can't remember the whole list but Thomas originally indicated the following candidate list.

      baseband/ -> librte_bbdev/rte_bbdev_pmd.h
      bus/ -> rte_bus.h
      common/ -> no interface
      crypto/ -> librte_cryptodev/rte_cryptodev_pmd.h
      event/ -> librte_eventdev/
      mempool/ -> librte_mempool/
      net/ -> librte_ethdev/
      raw/ -> librte_rawdev/rte_rawdev_pmd.h
      regex/ -> librte_regexdev/rte_regexdev_driver.h
      vdpa/ -> librte_vhost/rte_vdpa_dev.h

some of these headers are not published, some are.

Quoted 7 lines

Quoted 3 lines

These are public headers, so they should not have '__rte_internal' functions, are you saying some of public headers has internal functions that are presented as public APIs?

These are the headers for use by the drivers.
We should classify them as SDK headers, not API.

Yes, you are right, they shouldn't be public header.

So, agree to Tyler's comment, that some of those functions needs to be removed from the stable API list first, which will take time.

I can proceed with the ethdev one, any objection?

And for the rest of the list, how can we fix them? I guess best option is to distribute the work to each library, but we need an owner of the task to follow and communicate it.

Quoted 32 lines · Tyler Retzlaff

Quoted 10 lines · Ferruh Yigit

Quoted 10 lines

lib/librte_ethdev/meson.build +1 -0

index c37b2e377..4353fa6b7 100644
@@ -20,8 +20,14 @@ headers = files('rte_ethdev.h',
       'rte_mtr_driver.h',
       'rte_tm.h',
       'rte_tm_driver.h')
+

  indirect_headers += files(

Quoted 9 lines

Quoted 8 lines

      'rte_ethdev_core.h',
      'rte_eth_ctrl.h')

lib/librte_ethdev/meson.build +5 -0

+driver_sdk_headers += files(
+    'ethdev_driver.h',
+    'ethdev_pci.h',
+    'ethdev_vdev.h')
+

  deps += ['net', 'kvargs', 'meter', 'telemetry']

i feel like i missed a reply here.  but just to clarify only ethdev will be covered by this patch. inclusion of other driver headers was discussed off list (sorry) and it emerged that it would result in withdraw a number of driver api/abi that had not been marked as __rte_internal.

for driver api that were being exported as 'stable' a deprecation notice will need to be issued in order to make them part of the driver_sdk_headers. for that reason only ethdev is being made available under this option for now.

Quoted 2 lines

please ack/nack the patch as-is

I am OK the patch for the ethdev part, hence

Acked-by Ferruh Yigit <[email protected]>