Re: [systemd-devel] [PATCH 1/2] RFC: journald: Allow to cache the cg_get_root_path

2013-07-18 Thread Holger Hans Peter Freyther
On Thu, Jul 18, 2013 at 08:32:18PM +0200, Karol Lewandowski wrote:

> Agreed, this is why I have been naively thinking that caching wouldn't
> be that bad... However, sending all the required information in message
> itself is clearly the best solution to this problem.

It is nice to avoid the race for sure but unless I am not using perf
record/top correctly or perf itself is broken on ARM, the bottleneck
is really not the opening of files. For me glibc's _int_malloc remains
at the top. So even if we skip a log of reading of files the journald
will still create strings for KEY=VALUE.

So maybe we could put key/val separtaely into the iovec array and avoid
the extra strdup and then in the journal-file we can avoid the memchr
for '='? Anyway I have not looked at journal-file.c much yet...

holger

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


Re: [systemd-devel] [PATCH 1/2] RFC: journald: Allow to cache the cg_get_root_path

2013-07-18 Thread Karol Lewandowski
On 07/16/2013 03:26 AM, Lennart Poettering wrote:
> On Mon, 08.07.13 18:39, Karol Lewandowski (k.lewando...@samsung.com) wrote:

>> According to my analysis /proc access is costly and it would
>> be best to cache the result for later use.  Difficulty comes
>> from trying to keep cache up to date, though.
> 
> We can't really cache this. This stuff is already racy, as by the time
> we read the attributes the process might already be gone.

Agreed, this is why I have been naively thinking that caching wouldn't
be that bad... However, sending all the required information in message
itself is clearly the best solution to this problem.

> I think the best way to deal with the performance issue here is the
> stuff discussed here:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=963620
> 
> i.e. just have the kernel augment our messages with the data we need,
> unconditionally. That way we fix both the race issue, and the
> performance issue, since the data is just there and we can make use of
> it without any further work.

Thanks a lot for this link!

Having this issue sorted out will allow me to take closer look
into "eliminating" 64-bit divisions in message receive path.
ARM doesn't support divide operation so every div[1] is being
done purely in software.

[1] Every div/% by non-constant value and every div/% by
64-bit constant results in call to (slow) __aeabi_div*.

Thanks
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


Re: [systemd-devel] [PATCH 1/2] RFC: journald: Allow to cache the cg_get_root_path

2013-07-15 Thread Lennart Poettering
On Mon, 08.07.13 18:39, Karol Lewandowski (k.lewando...@samsung.com) wrote:

> 
> On 06/27/2013 06:30 PM, Holger Hans Peter Freyther wrote:
> > From: Holger Hans Peter Freyther 
> > 
> > Allow to cache the cg_get_root_path and introduce a new method
> > cg_pid_get_path_shifted_with_root that can use the cached version
> > instead of allocating a new string.
> 
> My 2c,
> 
> I have been thinking about something similar albeit a bit more
> generic.
> 
> According to my analysis /proc access is costly and it would
> be best to cache the result for later use.  Difficulty comes
> from trying to keep cache up to date, though.

We can't really cache this. This stuff is already racy, as by the time
we read the attributes the process might already be gone.

I think the best way to deal with the performance issue here is the
stuff discussed here:

https://bugzilla.redhat.com/show_bug.cgi?id=963620

i.e. just have the kernel augment our messages with the data we need,
unconditionally. That way we fix both the race issue, and the
performance issue, since the data is just there and we can make use of
it without any further work.

> I've just started looking into connector's cn_proc which _seem_
> to offer all the required functionality (fork, exec, exit
> notifications).  I have to finish my prototype to verify if
> it's worth complications it brings.

cn_proc is asynchronous, so you get even more race problems like this,
where processes are already gone by the time you get the message about
them.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


Re: [systemd-devel] [PATCH 1/2] RFC: journald: Allow to cache the cg_get_root_path

2013-07-15 Thread Lennart Poettering
On Thu, 27.06.13 18:30, Holger Hans Peter Freyther (hol...@freyther.de) wrote:

> From: Holger Hans Peter Freyther 
> 
> Allow to cache the cg_get_root_path and introduce a new method
> cg_pid_get_path_shifted_with_root that can use the cached version
> instead of allocating a new string.

Sorry for the delay in reviewing.

THe general approach of caching the result of cg_get_root_path() is a
good one, but I am not fond of placing this in a static variable. We try
to avoid that where possible, especially if we have some kind of context
object around anyway where we could place this.

> +if (!cg_root)
> +return -1;

We use negative "errno"-style errors everywhere for indicating errors,
not -1. i.e. please use "return -EINVAL" or suchlike, but not literal
numeric constants.

Thanks,

Lennart

-- 
Lennart Poettering - Red Hat, Inc.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


Re: [systemd-devel] [PATCH 1/2] RFC: journald: Allow to cache the cg_get_root_path

2013-07-08 Thread Karol Lewandowski
On 06/27/2013 06:30 PM, Holger Hans Peter Freyther wrote:
> From: Holger Hans Peter Freyther 
> 
> Allow to cache the cg_get_root_path and introduce a new method
> cg_pid_get_path_shifted_with_root that can use the cached version
> instead of allocating a new string.

My 2c,

I have been thinking about something similar albeit a bit more
generic.

According to my analysis /proc access is costly and it would
be best to cache the result for later use.  Difficulty comes
from trying to keep cache up to date, though.

I've just started looking into connector's cn_proc which _seem_
to offer all the required functionality (fork, exec, exit
notifications).  I have to finish my prototype to verify if
it's worth complications it brings.

Thanks
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


[systemd-devel] [PATCH 1/2] RFC: journald: Allow to cache the cg_get_root_path

2013-06-27 Thread Holger Hans Peter Freyther
From: Holger Hans Peter Freyther 

Allow to cache the cg_get_root_path and introduce a new method
cg_pid_get_path_shifted_with_root that can use the cached version
instead of allocating a new string.
---
 src/journal/journald-server.c |   20 +---
 src/shared/cgroup-util.c  |   19 ++-
 src/shared/cgroup-util.h  |1 +
 3 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/src/journal/journald-server.c b/src/journal/journald-server.c
index 44ba916..b08aa1d 100644
--- a/src/journal/journald-server.c
+++ b/src/journal/journald-server.c
@@ -89,6 +89,20 @@ static const char* const split_mode_table[] = {
 DEFINE_STRING_TABLE_LOOKUP(split_mode, SplitMode);
 DEFINE_CONFIG_PARSE_ENUM(config_parse_split_mode, split_mode, SplitMode, 
"Failed to parse split mode setting");
 
+static const char *cached_cg_root(void) {
+static char *cached = NULL;
+
+int r;
+
+if (!cached) {
+r = cg_get_root_path(&cached);
+if (r < 0)
+cached = NULL;
+}
+
+return cached;
+}
+
 static uint64_t available_space(Server *s, bool verbose) {
 char ids[33];
 _cleanup_free_ char *p = NULL;
@@ -592,7 +606,7 @@ static void dispatch_message_real(
 }
 #endif
 
-r = cg_pid_get_path_shifted(ucred->pid, NULL, &c);
+r = cg_pid_get_path_shifted_with_root(ucred->pid, NULL, &c, 
cached_cg_root());
 if (r >= 0) {
 char *session = NULL;
 
@@ -701,7 +715,7 @@ static void dispatch_message_real(
 }
 #endif
 
-r = cg_pid_get_path_shifted(object_pid, NULL, &c);
+r = cg_pid_get_path_shifted_with_root(object_pid, NULL, &c, 
cached_cg_root());
 if (r >= 0) {
 x = strappenda("OBJECT_SYSTEMD_CGROUP=", c);
 IOVEC_SET_STRING(iovec[n++], x);
@@ -841,7 +855,7 @@ void server_dispatch_message(
 if (!ucred)
 goto finish;
 
-r = cg_pid_get_path_shifted(ucred->pid, NULL, &path);
+r = cg_pid_get_path_shifted_with_root(ucred->pid, NULL, &path, 
cached_cg_root());
 if (r < 0)
 goto finish;
 
diff --git a/src/shared/cgroup-util.c b/src/shared/cgroup-util.c
index 5816b7d..1a76794 100644
--- a/src/shared/cgroup-util.c
+++ b/src/shared/cgroup-util.c
@@ -1059,14 +1059,12 @@ char **cg_shorten_controllers(char **controllers) {
 return strv_uniq(controllers);
 }
 
-int cg_pid_get_path_shifted(pid_t pid, char **root, char **cgroup) {
-_cleanup_free_ char *cg_root = NULL;
+int cg_pid_get_path_shifted_with_root(pid_t pid, char **root, char **cgroup, 
const char *cg_root) {
 char *cg_process, *p;
 int r;
 
-r = cg_get_root_path(&cg_root);
-if (r < 0)
-return r;
+if (!cg_root)
+return -1;
 
 r = cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, pid, &cg_process);
 if (r < 0)
@@ -1099,6 +1097,17 @@ int cg_pid_get_path_shifted(pid_t pid, char **root, char 
**cgroup) {
 return 0;
 }
 
+int cg_pid_get_path_shifted(pid_t pid, char **root, char **cgroup) {
+_cleanup_free_ char *cg_root = NULL;
+int r;
+
+r = cg_get_root_path(&cg_root);
+if (r < 0)
+return r;
+
+return cg_pid_get_path_shifted_with_root(pid, root, cgroup, cg_root);
+}
+
 int cg_path_decode_unit(const char *cgroup, char **unit){
 char *p, *e, *c, *s, *k;
 
diff --git a/src/shared/cgroup-util.h b/src/shared/cgroup-util.h
index 9883d94..7f6605c 100644
--- a/src/shared/cgroup-util.h
+++ b/src/shared/cgroup-util.h
@@ -104,6 +104,7 @@ int cg_path_get_machine_name(const char *path, char 
**machine);
 int cg_path_get_slice(const char *path, char **slice);
 
 int cg_pid_get_path_shifted(pid_t pid, char **root, char **cgroup);
+int cg_pid_get_path_shifted_with_root(pid_t pid, char **root, char **cgroup, 
const char *cg_root);
 
 int cg_pid_get_session(pid_t pid, char **session);
 int cg_pid_get_owner_uid(pid_t pid, uid_t *uid);
-- 
1.7.10.4

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel