* Simon Marchi ([email protected]) wrote: > Hi David, > > I just sent my v2 for this patch. I changed the logic so that it still > uses the DEFAULT_*_SUBBUF_SIZE defines, but check that it is >= page > size. It should answer your first two comments/questions. > > I don't think it would help to put these inline, since they are only > called in slow paths.
Even though those are slow paths, adding a symbol and function call just to fetch a variable seems overkill. I agree with David: those should be made static inline. Thanks, Mathieu > > Simon > > > 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 > > _______________________________________________ > lttng-dev mailing list > [email protected] > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com _______________________________________________ lttng-dev mailing list [email protected] http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
