Re: [PATCH v19 1/6] memarea: introduce memarea library

Re: [PATCH v19 1/6] memarea: introduce memarea library

[Stephen Hemminger]

On Thu, 20 Jul 2023 09:22:49 +0000
Chengwen Feng <fengchengwen@huawei.com> wrote:

> The memarea library is an allocator of variable-size object which based
> on a memory region.
> 
> This patch provides rte_memarea_create() and rte_memarea_destroy() API.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  MAINTAINERS                            |   5 +
>  doc/api/doxy-api-index.md              |   3 +-
>  doc/api/doxy-api.conf.in               |   1 +
>  doc/guides/prog_guide/index.rst        |   1 +
>  doc/guides/prog_guide/memarea_lib.rst  |  48 ++++++
>  doc/guides/rel_notes/release_23_07.rst |   6 +
>  lib/memarea/memarea_private.h          | 116 ++++++++++++++
>  lib/memarea/meson.build                |  18 +++
>  lib/memarea/rte_memarea.c              | 204 +++++++++++++++++++++++++
>  lib/memarea/rte_memarea.h              | 141 +++++++++++++++++
>  lib/memarea/version.map                |  12 ++
>  lib/meson.build                        |   1 +
>  12 files changed, 555 insertions(+), 1 deletion(-)
>  create mode 100644 doc/guides/prog_guide/memarea_lib.rst
>  create mode 100644 lib/memarea/memarea_private.h
>  create mode 100644 lib/memarea/meson.build
>  create mode 100644 lib/memarea/rte_memarea.c
>  create mode 100644 lib/memarea/rte_memarea.h
>  create mode 100644 lib/memarea/version.map
> 

The library is still interesting but patch needs some changes and rebasing.

> diff --git a/doc/guides/rel_notes/release_23_07.rst b/doc/guides/rel_notes/release_23_07.rst
> index 6a1c45162b..2751d70740 100644
> --- a/doc/guides/rel_notes/release_23_07.rst
> +++ b/doc/guides/rel_notes/release_23_07.rst
> @@ -222,6 +222,12 @@ New Features
>  
>    See the :doc:`../tools/dmaperf` for more details.
>  
> +* **Added memarea library.**
> +
> +  The memarea library is an allocator of variable-size objects, it is oriented
> +  towards the application layer, providing 'region-based memory management'
> +  function.
> +
>

This would need to be for 25_03 now


> diff --git a/lib/memarea/meson.build b/lib/memarea/meson.build
> new file mode 100644
> index 0000000000..7e18c02d3e
> --- /dev/null
> +++ b/lib/memarea/meson.build
> @@ -0,0 +1,18 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023 HiSilicon Limited
> +
> +if is_windows
> +    build = false
> +    reason = 'not supported on Windows'
> +    subdir_done()
> +endif

Is there a strong reason this can't be used on Windows?

...

> +void
> +rte_memarea_destroy(struct rte_memarea *ma)
> +{
> +	if (ma == NULL) {
> +		rte_errno = EINVAL;

No, this is normal, do not set EINVAL here.

> +		return;
> +	}
> +	memarea_free_area(&ma->init, ma->area_base);
> +	rte_free(ma);
> +}

Also add rte_memarea_destroy to the null_free coccinelle script
so that any cleanup pass later would fix unnecessary null checks
around this function.

> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Create memarea.
> + *
> + * Create one new memarea.
> + *
> + * @param init
> + *   The init parameter of memarea.
> + *
> + * @return
> + *   Non-NULL on success. Otherwise NULL is returned (the rte_errno is set).
> + */
> +__rte_experimental
> +struct rte_memarea *rte_memarea_create(const struct rte_memarea_param *init);

Now that DPDK has function attributes on allocators,
this function should be:
	__rte_malloc
and
	__rte_dealloc(rte_memarea_destroy, 1)


> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Destroy memarea.
> + *
> + * Destroy the memarea.
> + *
> + * @param ma
> + *   The pointer of memarea.
> + *
> + * @note The rte_errno is set if destroy failed.
> + */
> +__rte_experimental
> +void rte_memarea_destroy(struct rte_memarea *ma);
> +

When using __rte_dealloc, would need this prototype to be
before the create function

> +#ifdef __cplusplus
> +}
> +#endif