[Pvfs2-developers] [PATCH] threaded kmod helper
Sam Lang
slang at mcs.anl.gov
Thu Feb 14 18:18:27 EST 2008
On Feb 14, 2008, at 4:34 PM, Pete Wyckoff wrote:
> Add --enable option to build a threaded pvfs2-client-core, that is,
> an executable linked against pvfs2-threaded.so.
>
> Remove the "--threaded" argument from pvfs2-client. Whatever version
> of client-core that was configured is all that is available at
> runtime.
> It will always be installed as pvfs2-client-core.
> ---
> Makefile.in | 15 +++++++--------
> configure | 17 +++++++++++++++--
> configure.in | 20 ++++++++++++++++++++
> src/apps/kernel/linux/module.mk.in | 9 ++++++---
> src/apps/kernel/linux/pvfs2-client.c | 31 +
> +-----------------------------
> 5 files changed, 50 insertions(+), 42 deletions(-)
>
>
> What does everybody think about this? I had complained a long time
> ago about always getting a pvfs2-client-core and
> pvfs2-client-core-threaded during a kernapps build. The latter
> brings in the entire libpvfs2-threaded.a and all the objects for
> that.
>
> I don't have a feel for who uses the threaded client core. The only
> way to use it now is to start pvfs2-client with the "--threaded"
> option. So I made the --enable option default to off. Is there a
> reason to want both threaded and non-threaded versions installed?
I think the patch looks great. My only concern is that this will make
the threaded client daemon never get used. My view is that we should
probably have the threaded version as the default. I think in the
general case, the threaded version is going to perform better. Its
pretty hard to buy a system that doesn't have at least two cores
today, and even on a uni-processor system, the performance of the
threaded daemon shouldn't be much worse than the non-threaded.
I originally added the threaded client functionality to see if it
would help performance, but it turned out to be buggy. Maybe with
Phil's recent thread-safety fixes to the system interfaces, we should
revisit making the threaded daemon the default.
I'm not sure that the overhead of a single operation will be affected
much, honestly. We can't disable threads and locking entirely, since
we need the remount thread running. I think the patch I just
committed to fix the segfault in HEAD will likely help with individual
operation performance more than disabling threads would. Just my 2c
though.
Its hard to get a feeling for how much the threaded daemon would
help without a thorough performance analysis of the two on a number of
different systems. Something I would like to do at some point, but
its pretty low on the list.
-sam
>
>
> Please also correct me on the "dnl Enabling this option ..." section
> below. I won't check any of this in unless I get some good feedback.
>
> My next steps would be to make more of the various threading knobs
> configurable at compile time without editing Makefile.in. There's
> lots, and they're a bit messy.
>
> -- Pete
>
>
>
> diff --git a/Makefile.in b/Makefile.in
> index 33e123b..2a6b766 100644
> --- a/Makefile.in
> +++ b/Makefile.in
> @@ -406,8 +406,7 @@ VISMISCSRC :=
> KARMASRC :=
> # userland helper programs for kernel drivers
> KERNAPPSRC :=
> -#
> -KERNAPPSTHRSRC :=
> +KERNAPPTHRSRC :=
> # MISCSRC are sources that don't fall into the other categories
> MISCSRC :=
> # c files generated from state machines
> @@ -498,14 +497,14 @@ MISCOBJS := $(patsubst %.c,%.o, $(filter %.c,$
> (MISCSRC)))
> MISCDEPENDS := $(patsubst %.c,%.d, $(filter %.c,$(MISCSRC)))
>
> # KERNAPPOBJS is a list of kernel driver userland objects
> -KERNAPPOBJS := $(patsubst %.c,%.o, $(filter %.c,$(KERNAPPSRC)))
> +KERNAPPOBJS := $(patsubst %.c,%.o, $(filter %.c,$(KERNAPPSRC))) \
> + $(patsubst %.c,%-threaded.o, $(filter %.c,$
> (KERNAPPTHRSRC)))
> # KERNAPPS is a list of kernel driver userland executables
> KERNAPPS := $(patsubst %.c,%, $(filter %.c, $(KERNAPPSRC)))
> +KERNAPPSTHR := $(patsubst %.c,%, $(filter %.c, $(KERNAPPTHRSRC)))
> # KERNAPPDEPENDS is a list of dependency files for kernel driver
> userland
> # objects
> -KERNAPPDEPENDS := $(patsubst %.c,%.d, $(filter %.c,$(KERNAPPSRC)))
> -
> -KERNAPPSTHR := $(patsubst %.c,%-threaded, $(filter %.c, $
> (KERNAPPSTHRSRC)))
> +KERNAPPDEPENDS := $(patsubst %.c,%.d, $(filter %.c,$(KERNAPPSRC) $
> (KERNAPPTHRSRC)))
>
> # VISOBJS is a list of visualization program objects
> VISOBJS := $(patsubst %.c,%.o, $(filter %.c,$(VISSRC)))
> @@ -943,7 +942,7 @@ just_kmod_install: just_kmod
> .PHONY: kmod_install
> kmod_install: kmod kernapps just_kmod_install
> install -d $(prefix)/sbin
> - install -m 755 $(KERNAPPS) $(prefix)/sbin
> + install -m 755 $(KERNAPPS) $(KERNAPPSTHR) $(prefix)/sbin
> endif
>
> ifneq (,$(LINUX24_KERNEL_SRC))
> @@ -964,7 +963,7 @@ just_kmod24_install: just_kmod24
> .PHONY: kmod24_install
> kmod24_install: kmod24 kernapps just_kmod24_install
> install -d $(prefix)/sbin
> - install -m 755 $(KERNAPPS) $(prefix)/sbin
> + install -m 755 $(KERNAPPS) $(KERNAPPSTHR) $(prefix)/sbin
> install -m 755 src/apps/kernel/linux/mount.pvfs2 $(prefix)/sbin
> @echo ""
> @echo "For improved linux-2.4 support,"
> diff --git a/configure b/configure
> index fb5ca35..e2e6718 100755
> --- a/configure
> +++ b/configure
> @@ -692,6 +692,7 @@ build_static
> REDHAT_RELEASE
> NPTL_WORKAROUND
> MISC_TROVE_FLAGS
> +THREADED_KMOD_HELPER
> LINUX_KERNEL_SRC
> LINUX24_KERNEL_SRC
> LINUX24_KERNEL_MINOR_VER
> @@ -1331,6 +1332,7 @@ Optional Features:
> --disable-aio-threaded-callbacks Disable use of AIO threaded
> callbacks
> --disable-kernel-aio Forcibly disable kernel aio
> --enable-kernel-sendfile Forcibly enable kernel sendfile
> + --enable-threaded-kmod-helper Use threads in the kernel helper
> application
> --enable-fast Disable optional debugging, enable
> optimizations.
> --enable-strict Turn on strict compiler warnings
> --enable-verbose-build Enables full output during build process
> @@ -11782,6 +11784,16 @@ _ACEOF
>
> fi
>
> +# Check whether --enable-threaded-kmod-helper was given.
> +if test "${enable_threaded_kmod_helper+set}" = set; then
> + enableval=$enable_threaded_kmod_helper; if test "x$enableval" =
> "xyes" ; then
> + THREADED_KMOD_HELPER=yes
> + fi
> +
> +fi
> +
> +
> +
> BUILD_ABSOLUTE_TOP=`pwd`
> SRC_RELATIVE_TOP=$srcdir
> SRC_ABSOLUTE_TOP=`cd $srcdir ; pwd`
> @@ -18482,6 +18494,7 @@ build_static!$build_static$ac_delim
> REDHAT_RELEASE!$REDHAT_RELEASE$ac_delim
> NPTL_WORKAROUND!$NPTL_WORKAROUND$ac_delim
> MISC_TROVE_FLAGS!$MISC_TROVE_FLAGS$ac_delim
> +THREADED_KMOD_HELPER!$THREADED_KMOD_HELPER$ac_delim
> LINUX_KERNEL_SRC!$LINUX_KERNEL_SRC$ac_delim
> LINUX24_KERNEL_SRC!$LINUX24_KERNEL_SRC$ac_delim
> LINUX24_KERNEL_MINOR_VER!$LINUX24_KERNEL_MINOR_VER$ac_delim
> @@ -18499,7 +18512,6 @@ GNUC!$GNUC$ac_delim
> DB_CFLAGS!$DB_CFLAGS$ac_delim
> DB_LIB!$DB_LIB$ac_delim
> NEEDS_LIBRT!$NEEDS_LIBRT$ac_delim
> -TARGET_OS_DARWIN!$TARGET_OS_DARWIN$ac_delim
> _ACEOF
>
> if test `sed -n "s/.*$ac_delim\$/X/p" conf$$subs.sed | grep -c X`
> = 97; then
> @@ -18541,6 +18553,7 @@ _ACEOF
> ac_delim='%!_!# '
> for ac_last_try in false false false false false :; do
> cat >conf$$subs.sed <<_ACEOF
> +TARGET_OS_DARWIN!$TARGET_OS_DARWIN$ac_delim
> TARGET_OS_LINUX!$TARGET_OS_LINUX$ac_delim
> BUILD_BMI_TCP!$BUILD_BMI_TCP$ac_delim
> BUILD_GM!$BUILD_GM$ac_delim
> @@ -18567,7 +18580,7 @@ LIBOBJS!$LIBOBJS$ac_delim
> LTLIBOBJS!$LTLIBOBJS$ac_delim
> _ACEOF
>
> - if test `sed -n "s/.*$ac_delim\$/X/p" conf$$subs.sed | grep -c X`
> = 24; then
> + if test `sed -n "s/.*$ac_delim\$/X/p" conf$$subs.sed | grep -c X`
> = 25; then
> break
> elif $ac_last_try; then
> { { echo "$as_me:$LINENO: error: could not make $CONFIG_STATUS"
> >&5
> diff --git a/configure.in b/configure.in
> index bf53e89..40c65a6 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -546,6 +546,26 @@ if test -n "$lk_src" ; then
> AC_DEFINE(WITH_LINUX_KMOD, 1, [Define to build for linux kernel
> module userspace helper.])
> fi
>
> +dnl
> +dnl Enabling this option links pvfs2-client-core against libpvfs2-
> threaded.so.
> +dnl The objects in there were compiled using __GEN_POSIX_LOCKING__
> and
> +dnl __PVFS2_JOB_THREADED__. If enabled, the client core will run
> with two
> +dnl extra threads: one for listening to the kernel device, and
> another
> +dnl to interact with the network. This has the potential to
> increase the
> +dnl rate at which concurrent operations are processed, but has the
> drawback
> +dnl of somewhat higher overhead for a single operation.
> +dnl
> +dnl Note that even without this option, pvfs2-client-core always
> requires
> +dnl pthreads to run its remount thread.
> +dnl
> +AC_ARG_ENABLE([threaded-kmod-helper],
> +[ --enable-threaded-kmod-helper Use threads in the kernel helper
> application],
> +[ if test "x$enableval" = "xyes" ; then
> + THREADED_KMOD_HELPER=yes
> + fi
> +])
> +AC_SUBST(THREADED_KMOD_HELPER)
> +
> dnl PAV configuration needs absolute location of source and build.
> dnl Linux-2.6 module needs absolute location of source, and uses the
> dnl relative location for soft links for out-of-tree builds.
> diff --git a/src/apps/kernel/linux/module.mk.in b/src/apps/kernel/
> linux/module.mk.in
> index 7e71f5f..6e69bb8 100644
> --- a/src/apps/kernel/linux/module.mk.in
> +++ b/src/apps/kernel/linux/module.mk.in
> @@ -1,11 +1,14 @@
> DIR := src/apps/kernel/linux
>
> KERNAPPSRC += \
> - $(DIR)/pvfs2-client-core.c \
> $(DIR)/pvfs2-client.c
>
> -KERNAPPSTHRSRC += \
> - $(DIR)/pvfs2-client-core.c
> +# if requested, build a threaded client core
> +ifeq (, at THREADED_KMOD_HELPER@)
> +KERNAPPSRC += $(DIR)/pvfs2-client-core.c
> +else
> +KERNAPPTHRSRC += $(DIR)/pvfs2-client-core.c
> +endif
>
> ifneq (,$(LINUX24_KERNEL_SRC))
> KERNAPPSRC += $(DIR)/mount.pvfs2.c
> diff --git a/src/apps/kernel/linux/pvfs2-client.c b/src/apps/kernel/
> linux/pvfs2-client.c
> index dd38e39..09d2bad 100644
> --- a/src/apps/kernel/linux/pvfs2-client.c
> +++ b/src/apps/kernel/linux/pvfs2-client.c
> @@ -28,7 +28,6 @@
>
> #define PVFS2_CLIENT_CORE_SUFFIX "-core"
> #define PVFS2_CLIENT_CORE_NAME "pvfs2-client" PVFS2_CLIENT_CORE_SUFFIX
> -#define PVFS2_CLIENT_CORE_THR_SUFFIX "-threaded"
>
> static char s_client_core_path[PATH_MAX];
>
> @@ -59,7 +58,6 @@ typedef struct
> char *logstamp;
> char *dev_buffer_count;
> char *dev_buffer_size;
> - int threaded;
> char *logtype;
> } options_t;
>
> @@ -282,15 +280,7 @@ static int monitor_pvfs2_client(options_t *opts)
> {
> sleep(1);
>
> - if(opts->threaded)
> - {
> - arg_list[0] = PVFS2_CLIENT_CORE_NAME
> PVFS2_CLIENT_CORE_THR_SUFFIX;
> - }
> - else
> - {
> - arg_list[0] = PVFS2_CLIENT_CORE_NAME;
> - }
> -
> + arg_list[0] = PVFS2_CLIENT_CORE_NAME;
> arg_index = 1;
> arg_list[arg_index++] = "-a";
> arg_list[arg_index++] = opts->acache_timeout;
> @@ -429,7 +419,6 @@ static void print_help(char *progname)
> "PATH\n");
> printf("--logstamp=none|usec|datetime override default log
> message time stamp format\n");
> printf("--logtype=file|syslog specify writing logs to
> file or syslog\n");
> - printf("--threaded use threaded client\n");
> }
>
> static void parse_args(int argc, char **argv, options_t *opts)
> @@ -460,7 +449,6 @@ static void parse_args(int argc, char **argv,
> options_t *opts)
> {"gossip-mask",1,0,0},
> {"path",1,0,0},
> {"logstamp",1,0,0},
> - {"threaded",0,0,0},
> {0,0,0,0}
> };
>
> @@ -569,11 +557,6 @@ static void parse_args(int argc, char **argv,
> options_t *opts)
> opts->gossip_mask = optarg;
> break;
> }
> - else if (strcmp("threaded", cur_option) == 0)
> - {
> - opts->threaded = 1;
> - break;
> - }
> break;
> case 'h':
> do_help:
> @@ -644,17 +627,7 @@ static void parse_args(int argc, char **argv,
> options_t *opts)
>
> if (!opts->path)
> {
> - if(opts->threaded)
> - {
> - sprintf(s_client_core_path,
> - "%s" PVFS2_CLIENT_CORE_SUFFIX
> PVFS2_CLIENT_CORE_THR_SUFFIX,
> - argv[0]);
> - }
> - else
> - {
> - sprintf(s_client_core_path, "%s"
> PVFS2_CLIENT_CORE_SUFFIX,
> - argv[0]);
> - }
> + sprintf(s_client_core_path, "%s" PVFS2_CLIENT_CORE_SUFFIX,
> argv[0]);
> opts->path = s_client_core_path;
> }
>
> --
> 1.5.3.8
>
> _______________________________________________
> Pvfs2-developers mailing list
> Pvfs2-developers at beowulf-underground.org
> http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
>
More information about the Pvfs2-developers
mailing list