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. 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
