From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934301AbbDWAnR (ORCPT ); Wed, 22 Apr 2015 20:43:17 -0400 Received: from mail-ig0-f175.google.com ([209.85.213.175]:36206 "EHLO mail-ig0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934164AbbDWAnN (ORCPT ); Wed, 22 Apr 2015 20:43:13 -0400 MIME-Version: 1.0 X-Originating-IP: [122.106.150.15] In-Reply-To: <20150422162954.GF10738@htj.duckdns.org> References: <1429446154-10660-1-git-send-email-cyphar@cyphar.com> <1429446154-10660-5-git-send-email-cyphar@cyphar.com> <20150422162954.GF10738@htj.duckdns.org> Date: Thu, 23 Apr 2015 10:43:12 +1000 Message-ID: Subject: Re: [PATCH v10 4/4] cgroups: implement the PIDs subsystem From: Aleksa Sarai To: Tejun Heo Cc: lizefan@huawei.com, mingo@redhat.com, peterz@infradead.org, richard@nod.at, =?UTF-8?B?RnLDqWTDqXJpYyBXZWlzYmVja2Vy?= , linux-kernel@vger.kernel.org, cgroups@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Tejun, >> + rcu_read_lock(); >> + css = task_css(current, pids_cgrp_id); >> + if (!css_tryget_online(css)) { >> + retval = -EBUSY; >> + goto err_rcu_unlock; >> + } >> + rcu_read_unlock(); > > Hmmm... so, the above is guaranteed to succeed in finite amount of > time (the race window is actually very narrow) and it'd be silly to > fail fork because a task was being moved across cgroups. > > I think it'd be a good idea to implement task_get_css() which loops > and returns the current css for the requested subsystem with reference > count bumped and it can use css_tryget() too. Holding a ref doesn't > prevent css from dying anyway, so it doesn't make any difference. Hmmm, okay. I'll work on this later. >> + rcu_read_lock(); >> + css = task_css(task, pids_cgrp_id); >> + css_get(css); > > Why is this safe? What guarantees that css's ref isn't already zero > at this point? Because it's already been exposed by pids_fork, so the current css_set (which contains the current css)'s ref has been bumped. There isn't a guarantee that there is a ref to css, but there is a guarantee the css_set it is in has a ref. The problem with using tryget is that we can't fail here. >> +static ssize_t pids_max_write(struct kernfs_open_file *of, char *buf, >> + size_t nbytes, loff_t off) >> +{ >> + struct cgroup_subsys_state *css = of_css(of); >> + struct pids_cgroup *pids = css_pids(css); >> + int64_t limit; >> + int err; >> + >> + buf = strstrip(buf); >> + if (!strcmp(buf, PIDS_MAX_STR)) { >> + limit = PIDS_MAX; >> + goto set_limit; >> + } >> + >> + err = kstrtoll(buf, 0, &limit); >> + if (err) >> + return err; >> + >> + /* We use INT_MAX as the maximum value of pid_t. */ >> + if (limit < 0 || limit > INT_MAX) > > This is kinda weird if we're using PIDS_MAX for max as it may end up > showing "max" after some larger number is written to the file. The reason for this is because I believe you said "PIDS_MAX isn't meant to be exposed to userspace" (one of the previous patchsets used PIDS_MAX as the maximum valid value). -- Aleksa Sarai (cyphar) www.cyphar.com