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
