Re: [dpdk-dev] [PATCH] gpudev: introduce memory API

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 6/2/21 11:35 PM, Thomas Monjalon wrote:
> From: Elena Agostini <eagostini@nvidia.com>
> 
> The new library gpudev is for dealing with GPU from a DPDK application
> in a vendor-agnostic way.
> 
> As a first step, the features are focused on memory management.
> A function allows to allocate memory inside the GPU,
> while another one allows to use main (CPU) memory from the GPU.
> 
> The infrastructure is prepared to welcome drivers in drivers/gpu/
> as the upcoming NVIDIA one, implementing the gpudev API.
> Other additions planned for next revisions:
>   - C implementation file
>   - guide documentation
>   - unit tests
>   - integration in testpmd to enable Rx/Tx to/from GPU memory.
> 
> The next step should focus on GPU processing task control.
> 
> Signed-off-by: Elena Agostini <eagostini@nvidia.com>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>


LGTM as an RFC. It is definitely to a patch to apply
since implementation is missing. See my notes below.

> ---
>  .gitignore                           |   1 +
>  MAINTAINERS                          |   6 +
>  doc/api/doxy-api-index.md            |   1 +
>  doc/api/doxy-api.conf.in             |   1 +
>  doc/guides/conf.py                   |   8 ++
>  doc/guides/gpus/features/default.ini |  13 ++
>  doc/guides/gpus/index.rst            |  11 ++
>  doc/guides/gpus/overview.rst         |   7 +
>  doc/guides/index.rst                 |   1 +
>  doc/guides/prog_guide/gpu.rst        |   5 +
>  doc/guides/prog_guide/index.rst      |   1 +
>  drivers/gpu/meson.build              |   4 +
>  drivers/meson.build                  |   1 +
>  lib/gpudev/gpu_driver.h              |  44 +++++++
>  lib/gpudev/meson.build               |   9 ++
>  lib/gpudev/rte_gpudev.h              | 183 +++++++++++++++++++++++++++
>  lib/gpudev/version.map               |  11 ++
>  lib/meson.build                      |   1 +
>  18 files changed, 308 insertions(+)
>  create mode 100644 doc/guides/gpus/features/default.ini
>  create mode 100644 doc/guides/gpus/index.rst
>  create mode 100644 doc/guides/gpus/overview.rst
>  create mode 100644 doc/guides/prog_guide/gpu.rst
>  create mode 100644 drivers/gpu/meson.build
>  create mode 100644 lib/gpudev/gpu_driver.h
>  create mode 100644 lib/gpudev/meson.build
>  create mode 100644 lib/gpudev/rte_gpudev.h
>  create mode 100644 lib/gpudev/version.map
> 
> diff --git a/.gitignore b/.gitignore
> index b19c0717e6..49494e0c6c 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -14,6 +14,7 @@ doc/guides/compressdevs/overview_feature_table.txt
>  doc/guides/regexdevs/overview_feature_table.txt
>  doc/guides/vdpadevs/overview_feature_table.txt
>  doc/guides/bbdevs/overview_feature_table.txt
> +doc/guides/gpus/overview_feature_table.txt
>  
>  # ignore generated ctags/cscope files
>  cscope.out.po
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5877a16971..c4755dfe9a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -452,6 +452,12 @@ F: app/test-regex/
>  F: doc/guides/prog_guide/regexdev.rst
>  F: doc/guides/regexdevs/features/default.ini
>  
> +GPU API - EXPERIMENTAL
> +M: Elena Agostini <eagostini@nvidia.com>
> +F: lib/gpudev/
> +F: doc/guides/prog_guide/gpu.rst
> +F: doc/guides/gpus/features/default.ini
> +
>  Eventdev API
>  M: Jerin Jacob <jerinj@marvell.com>
>  T: git://dpdk.org/next/dpdk-next-eventdev
> diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> index 1992107a03..bd10342ca2 100644
> --- a/doc/api/doxy-api-index.md
> +++ b/doc/api/doxy-api-index.md
> @@ -21,6 +21,7 @@ The public API headers are grouped by topics:
>    [compressdev]        (@ref rte_compressdev.h),
>    [compress]           (@ref rte_comp.h),
>    [regexdev]           (@ref rte_regexdev.h),
> +  [gpudev]             (@ref rte_gpudev.h),
>    [eventdev]           (@ref rte_eventdev.h),
>    [event_eth_rx_adapter]   (@ref rte_event_eth_rx_adapter.h),
>    [event_eth_tx_adapter]   (@ref rte_event_eth_tx_adapter.h),
> diff --git a/doc/api/doxy-api.conf.in b/doc/api/doxy-api.conf.in
> index 325a0195c6..831b9a6b33 100644
> --- a/doc/api/doxy-api.conf.in
> +++ b/doc/api/doxy-api.conf.in
> @@ -40,6 +40,7 @@ INPUT                   = @TOPDIR@/doc/api/doxy-api-index.md \
>                            @TOPDIR@/lib/eventdev \
>                            @TOPDIR@/lib/fib \
>                            @TOPDIR@/lib/flow_classify \
> +                          @TOPDIR@/lib/gpudev \
>                            @TOPDIR@/lib/graph \
>                            @TOPDIR@/lib/gro \
>                            @TOPDIR@/lib/gso \
> diff --git a/doc/guides/conf.py b/doc/guides/conf.py
> index 67d2dd62c7..7930da9ceb 100644
> --- a/doc/guides/conf.py
> +++ b/doc/guides/conf.py
> @@ -152,6 +152,9 @@ def generate_overview_table(output_filename, table_id, section, table_name, titl
>          name = ini_filename[:-4]
>          name = name.replace('_vf', 'vf')
>          pmd_names.append(name)
> +    if not pmd_names:
> +        # Add an empty column if table is empty (required by RST syntax)
> +        pmd_names.append(' ')
>  
>      # Pad the table header names.
>      max_header_len = len(max(pmd_names, key=len))
> @@ -388,6 +391,11 @@ def setup(app):
>                              'Features',
>                              'Features availability in bbdev drivers',
>                              'Feature')
> +    table_file = dirname(__file__) + '/gpus/overview_feature_table.txt'
> +    generate_overview_table(table_file, 1,
> +                            'Features',
> +                            'Features availability in GPU drivers',
> +                            'Feature')
>  
>      if LooseVersion(sphinx_version) < LooseVersion('1.3.1'):
>          print('Upgrade sphinx to version >= 1.3.1 for '
> diff --git a/doc/guides/gpus/features/default.ini b/doc/guides/gpus/features/default.ini
> new file mode 100644
> index 0000000000..c363447b0d
> --- /dev/null
> +++ b/doc/guides/gpus/features/default.ini
> @@ -0,0 +1,13 @@
> +;
> +; Features of a GPU driver.
> +;
> +; This file defines the features that are valid for inclusion in
> +; the other driver files and also the order that they appear in
> +; the features table in the documentation. The feature description
> +; string should not exceed feature_str_len defined in conf.py.
> +;
> +[Features]
> +Get device info                =
> +Share CPU memory with GPU      =
> +Allocate GPU memory            =
> +Free memory                    =
> diff --git a/doc/guides/gpus/index.rst b/doc/guides/gpus/index.rst
> new file mode 100644
> index 0000000000..f9c62aeb36
> --- /dev/null
> +++ b/doc/guides/gpus/index.rst
> @@ -0,0 +1,11 @@
> +.. SPDX-License-Identifier: BSD-3-Clause
> +   Copyright 2021 NVIDIA Corporation & Affiliates
> +
> +GPU Drivers
> +===========
> +
> +.. toctree::
> +   :maxdepth: 2
> +   :numbered:
> +
> +   overview
> diff --git a/doc/guides/gpus/overview.rst b/doc/guides/gpus/overview.rst
> new file mode 100644
> index 0000000000..e7f985e98b
> --- /dev/null
> +++ b/doc/guides/gpus/overview.rst
> @@ -0,0 +1,7 @@
> +.. SPDX-License-Identifier: BSD-3-Clause
> +   Copyright 2021 NVIDIA Corporation & Affiliates
> +
> +Overview of GPU Drivers
> +=======================
> +
> +.. include:: overview_feature_table.txt
> diff --git a/doc/guides/index.rst b/doc/guides/index.rst
> index 857f0363d3..ee4d79a4eb 100644
> --- a/doc/guides/index.rst
> +++ b/doc/guides/index.rst
> @@ -21,6 +21,7 @@ DPDK documentation
>     compressdevs/index
>     vdpadevs/index
>     regexdevs/index
> +   gpus/index
>     eventdevs/index
>     rawdevs/index
>     mempool/index
> diff --git a/doc/guides/prog_guide/gpu.rst b/doc/guides/prog_guide/gpu.rst
> new file mode 100644
> index 0000000000..54f9fa8300
> --- /dev/null
> +++ b/doc/guides/prog_guide/gpu.rst
> @@ -0,0 +1,5 @@
> +.. SPDX-License-Identifier: BSD-3-Clause
> +   Copyright 2021 NVIDIA Corporation & Affiliates
> +
> +GPU Library
> +===========
> diff --git a/doc/guides/prog_guide/index.rst b/doc/guides/prog_guide/index.rst
> index 2dce507f46..dfddf90b51 100644
> --- a/doc/guides/prog_guide/index.rst
> +++ b/doc/guides/prog_guide/index.rst
> @@ -27,6 +27,7 @@ Programmer's Guide
>      cryptodev_lib
>      compressdev
>      regexdev
> +    gpu
>      rte_security
>      rawdev
>      link_bonding_poll_mode_drv_lib
> diff --git a/drivers/gpu/meson.build b/drivers/gpu/meson.build
> new file mode 100644
> index 0000000000..5189950616
> --- /dev/null
> +++ b/drivers/gpu/meson.build
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright 2021 NVIDIA Corporation & Affiliates
> +
> +drivers = []
> diff --git a/drivers/meson.build b/drivers/meson.build
> index bc6f4f567f..f607040d79 100644
> --- a/drivers/meson.build
> +++ b/drivers/meson.build
> @@ -18,6 +18,7 @@ subdirs = [
>          'vdpa',           # depends on common, bus and mempool.
>          'event',          # depends on common, bus, mempool and net.
>          'baseband',       # depends on common and bus.
> +        'gpu',            # depends on common and bus.
>  ]
>  
>  if meson.is_cross_build()
> diff --git a/lib/gpudev/gpu_driver.h b/lib/gpudev/gpu_driver.h
> new file mode 100644
> index 0000000000..5ff609e49d
> --- /dev/null
> +++ b/lib/gpudev/gpu_driver.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 NVIDIA Corporation & Affiliates
> + */
> +
> +#ifndef GPU_DRIVER_H
> +#define GPU_DRIVER_H
> +
> +#include <stdint.h>
> +
> +#include <rte_common.h>
> +
> +#include "rte_gpudev.h"
> +
> +struct rte_gpu_dev;
> +
> +typedef int (*gpu_malloc_t)(struct rte_gpu_dev *dev, size_t size, void **ptr);
> +typedef int (*gpu_free_t)(struct rte_gpu_dev *dev, void *ptr);

Not that important but I always prefer to typedef
function prototypes w/o pointer and use pointer in
the structure below. I.e.

typedef int (gpu_malloc_t)(struct rte_gpu_dev *dev, size_t size, void
**ptr);

It allows to specify that corresponding callback
must comply to the prototype and produce build
error otherwise (and do not rely on warnings), e.g.

static gpu_malloc_t mlx5_gpu_malloc;
static int
mlx5_gpu_malloc(struct rte_gpu_dev *dev, size_t size, void **ptr)
{
     ...
}

May be a new library should go this way.

> +
> +struct rte_gpu_dev {
> +	/* Backing device. */
> +	struct rte_device *device;
> +	/* GPU info structure. */
> +	struct rte_gpu_info info;
> +	/* Counter of processes using the device. */
> +	uint16_t process_cnt;
> +	/* If device is currently used or not. */
> +	enum rte_gpu_state state;
> +	/* FUNCTION: Allocate memory on the GPU. */
> +	gpu_malloc_t gpu_malloc;
> +	/* FUNCTION: Allocate memory on the CPU visible from the GPU. */
> +	gpu_malloc_t gpu_malloc_visible;
> +	/* FUNCTION: Free allocated memory on the GPU. */
> +	gpu_free_t gpu_free;

Don't we need a callback to get dev_info?

> +	/* Device interrupt handle. */
> +	struct rte_intr_handle *intr_handle;
> +	/* Driver-specific private data. */
> +	void *dev_private;
> +} __rte_cache_aligned;
> +
> +struct rte_gpu_dev *rte_gpu_dev_allocate(const char *name);
> +struct rte_gpu_dev *rte_gpu_dev_get_by_name(const char *name);
> +int rte_gpu_dev_release(struct rte_gpu_dev *gpudev);
> +
> +#endif /* GPU_DRIVER_H */
> diff --git a/lib/gpudev/meson.build b/lib/gpudev/meson.build
> new file mode 100644
> index 0000000000..f05459e18d
> --- /dev/null
> +++ b/lib/gpudev/meson.build
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright 2021 NVIDIA Corporation & Affiliates
> +
> +headers = files(
> +        'rte_gpudev.h',
> +)
> +
> +sources = files(
> +)
> diff --git a/lib/gpudev/rte_gpudev.h b/lib/gpudev/rte_gpudev.h
> new file mode 100644
> index 0000000000..b12f35c17e
> --- /dev/null
> +++ b/lib/gpudev/rte_gpudev.h
> @@ -0,0 +1,183 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 NVIDIA Corporation & Affiliates
> + */
> +
> +#ifndef RTE_GPUDEV_H
> +#define RTE_GPUDEV_H
> +
> +#include <stdint.h>
> +#include <stdbool.h>
> +
> +#include <rte_common.h>
> +
> +/**
> + * @file
> + * Generic library to interact with a GPU.
> + *
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/** Maximum number of GPU engines. */
> +#define RTE_GPU_MAX_DEVS UINT16_C(32)
> +/** Maximum length of device name. */
> +#define RTE_GPU_NAME_MAX_LEN 128
> +
> +/** Flags indicate current state of GPU device. */
> +enum rte_gpu_state {
> +	RTE_GPU_STATE_UNUSED,        /**< not initialized */
> +	RTE_GPU_STATE_INITIALIZED,   /**< initialized */
> +};
> +
> +/** Store a list of info for a given GPU. */
> +struct rte_gpu_info {
> +	/** GPU device ID. */
> +	uint16_t gpu_id;
> +	/** Unique identifier name. */
> +	char name[RTE_GPU_NAME_MAX_LEN];
> +	/** Total memory available on device. */
> +	size_t total_memory;
> +	/** Total processors available on device. */
> +	int processor_count;
> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Returns the number of GPUs detected and associated to DPDK.
> + *
> + * @return
> + *   The number of available GPUs.
> + */
> +__rte_experimental
> +uint16_t rte_gpu_dev_count_avail(void);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Check if the device is valid and initialized in DPDK.
> + *
> + * @param gpu_id
> + *   The input GPU ID.
> + *
> + * @return
> + *   - True if gpu_id is a valid and initialized GPU.
> + *   - False otherwise.
> + */
> +__rte_experimental
> +bool rte_gpu_dev_is_valid(uint16_t gpu_id);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Get the GPU ID of the next valid GPU initialized in DPDK.
> + *
> + * @param gpu_id
> + *   The initial GPU ID to start the research.
> + *
> + * @return
> + *   Next GPU ID corresponding to a valid and initialized GPU device.
> + */
> +__rte_experimental
> +uint16_t rte_gpu_dev_find_next(uint16_t gpu_id);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Macro to iterate over all valid GPUs.
> + *
> + * @param gpu_id
> + *   The ID of the next possible valid GPU.
> + * @return
> + *   Next valid GPU ID, RTE_GPU_MAX_DEVS if there is none.
> + */
> +#define RTE_GPU_FOREACH_DEV(gpu_id) \
> +	for (gpu_id = rte_gpu_find_next(0); \
> +	     gpu_id < RTE_GPU_MAX_DEVS; \
> +	     gpu_id = rte_gpu_find_next(gpu_id + 1))
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Return GPU specific info.
> + *
> + * @param gpu_id
> + *   GPU ID to get info.
> + * @param info
> + *   Memory structure to fill with the info.
> + *
> + * @return
> + *   0 on success, -1 otherwise.
> + */
> +__rte_experimental
> +int rte_gpu_dev_info_get(uint16_t gpu_id, struct rte_gpu_info **info);

Hm, I think it is better to have 'struct rte_gpu_info *info'.
Why should it allocate and return memory to be freed by caller?

> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Allocate a chunk of memory on the GPU.

Looking a below function it is required to clarify here if
the memory is visible or invisible to GPU (or both allowed).

> + *
> + * @param gpu_id
> + *   GPU ID to allocate memory.
> + * @param size
> + *   Number of bytes to allocate.

Is behaviour defined if zero size is requested?
IMHO, it would be good to define.

> + * @param ptr
> + *   Pointer to store the address of the allocated memory.
> + *
> + * @return
> + *   0 on success, -1 otherwise.

Don't we want to differentiate various errors using
negative errno as it is done in many DPDK libraries?

> + */
> +__rte_experimental
> +int rte_gpu_malloc(uint16_t gpu_id, size_t size, void **ptr);

May be *malloc() should return a pointer and "negative"
values used to report various errnos?

The problem with the approach that comparison vs NULL will
not work in this case and we need special macro or small
inline function to check error condition.

Returned pointer is definitely more convenient, but above
not may result in bugs.

> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Allocate a chunk of memory on the CPU that is visible from the GPU.
> + *
> + * @param gpu_id
> + *   Reference GPU ID.
> + * @param size
> + *   Number of bytes to allocate.
> + * @param ptr
> + *   Pointer to store the address of the allocated memory.
> + *
> + * @return
> + *   0 on success, -1 otherwise.

Same here

> + */
> +__rte_experimental
> +int rte_gpu_malloc_visible(uint16_t gpu_id, size_t size, void **ptr);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Deallocate a chunk of memory allocated with rte_gpu_malloc*.
> + *
> + * @param gpu_id
> + *   Reference GPU ID.
> + * @param ptr
> + *   Pointer to the memory area to be deallocated.

I think it should be NOP in the case of NULL pointer and it
should be documented. If not, it must be documented as well.

> + *
> + * @return
> + *   0 on success, -1 otherwise.

Same here

> + */
> +__rte_experimental
> +int rte_gpu_free(uint16_t gpu_id, void *ptr);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* RTE_GPUDEV_H */
> diff --git a/lib/gpudev/version.map b/lib/gpudev/version.map
> new file mode 100644
> index 0000000000..9e0f218e8b
> --- /dev/null
> +++ b/lib/gpudev/version.map
> @@ -0,0 +1,11 @@
> +EXPERIMENTAL {
> +	global:
> +
> +	rte_gpu_dev_count_avail;
> +	rte_gpu_dev_find_next;
> +	rte_gpu_dev_info_get;
> +	rte_gpu_dev_is_valid;
> +	rte_gpu_free;
> +	rte_gpu_malloc;
> +	rte_gpu_malloc_visible;
> +};
> diff --git a/lib/meson.build b/lib/meson.build
> index 4a64756a68..ffefc64c69 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -33,6 +33,7 @@ libraries = [
>          'distributor',
>          'efd',
>          'eventdev',
> +        'gpudev',
>          'gro',
>          'gso',
>          'ip_frag',
>