Re: [RFC 02/18] cgroup_pids: track maximum pids
On Tue, Jul 19, 2016 at 04:59:18PM +, Topi Miettinen wrote: > With the example systemd-timesyncd case, I was only getting 1 as the > highwatermark, but there were already two tasks. Can you please find out why that is so? Given that that's where we charge pid usage, it doesn't make sense to me that you're getting lower numbers than actual usage there. Thanks. -- tejun
Re: [RFC 02/18] cgroup_pids: track maximum pids
On 07/19/16 01:09, Tejun Heo wrote: > On Sun, Jul 17, 2016 at 08:11:31PM +, Topi Miettinen wrote: >> On 06/13/16 21:33, Tejun Heo wrote: >>> Hello, >>> >>> On Mon, Jun 13, 2016 at 09:29:32PM +, Topi Miettinen wrote: I used fork callback as I don't want to lower the watermark in all cases where the charge can be lowered, so I'd update the watermark only when the fork really happens. >>> >>> I don't think that would make a noticeable difference. That's where >>> we decide whether to grant fork or not after all and thus where the >>> actual usage is. >> >> I tried using only charge functions, but then the result was too low. >> With fork callback, the result was as expected. > > Can you please elaborate in more details? With the example systemd-timesyncd case, I was only getting 1 as the highwatermark, but there were already two tasks. -Topi
Re: [RFC 02/18] cgroup_pids: track maximum pids
On Sun, Jul 17, 2016 at 08:11:31PM +, Topi Miettinen wrote: > On 06/13/16 21:33, Tejun Heo wrote: > > Hello, > > > > On Mon, Jun 13, 2016 at 09:29:32PM +, Topi Miettinen wrote: > >> I used fork callback as I don't want to lower the watermark in all cases > >> where the charge can be lowered, so I'd update the watermark only when > >> the fork really happens. > > > > I don't think that would make a noticeable difference. That's where > > we decide whether to grant fork or not after all and thus where the > > actual usage is. > > I tried using only charge functions, but then the result was too low. > With fork callback, the result was as expected. Can you please elaborate in more details? Thanks. -- tejun
Re: [RFC 02/18] cgroup_pids: track maximum pids
On 06/13/16 21:33, Tejun Heo wrote: > Hello, > > On Mon, Jun 13, 2016 at 09:29:32PM +, Topi Miettinen wrote: >> I used fork callback as I don't want to lower the watermark in all cases >> where the charge can be lowered, so I'd update the watermark only when >> the fork really happens. > > I don't think that would make a noticeable difference. That's where > we decide whether to grant fork or not after all and thus where the > actual usage is. I tried using only charge functions, but then the result was too low. With fork callback, the result was as expected. -Topi > >> Is there a better way to compare and set? I don't think atomic_cmpxchg() >> does what's needed, > > cmpxchg loop should do what's necessary although I'm not sure how much > being strictly correct matters here. > > Thanks. >
Re: [RFC 02/18] cgroup_pids: track maximum pids
On Mon, Jun 13, 2016 at 09:59:32PM +, Topi Miettinen wrote: > On 06/13/16 21:33, Tejun Heo wrote: > > Hello, > > > > On Mon, Jun 13, 2016 at 09:29:32PM +, Topi Miettinen wrote: > >> I used fork callback as I don't want to lower the watermark in all cases > >> where the charge can be lowered, so I'd update the watermark only when > >> the fork really happens. > > > > I don't think that would make a noticeable difference. That's where > > we decide whether to grant fork or not after all and thus where the > > actual usage is. > > > > You mean, increment count on cgroup_can_fork()? But what if the fork() > fails after that (signal_pending case)? That number isn't gonna deviate by any significant amount and the counter is to estimate what the limit should be set to to begin with. It's logical to collect how close the usage got to can_attach failure due to limit breach. > >> Is there a better way to compare and set? I don't think atomic_cmpxchg() > >> does what's needed, > > > > cmpxchg loop should do what's necessary although I'm not sure how much > > being strictly correct matters here. > > These are not used for any decisions taken by kernel, but by the user. I > have to say I don't know where's the line between strict correctness and > less strict. Provided that cmpxchg is done only when the counter needs to be actually updated, it's not gonna be noticeably expensive. Might as well make it correct. Thanks. -- tejun
Re: [RFC 02/18] cgroup_pids: track maximum pids
On 06/13/16 21:33, Tejun Heo wrote: > Hello, > > On Mon, Jun 13, 2016 at 09:29:32PM +, Topi Miettinen wrote: >> I used fork callback as I don't want to lower the watermark in all cases >> where the charge can be lowered, so I'd update the watermark only when >> the fork really happens. > > I don't think that would make a noticeable difference. That's where > we decide whether to grant fork or not after all and thus where the > actual usage is. > You mean, increment count on cgroup_can_fork()? But what if the fork() fails after that (signal_pending case)? >> Is there a better way to compare and set? I don't think atomic_cmpxchg() >> does what's needed, > > cmpxchg loop should do what's necessary although I'm not sure how much > being strictly correct matters here. > > Thanks. > These are not used for any decisions taken by kernel, but by the user. I have to say I don't know where's the line between strict correctness and less strict. -Topi
Re: [RFC 02/18] cgroup_pids: track maximum pids
Hello, On Mon, Jun 13, 2016 at 09:29:32PM +, Topi Miettinen wrote: > I used fork callback as I don't want to lower the watermark in all cases > where the charge can be lowered, so I'd update the watermark only when > the fork really happens. I don't think that would make a noticeable difference. That's where we decide whether to grant fork or not after all and thus where the actual usage is. > Is there a better way to compare and set? I don't think atomic_cmpxchg() > does what's needed, cmpxchg loop should do what's necessary although I'm not sure how much being strictly correct matters here. Thanks. -- tejun
Re: [RFC 02/18] cgroup_pids: track maximum pids
On 06/13/16 21:12, Tejun Heo wrote:
> Hello,
>
> On Mon, Jun 13, 2016 at 10:44:09PM +0300, Topi Miettinen wrote:
>> Track maximum pids in the cgroup, present it in cgroup pids.current_max.
>
> "max" is often used for maximum limits in cgroup. I think "watermark"
> or "high_watermark" would be a lot clearer.
OK, I have no preference.
>
>> @@ -236,6 +246,14 @@ static void pids_free(struct task_struct *task)
>> pids_uncharge(pids, 1);
>> }
>>
>> +static void pids_fork(struct task_struct *task)
>> +{
>> +struct pids_cgroup *pids = css_pids(task_css(task, pids_cgrp_id));
>> +
>> +if (atomic64_read(&pids->cur_max) < atomic64_read(&pids->counter))
>> +atomic64_set(&pids->cur_max, atomic64_read(&pids->counter));
>> +}
>
> Wouldn't it make more sense to track high watermark from the charge
> functions instead? I don't get why this requires a separate fork
> callback. Also, racing atomic64_set's are racy. The counter can end
> up with a lower number than it should be.
>
I used fork callback as I don't want to lower the watermark in all cases
where the charge can be lowered, so I'd update the watermark only when
the fork really happens.
Is there a better way to compare and set? I don't think atomic_cmpxchg()
does what's needed,
>> @@ -300,6 +326,11 @@ static struct cftype pids_files[] = {
>> .read_s64 = pids_current_read,
>> .flags = CFTYPE_NOT_ON_ROOT,
>> },
>> +{
>> +.name = "current_max",
>
> Please make this "high_watermark" field in pids.stats file.
>
> Thanks.
>
OK.
-Topi
Re: [RFC 02/18] cgroup_pids: track maximum pids
Hello,
On Mon, Jun 13, 2016 at 10:44:09PM +0300, Topi Miettinen wrote:
> Track maximum pids in the cgroup, present it in cgroup pids.current_max.
"max" is often used for maximum limits in cgroup. I think "watermark"
or "high_watermark" would be a lot clearer.
> @@ -236,6 +246,14 @@ static void pids_free(struct task_struct *task)
> pids_uncharge(pids, 1);
> }
>
> +static void pids_fork(struct task_struct *task)
> +{
> + struct pids_cgroup *pids = css_pids(task_css(task, pids_cgrp_id));
> +
> + if (atomic64_read(&pids->cur_max) < atomic64_read(&pids->counter))
> + atomic64_set(&pids->cur_max, atomic64_read(&pids->counter));
> +}
Wouldn't it make more sense to track high watermark from the charge
functions instead? I don't get why this requires a separate fork
callback. Also, racing atomic64_set's are racy. The counter can end
up with a lower number than it should be.
> @@ -300,6 +326,11 @@ static struct cftype pids_files[] = {
> .read_s64 = pids_current_read,
> .flags = CFTYPE_NOT_ON_ROOT,
> },
> + {
> + .name = "current_max",
Please make this "high_watermark" field in pids.stats file.
Thanks.
--
tejun
