Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1444163 > unrolled thread

[PATCH 02/14] resource limits: aggregate task highwater marks to cgroup level

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 02/14] resource limits: aggregate task highwater marks to cgroup level Topi Miettinen <toiwoton@gmail.com> - 2016-07-15 12:40 +0200
    Re: [PATCH 02/14] resource limits: aggregate task highwater marks to  cgroup level kbuild test robot <lkp@intel.com> - 2016-07-15 14:40 +0200
    Re: [PATCH 02/14] resource limits: aggregate task highwater marks to  cgroup level Tejun Heo <tj@kernel.org> - 2016-07-15 16:20 +0200
      Re: [PATCH 02/14] resource limits: aggregate task highwater marks to  cgroup level Topi Miettinen <toiwoton@gmail.com> - 2016-07-15 19:20 +0200
        Re: [PATCH 02/14] resource limits: aggregate task highwater marks to  cgroup level Tejun Heo <tj@kernel.org> - 2016-07-19 01:00 +0200
          Re: [PATCH 02/14] resource limits: aggregate task highwater marks to  cgroup level Topi Miettinen <toiwoton@gmail.com> - 2016-07-19 19:00 +0200
            Re: [PATCH 02/14] resource limits: aggregate task highwater marks to  cgroup level Tejun Heo <tj@kernel.org> - 2016-07-19 20:20 +0200

#1444163 — [PATCH 02/14] resource limits: aggregate task highwater marks to cgroup level

Collect resource usage highwater marks of a task to cgroup
statistics when the task exits.

Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
---
 Documentation/accounting/getdelays.c | 10 ++++++-
 include/linux/cgroup-defs.h          |  5 ++++
 include/uapi/linux/cgroupstats.h     |  3 ++
 kernel/cgroup.c                      | 55 ++++++++++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/Documentation/accounting/getdelays.c b/Documentation/accounting/getdelays.c
index 489f1b7..7c86279 100644
--- a/Documentation/accounting/getdelays.c
+++ b/Documentation/accounting/getdelays.c
@@ -27,7 +27,7 @@
 
 #include <linux/genetlink.h>
 #include "include/uapi/linux/taskstats.h"
-#include <linux/cgroupstats.h>
+#include "include/uapi/linux/cgroupstats.h"
 
 /*
  * Generic macros for dealing with netlink sockets. Might be duplicated
@@ -258,12 +258,20 @@ static const char *const rlimit_names[] = {
 
 static void print_cgroupstats(struct cgroupstats *c)
 {
+	int i;
+
 	printf("sleeping %llu, blocked %llu, running %llu, stopped %llu, "
 		"uninterruptible %llu\n", (unsigned long long)c->nr_sleeping,
 		(unsigned long long)c->nr_io_wait,
 		(unsigned long long)c->nr_running,
 		(unsigned long long)c->nr_stopped,
 		(unsigned long long)c->nr_uninterruptible);
+
+	if (print_resource_accounting)
+		for (i = 0; i < RLIM_NLIMITS; i++)
+			printf("%s=%llu\n",
+			       rlimit_names[i],
+			       (unsigned long long)c->resource_hiwater[i]);
 }
 
 
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 5b17de6..86bbc08 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -16,6 +16,7 @@
 #include <linux/percpu-refcount.h>
 #include <linux/percpu-rwsem.h>
 #include <linux/workqueue.h>
+#include <linux/cgroupstats.h>
 
 #ifdef CONFIG_CGROUPS
 
@@ -300,6 +301,10 @@ struct cgroup {
 	/* used to schedule release agent */
 	struct work_struct release_agent_work;
 
+#ifdef CONFIG_TASK_XACCT
+	struct cgroupstats stats;
+#endif
+
 	/* ids of the ancestors at each level including self */
 	int ancestor_ids[];
 };
diff --git a/include/uapi/linux/cgroupstats.h b/include/uapi/linux/cgroupstats.h
index 3753c33..18b5b11 100644
--- a/include/uapi/linux/cgroupstats.h
+++ b/include/uapi/linux/cgroupstats.h
@@ -35,6 +35,9 @@ struct cgroupstats {
 	__u64	nr_uninterruptible;	/* Number of tasks in uninterruptible */
 					/* state */
 	__u64	nr_io_wait;		/* Number of tasks waiting on IO */
+	__u64   resource_hiwater[RLIM_NLIMITS]; /* high-watermark of
+						     RLIMIT
+						     resources */
 };
 
 /*
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 75c0ff0..9b2d805 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -247,6 +247,7 @@ static void kill_css(struct cgroup_subsys_state *css);
 static int cgroup_addrm_files(struct cgroup_subsys_state *css,
 			      struct cgroup *cgrp, struct cftype cfts[],
 			      bool is_add);
+static void cgroup_update_stats(void);
 
 /**
  * cgroup_ssid_enabled - cgroup subsys enabled test by subsys ID
@@ -2609,6 +2610,8 @@ out_release_tset:
 		list_splice_tail_init(&cset->mg_tasks, &cset->tasks);
 		list_del_init(&cset->mg_node);
 	}
+	cgroup_update_stats();
+
 	spin_unlock_irq(&css_set_lock);
 	return ret;
 }
@@ -4657,6 +4660,53 @@ static int pidlist_array_load(struct cgroup *cgrp, enum cgroup_filetype type,
 	return 0;
 }
 
+/*
+ * Update cgroupstats based on the stats from exiting task
+ */
+static void cgroup_update_stats_from_task(struct cgroup *cgrp,
+					  struct task_struct *tsk)
+{
+	struct signal_struct *sig = tsk->signal;
+	int i;
+	unsigned int seq, nextseq;
+	unsigned long flags;
+
+	rcu_read_lock();
+	/* Attempt a lockless read on the first round. */
+	nextseq = 0;
+	do {
+		seq = nextseq;
+		flags = read_seqbegin_or_lock_irqsave(&sig->stats_lock, &seq);
+		for (i = 0; i < RLIM_NLIMITS; i++)
+			if (cgrp->stats.resource_hiwater[i] <
+			    sig->resource_highwatermark[i])
+				cgrp->stats.resource_hiwater[i] =
+					sig->resource_highwatermark[i];
+
+		/* If lockless access failed, take the lock. */
+		nextseq = 1;
+	} while (need_seqretry(&sig->stats_lock, seq));
+	done_seqretry_irqrestore(&sig->stats_lock, seq, flags);
+	rcu_read_unlock();
+}
+
+static void cgroup_update_stats(void)
+{
+	struct cgroup_root *root;
+
+	for_each_root(root) {
+		struct cgroup *cgrp;
+
+		if (root == &cgrp_dfl_root && !cgrp_dfl_visible)
+			continue;
+
+		cgrp = task_cgroup_from_root(current, root);
+
+		if (cgroup_on_dfl(cgrp))
+			cgroup_update_stats_from_task(cgrp, current);
+	}
+}
+
 /**
  * cgroupstats_build - build and fill cgroupstats
  * @stats: cgroupstats to fill information into
@@ -4672,6 +4722,7 @@ int cgroupstats_build(struct cgroupstats *stats, struct dentry *dentry)
 	struct cgroup *cgrp;
 	struct css_task_iter it;
 	struct task_struct *tsk;
+	int i;
 
 	/* it should be kernfs_node belonging to cgroupfs and is a directory */
 	if (dentry->d_sb->s_type != &cgroup_fs_type || !kn ||
@@ -4714,9 +4765,13 @@ int cgroupstats_build(struct cgroupstats *stats, struct dentry *dentry)
 				stats->nr_io_wait++;
 			break;
 		}
+		cgroup_update_stats_from_task(cgrp, tsk);
 	}
 	css_task_iter_end(&it);
 
+	for (i = 0; i < RLIM_NLIMITS; i++)
+		stats->resource_hiwater[i] = cgrp->stats.resource_hiwater[i];
+
 	mutex_unlock(&cgroup_mutex);
 	return 0;
 }
-- 
2.8.1

[toc] | [next] | [standalone]

#1444263 — Re: [PATCH 02/14] resource limits: aggregate task highwater marks to cgroup level

[Multipart message — attachments visible in raw view] — view raw

Hi,

[auto build test ERROR on v4.7-rc7]
[also build test ERROR on next-20160715]
[cannot apply to tip/sched/core rdma/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Topi-Miettinen/Present-useful-limits-to-user-v2/20160715-194333
config: i386-randconfig-s1-201628 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   kernel/cgroup.c: In function 'cgroup_update_stats_from_task':
>> kernel/cgroup.c:4681:12: error: 'struct cgroup' has no member named 'stats'
       if (cgrp->stats.resource_hiwater[i] <
               ^~
>> kernel/cgroup.c:4682:11: error: 'struct signal_struct' has no member named 'resource_highwatermark'
           sig->resource_highwatermark[i])
              ^~
   kernel/cgroup.c:4683:9: error: 'struct cgroup' has no member named 'stats'
        cgrp->stats.resource_hiwater[i] =
            ^~
   kernel/cgroup.c:4684:9: error: 'struct signal_struct' has no member named 'resource_highwatermark'
         sig->resource_highwatermark[i];
            ^~
   kernel/cgroup.c: In function 'cgroupstats_build':
   kernel/cgroup.c:4773:36: error: 'struct cgroup' has no member named 'stats'
      stats->resource_hiwater[i] = cgrp->stats.resource_hiwater[i];
                                       ^~

vim +4681 kernel/cgroup.c

  4675		/* Attempt a lockless read on the first round. */
  4676		nextseq = 0;
  4677		do {
  4678			seq = nextseq;
  4679			flags = read_seqbegin_or_lock_irqsave(&sig->stats_lock, &seq);
  4680			for (i = 0; i < RLIM_NLIMITS; i++)
> 4681				if (cgrp->stats.resource_hiwater[i] <
> 4682				    sig->resource_highwatermark[i])
  4683					cgrp->stats.resource_hiwater[i] =
  4684						sig->resource_highwatermark[i];
  4685	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1444341 — Re: [PATCH 02/14] resource limits: aggregate task highwater marks to cgroup level

Hello, Topi.

On Fri, Jul 15, 2016 at 01:35:49PM +0300, Topi Miettinen wrote:
> Collect resource usage highwater marks of a task to cgroup
> statistics when the task exits.

I'm not sure how this makes sense.  The limits are enforced and
collected per user or along the process hierarchy which can be very
different from cgroup organization.  What does collecting high
watermarks from orthogonal structure, sometimes even combining
per-user numbers from different users, even mean?  These are numbers
without clear semantics.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1444463 — Re: [PATCH 02/14] resource limits: aggregate task highwater marks to cgroup level

On 07/15/16 14:10, Tejun Heo wrote:
> Hello, Topi.
> 
> On Fri, Jul 15, 2016 at 01:35:49PM +0300, Topi Miettinen wrote:
>> Collect resource usage highwater marks of a task to cgroup
>> statistics when the task exits.
> 
> I'm not sure how this makes sense.  The limits are enforced and
> collected per user or along the process hierarchy which can be very
> different from cgroup organization.  What does collecting high
> watermarks from orthogonal structure, sometimes even combining
> per-user numbers from different users, even mean?  These are numbers
> without clear semantics.

There are clear semantics for the limits themselves, either they apply
per task or per user. It makes sense to gather values according to these
semantics. Then with systemd or other tools you can use the valuse to
set the limits for a service regardless if the limit applies per task or
per user and it works according to each limit's semantics.

cgroups are used to aggregate values from a group of tasks, which still
are related to one service. Because with systemd the services also are
given a cgroup context, the values will completely make sense there too.

It could be useful to introduce a new set of limits that apply only
cgroup level. It would not remove the need to aggregate values from a
group of tasks.

-Topi

> 
> Thanks.
> 

[toc] | [prev] | [next] | [standalone]

#1445893 — Re: [PATCH 02/14] resource limits: aggregate task highwater marks to cgroup level

On Fri, Jul 15, 2016 at 05:15:41PM +0000, Topi Miettinen wrote:
> There are clear semantics for the limits themselves, either they apply
> per task or per user. It makes sense to gather values according to these
> semantics. Then with systemd or other tools you can use the valuse to
> set the limits for a service regardless if the limit applies per task or
> per user and it works according to each limit's semantics.

What does it mean to collect the maximum of the high watermarks of
multiple users or the high water marks along process hierarchy which
is spread across multiple cgroups?  These are non-sensical numbers.
If you want to collect high watermarks per-cgroup, the numbers have to
be per-cgroup - how many fds are being used in that particular cgroup
and what's the high watermark of that number and so on.  You can't
just take maximum from process hierarchy or user watermarks.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1446603 — Re: [PATCH 02/14] resource limits: aggregate task highwater marks to cgroup level

On 07/18/16 22:52, Tejun Heo wrote:
> On Fri, Jul 15, 2016 at 05:15:41PM +0000, Topi Miettinen wrote:
>> There are clear semantics for the limits themselves, either they apply
>> per task or per user. It makes sense to gather values according to these
>> semantics. Then with systemd or other tools you can use the valuse to
>> set the limits for a service regardless if the limit applies per task or
>> per user and it works according to each limit's semantics.
> 
> What does it mean to collect the maximum of the high watermarks of
> multiple users or the high water marks along process hierarchy which
> is spread across multiple cgroups?  These are non-sensical numbers.
> If you want to collect high watermarks per-cgroup, the numbers have to
> be per-cgroup - how many fds are being used in that particular cgroup
> and what's the high watermark of that number and so on.  You can't
> just take maximum from process hierarchy or user watermarks.

Then there would need to be new limit checks at cgroup level. Would you
see problems with that approach?

-Topi

[toc] | [prev] | [next] | [standalone]

#1446638 — Re: [PATCH 02/14] resource limits: aggregate task highwater marks to cgroup level

Hello, Topi.

On Tue, Jul 19, 2016 at 04:57:10PM +0000, Topi Miettinen wrote:
> Then there would need to be new limit checks at cgroup level. Would you
> see problems with that approach?

I'm worried that you're rushing this feature without thinking through
it.  You were mixing up completely orthogonal planes of accounting and
control without too much thought and are now suggesting something
which is also strange.  What do you mean by "new limit checks at
cgroup level"?  How would this be different from the resource
accounting and control implemented in the existing controllers?

Please take a step back and think through the overall design before
proposing changes to userland visible interface.

Thanks.

-- 
tejun

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web