linux-kernel - Re: [RFC 02/18] cgroup_pids: track maximum pids

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <17cb1a37-47b1-dbd4-6835-efad3cf6c12f@gmail.com>
Date:	Mon, 13 Jun 2016 21:29:32 +0000
From:	Topi Miettinen <toiwoton@...il.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	linux-kernel@...r.kernel.org, Li Zefan <lizefan@...wei.com>,
	Johannes Weiner <hannes@...xchg.org>,
	"open list:CONTROL GROUP (CGROUP)" <cgroups@...r.kernel.org>
Subject: 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