Re: [RFC] memarea: introduce memory area library

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4369 bytes --]

>
> Note:
> a) the memarea is oriented towards the application layer, which could
> provides 'region-based memory management' [1] function.
>

Judging from the API, this library would rather provide
an interface to a generic allocator over a fixed memory extent,
because it offers freeing of specific elements, and thus must track them.
So it's more than RBMM. Is this intended?
It's a very interesting RFC anyway, just trying to understand the scope.

b) the eal library also provide memory zone/heap management, but these
> are tied to huge pages management.
>
[...]
> + * The memarea is a collection of allocated objects that can be
> efficiently
> + * alloc or free all at once, the main feature are as follows:
> + *   a) it facilitate alloc and free of memory with low overhead.
> + *   [...]
>
+ *   c) it supports MT-safe as long as it's specified at creation time.
>

These two bullets seem to add the most value compared to DPDK heap API.
DPDK heap overhead is at least 64 bytes per allocation (sizeof malloc_elem),
so I assume memarea aims at a large number of small elements.


> +struct rte_memarea_param {
> +       char name[RTE_MEMAREA_NAMESIZE]; /**< Name of memarea */
> +       enum rte_memarea_source source;  /**< Memory source of memarea */
> +       uint64_t size;                   /**< Size (byte) of memarea */
>

Is it an upper limit or a guaranteed size?
It probably depends on the source: guaranteed for USER_ADDR,
upper limit for SYSAPI (or it would be no different from USER_ADDR),
not sure about USER_MEMAREA.

Do you envision memarea as always limited?
Generic allocators usually have means of adding extents,
even if this one doesn't currently.

Nit: size is uint64_t here but uint32_t in rte_memarea_allloc().
Should be size_t in both places.


> +       uint32_t align;                  /**< Align of allocated object */
>
+       /** Indicates whether the memarea should be MT-safe */
> +       bool mt_safe;
> +       /** Indicates whether the memarea is visible to multiple process.
> +        * If the memory source is RTE_MEMAREA_SOURCE_USER_ADDR, this filed
> +        * depends on user settings and must be set.
> +        * If the memory source is RTE_MEMAREA_SOURCE_SYSAPI or
> +        * RTE_MEMAREA_SOURCE_USER_MEMAREA, this filed does not need to be
> set.
> +        */
> +       bool mp_visible;
> +       /** User provided address, this field is valid only when source
> +        * is set to RTE_MEMAREA_SOURCE_USER_ADDR.
> +        */
> +       void *user_addr;
> +       /** User provided memarea, this field is valid only when source
> +        * is set to RTE_MEMAREA_SOURCE_MEMAREA.
> +        */
> +       struct rte_memarea *user_memarea;
>

Jerin already suggested a union here.
I'll add another reason to do so: if in the future there will be new
memarea types
that require new options, one pointer-sized field can be used to pass
anything
without breaking the ABI once this structure becomes stable.


> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Update memory's refcnt.
> + *
> + * Update one memory region's refcnt.
> + *
> + * @param ma
> + *   The pointer of memarea.
> + * @param ptr
> + *   The pointer of memory region which need be updated refcnt.
> + * @param value
> + *   The value which need be updated.
> + *   Note: it could be negative.
> + *
> + * @return
> + *   0 on success. Otherwise negative value is returned.
> + */
> +__rte_experimental
> +int rte_memarea_refcnt_update(struct rte_memarea *ma, void *ptr, int16_t
> value);
>

If this function only updates the refcnt, an API to inspect the refcnt is
missing.
Furthermore, in this case refcnt is just a value attached to every object,
what is the benefit compared to simply storing it in the object?

If this function also frees "ptr" when refcnt reaches zero,
missing is a way for the user to know that it did.
What happens if refcnt > 1 on rte_memarea_free()?

I don't think refcnt belongs to this library.
A principal objection: memarea is for freeing all objects at once,
refcnt is for releasing objects one-by-one when they're not used.
Technical issues I foresee: refcnt can be atomic (and require alignment) or
not,
16 bits may be too few (rte_flow_action_handle ref'd by thousands of
rte_flow).
Refcnt could be optional per memarea, but it seems like another
complication.

[-- Attachment #2: Type: text/html, Size: 6663 bytes --]