[ovs-dev,2/3] util: Add allocation wrappers that don't increment coverage counters.

The thread-local data allocators can't increment coverage counters
because this can cause reentrancy.  Until now, this code has used
explicit calls to malloc().  This code replaces them by calls to the
new functions.  This will make it easier in an upcoming patch to update
all the code that can run out of memory.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/ovs-thread.h | 12 ++----------
 lib/util.c       | 41 +++++++++++++++++++++++++++++++++--------
 lib/util.h       | 10 +++++++++-
 3 files changed, 44 insertions(+), 19 deletions(-)

On 3/26/21 7:30 PM, Ben Pfaff wrote:
> The thread-local data allocators can't increment coverage counters
> because this can cause reentrancy.  Until now, this code has used
> explicit calls to malloc().  This code replaces them by calls to the
> new functions.  This will make it easier in an upcoming patch to update
> all the code that can run out of memory.

Commit message should be adjusted if we're no going to have patch #3.
Beside that and a couple of minor comments inline it looks OK to me.
Have no strong opinion if we need this patch or not.

Anyway,
Acked-by: Ilya Maximets <i.maximets@ovn.org>

> 
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>  lib/ovs-thread.h | 12 ++----------
>  lib/util.c       | 41 +++++++++++++++++++++++++++++++++--------
>  lib/util.h       | 10 +++++++++-
>  3 files changed, 44 insertions(+), 19 deletions(-)
> 
> diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
> index 1050fc29af7c..7552a4e4b215 100644
> --- a/lib/ovs-thread.h
> +++ b/lib/ovs-thread.h
> @@ -294,11 +294,7 @@ void xpthread_join(pthread_t, void **);
>          value = NAME##_get_unsafe();                                    \
>          if (!value) {                                                   \
>              static const NAME##_type initial_value = __VA_ARGS__;       \
> -                                                                        \

I'd like to keep the empty line here for readability.

> -            value = malloc(sizeof *value);                              \
> -            if (value == NULL) {                                        \
> -                out_of_memory();                                        \
> -            }                                                           \
> +            value = xmalloc__(sizeof *value);                           \
>              *value = initial_value;                                     \
>              xpthread_setspecific(NAME##_key, value);                    \
>          }                                                               \
> @@ -334,11 +330,7 @@ void xpthread_join(pthread_t, void **);
>          value = NAME##_get_unsafe();                                    \
>          if (!value) {                                                   \
>              static const NAME##_type initial_value = __VA_ARGS__;       \
> -                                                                        \

Same here.

> -            value = malloc(sizeof *value);                              \
> -            if (value == NULL) {                                        \
> -                out_of_memory();                                        \
> -            }                                                           \
> +            value = xmalloc__(sizeof *value);                           \
>              *value = initial_value;                                     \
>              xpthread_setspecific(NAME##_key, value);                    \
>          }                                                               \
> diff --git a/lib/util.c b/lib/util.c
> index 25635b27ff00..1195c7982118 100644
> --- a/lib/util.c
> +++ b/lib/util.c
> @@ -116,10 +116,9 @@ out_of_memory(void)
>  }
>  
>  void *
> -xcalloc(size_t count, size_t size)
> +xcalloc__(size_t count, size_t size)
>  {
>      void *p = count && size ? calloc(count, size) : malloc(1);
> -    COVERAGE_INC(util_xalloc);
>      if (p == NULL) {
>          out_of_memory();
>      }
> @@ -127,16 +126,15 @@ xcalloc(size_t count, size_t size)
>  }
>  
>  void *
> -xzalloc(size_t size)
> +xzalloc__(size_t size)
>  {
> -    return xcalloc(1, size);
> +    return xcalloc__(1, size);
>  }
>  
>  void *
> -xmalloc(size_t size)
> +xmalloc__(size_t size)
>  {
>      void *p = malloc(size ? size : 1);
> -    COVERAGE_INC(util_xalloc);
>      if (p == NULL) {
>          out_of_memory();
>      }
> @@ -144,16 +142,43 @@ xmalloc(size_t size)
>  }
>  
>  void *
> -xrealloc(void *p, size_t size)
> +xrealloc__(void *p, size_t size)
>  {
>      p = realloc(p, size ? size : 1);
> -    COVERAGE_INC(util_xalloc);
>      if (p == NULL) {
>          out_of_memory();
>      }
>      return p;
>  }
>  
> +void *
> +xcalloc(size_t count, size_t size)
> +{
> +    COVERAGE_INC(util_xalloc);
> +    return xcalloc__(count, size);
> +}
> +
> +void *
> +xzalloc(size_t size)
> +{
> +    COVERAGE_INC(util_xalloc);
> +    return xzalloc__(size);
> +}
> +
> +void *
> +xmalloc(size_t size)
> +{
> +    COVERAGE_INC(util_xalloc);
> +    return xmalloc__(size);
> +}
> +
> +void *
> +xrealloc(void *p, size_t size)
> +{
> +    COVERAGE_INC(util_xalloc);
> +    return xrealloc__(p, size);
> +}
> +
>  void *
>  xmemdup(const void *p_, size_t size)
>  {
> diff --git a/lib/util.h b/lib/util.h
> index 067dcad15786..9c2b14fae304 100644
> --- a/lib/util.h
> +++ b/lib/util.h
> @@ -145,8 +145,9 @@ void ovs_print_version(uint8_t min_ofp, uint8_t max_ofp);
>  
>  void set_memory_locked(void);
>  bool memory_locked(void);
> -

And here.

>  OVS_NO_RETURN void out_of_memory(void);
> +
> +/* Allocation wrappers that abort if memory is exhausted. */
>  void *xmalloc(size_t) MALLOC_LIKE;
>  void *xcalloc(size_t, size_t) MALLOC_LIKE;
>  void *xzalloc(size_t) MALLOC_LIKE;
> @@ -160,6 +161,13 @@ char *xasprintf(const char *format, ...) OVS_PRINTF_FORMAT(1, 2) MALLOC_LIKE;
>  char *xvasprintf(const char *format, va_list) OVS_PRINTF_FORMAT(1, 0) MALLOC_LIKE;
>  void *x2nrealloc(void *p, size_t *n, size_t s);
>  
> +/* Allocation wrappers for specialized situations where coverage counters
> + * cannot be used. */
> +void *xmalloc__(size_t) MALLOC_LIKE;
> +void *xcalloc__(size_t, size_t) MALLOC_LIKE;
> +void *xzalloc__(size_t) MALLOC_LIKE;
> +void *xrealloc__(void *, size_t);
> +
>  void *xmalloc_cacheline(size_t) MALLOC_LIKE;
>  void *xzalloc_cacheline(size_t) MALLOC_LIKE;
>  void free_cacheline(void *);
>

On Wed, Apr 07, 2021 at 01:12:11PM +0200, Ilya Maximets wrote:
> On 3/26/21 7:30 PM, Ben Pfaff wrote:
> > The thread-local data allocators can't increment coverage counters
> > because this can cause reentrancy.  Until now, this code has used
> > explicit calls to malloc().  This code replaces them by calls to the
> > new functions.  This will make it easier in an upcoming patch to update
> > all the code that can run out of memory.
> 
> Commit message should be adjusted if we're no going to have patch #3.
> Beside that and a couple of minor comments inline it looks OK to me.
> Have no strong opinion if we need this patch or not.
> 
> Anyway,
> Acked-by: Ilya Maximets <i.maximets@ovn.org>

Thanks, I folded in your comments and applied this to master.