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 <[email protected]> > --- > 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 <[email protected]> > + * > + * 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 _______________________________________________ lttng-dev mailing list [email protected] http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
