[lttng-dev] [PATCH 1/3] Add default subbuf sizes getter functions.

[David Goulet]

Hi Simon,

Can you please rename the get_default_* function family to default_get_*
in order to respect the name spacing of files.

Comment below:

Simon Marchi:
> This patch adds src/common/defaults.c file. It contains functions to
> retrieve defaults subbuf sizes based on the architecture page size.
> 
> Signed-off-by: Simon Marchi <simon.marchi at polymtl.ca>
> ---
>  src/common/Makefile.am |    2 +-
>  src/common/defaults.c  |   86 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/common/defaults.h  |    7 ++++
>  3 files changed, 94 insertions(+), 1 deletions(-)
>  create mode 100644 src/common/defaults.c
> 
> diff --git a/src/common/Makefile.am b/src/common/Makefile.am
> index b43b376..f91259c 100644
> --- a/src/common/Makefile.am
> +++ b/src/common/Makefile.am
> @@ -12,7 +12,7 @@ noinst_HEADERS = lttng-kernel.h defaults.h macros.h error.h futex.h \
>  noinst_LTLIBRARIES = libcommon.la
>  
>  libcommon_la_SOURCES = error.h error.c utils.c utils.h runas.c runas.h \
> -                       common.h futex.c futex.h uri.c uri.h
> +                       common.h futex.c futex.h uri.c uri.h defaults.c
>  
>  # Consumer library
>  noinst_LTLIBRARIES += libconsumer.la
> diff --git a/src/common/defaults.c b/src/common/defaults.c
> new file mode 100644
> index 0000000..48d6605
> --- /dev/null
> +++ b/src/common/defaults.c
> @@ -0,0 +1,86 @@
> +/*
> + * Copyright (C) 2012 - Simon Marchi <simon.marchi at polymtl.ca>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2 only,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <stddef.h>
> +#include <unistd.h>
> +
> +#include "defaults.h"
> +
> +static size_t default_channel_subbuf_size;
> +static size_t default_metadata_subbuf_size;
> +static size_t default_kernel_channel_subbuf_size;
> +static size_t default_ust_channel_subbuf_size;
> +
> +static void __attribute__((constructor)) init_defaults(void)
> +{
> +	default_kernel_channel_subbuf_size = DEFAULT_KERNEL_CHANNEL_SUBBUF_SIZE; 

Why is the kernel getting a default value and not a page size ?

> +	
> +	long page_size = sysconf(_SC_PAGESIZE);
> +	if (page_size > 0) {
> +		default_channel_subbuf_size = page_size;
> +		default_metadata_subbuf_size = page_size;
> +		default_ust_channel_subbuf_size = page_size;
> +	} else {
> +		default_channel_subbuf_size = 4096;
> +		default_metadata_subbuf_size = 4096;
> +		default_ust_channel_subbuf_size = 4096;

Should we use a default value for this 4096 size here with a
DEFINE_*_SUBBUF_SIZE or... ?

Seems a bit odd that the kernel got a default define value but not UST
and metadata.

> +	}
> +}
> +
> +/*
> + * Returns the default subbuf size.
> + *
> + * This function depends on a value that is set at constructor time, so it is
> + * unsafe to call it from another constructor.
> + */
> +size_t get_default_channel_subbuf_size(void)
> +{
> +	return default_channel_subbuf_size;
> +}
> +
> +/*
> + * Returns the default metadata subbuf size.
> + *
> + * This function depends on a value that is set at constructor time, so it is
> + * unsafe to call it from another constructor.
> + */
> +size_t get_default_metadata_subbuf_size(void)
> +{
> +	return default_metadata_subbuf_size;
> +}
> +
> +/*
> + * Returns the default subbuf size for the kernel domain.
> + *
> + * This function depends on a value that is set at constructor time, so it is
> + * unsafe to call it from another constructor.
> + */
> +size_t get_default_kernel_channel_subbuf_size(void)
> +{
> +	return default_kernel_channel_subbuf_size;
> +}
> +
> +/*
> + * Returns the default subbuf size for the UST domain.
> + *
> + * This function depends on a value that is set at constructor time, so it is
> + * unsafe to call it from another constructor.
> + */
> +size_t get_default_ust_channel_subbuf_size(void)
> +{
> +	return default_ust_channel_subbuf_size;
> +}

I'm wondering if all these functions should be static inline in the
default.h...

Thoughts?

Otherwise, the patch looks good!

Thanks
David

> diff --git a/src/common/defaults.h b/src/common/defaults.h
> index 0c0b6ac..efff43e 100644
> --- a/src/common/defaults.h
> +++ b/src/common/defaults.h
> @@ -109,6 +109,9 @@
>  #define DEFAULT_METADATA_SUBBUF_SIZE    4096
>  #define DEFAULT_METADATA_SUBBUF_NUM     2
>  
> +size_t get_default_channel_subbuf_size(void);
> +size_t get_default_metadata_subbuf_size(void);
> +
>  /* Kernel has different defaults */
>  
>  /* DEFAULT_KERNEL_CHANNEL_SUBBUF_SIZE must always be a power of 2 */
> @@ -121,6 +124,8 @@
>  /* See lttng-kernel.h enum lttng_kernel_output for channel output */
>  #define DEFAULT_KERNEL_CHANNEL_OUTPUT       LTTNG_EVENT_SPLICE
>  
> +size_t get_default_kernel_channel_subbuf_size(void);
> +
>  /* User space defaults */
>  
>  /* Must be a power of 2 */
> @@ -130,6 +135,8 @@
>  /* See lttng-ust.h enum lttng_ust_output */
>  #define DEFAULT_UST_CHANNEL_OUTPUT          LTTNG_EVENT_MMAP
>  
> +size_t get_default_ust_channel_subbuf_size(void);
> +
>  /*
>   * Default timeout value for the sem_timedwait() call. Blocking forever is not
>   * wanted so a timeout is used to control the data flow and not freeze the