| Message ID | 1465847065-3577-6-git-send-email-toiwoton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/13/2016 12:44 PM, Topi Miettinen wrote: > Track maximum number of files for the process, present current maximum > in /proc/self/limits. The core part should be its own patch. Also, you have this weirdly named (and racy!) function bump_rlimit. Wouldn't this be nicer if you taught the rlimit code to track the *current* usage generically and to derive the max usage from that? > diff --git a/fs/proc/base.c b/fs/proc/base.c > index a11eb71..227997b 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -630,8 +630,8 @@ static int proc_pid_limits(struct seq_file *m, struct pid_namespace *ns, > /* > * print the file header > */ > - seq_printf(m, "%-25s %-20s %-20s %-10s\n", > - "Limit", "Soft Limit", "Hard Limit", "Units"); > + seq_printf(m, "%-25s %-20s %-20s %-10s %-20s\n", > + "Limit", "Soft Limit", "Hard Limit", "Units", "Max"); What existing programs, if any, does this break? > > for (i = 0; i < RLIM_NLIMITS; i++) { > if (rlim[i].rlim_cur == RLIM_INFINITY) > @@ -647,9 +647,11 @@ static int proc_pid_limits(struct seq_file *m, struct pid_namespace *ns, > seq_printf(m, "%-20lu ", rlim[i].rlim_max); > > if (lnames[i].unit) > - seq_printf(m, "%-10s\n", lnames[i].unit); > + seq_printf(m, "%-10s", lnames[i].unit); > else > - seq_putc(m, '\n'); > + seq_printf(m, "%-10s", ""); > + seq_printf(m, "%-20lu\n", > + task->signal->rlim_curmax[i]); > } > > return 0; > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 9c48a08..0150380 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -782,6 +782,7 @@ struct signal_struct { > * have no need to disable irqs. > */ > struct rlimit rlim[RLIM_NLIMITS]; > + unsigned long rlim_curmax[RLIM_NLIMITS]; > > #ifdef CONFIG_BSD_PROCESS_ACCT > struct pacct_struct pacct; /* per-process accounting information */ > @@ -3376,6 +3377,12 @@ static inline unsigned long rlimit_max(unsigned int limit) > return task_rlimit_max(current, limit); > } > > +static inline void bump_rlimit(unsigned int limit, unsigned long r) > +{ > + if (READ_ONCE(current->signal->rlim_curmax[limit]) < r) > + current->signal->rlim_curmax[limit] = r; > +} > + > #ifdef CONFIG_CPU_FREQ > struct update_util_data { > void (*func)(struct update_util_data *data, > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/13/16 20:40, Andy Lutomirski wrote: > On 06/13/2016 12:44 PM, Topi Miettinen wrote: >> Track maximum number of files for the process, present current maximum >> in /proc/self/limits. > > The core part should be its own patch. > > Also, you have this weirdly named (and racy!) function bump_rlimit. I can change the name if you have better suggestions. rlimit_track_max? The max value is written often but read seldom, if ever. What kind of locking should I use then? > Wouldn't this be nicer if you taught the rlimit code to track the > *current* usage generically and to derive the max usage from that? Current rlimit code performs checks against current limits. These are typically done early in the calling function and further checks could also fail. Thus max should not be updated until much later. Maybe these could be combined, but not easily if at all. > >> diff --git a/fs/proc/base.c b/fs/proc/base.c >> index a11eb71..227997b 100644 >> --- a/fs/proc/base.c >> +++ b/fs/proc/base.c >> @@ -630,8 +630,8 @@ static int proc_pid_limits(struct seq_file *m, >> struct pid_namespace *ns, >> /* >> * print the file header >> */ >> - seq_printf(m, "%-25s %-20s %-20s %-10s\n", >> - "Limit", "Soft Limit", "Hard Limit", "Units"); >> + seq_printf(m, "%-25s %-20s %-20s %-10s %-20s\n", >> + "Limit", "Soft Limit", "Hard Limit", "Units", "Max"); > > What existing programs, if any, does this break? Using Debian codesearch for /limits" string, I'd check pam_limits and rtkit. The max values could be put into a new file if you prefer. > >> >> for (i = 0; i < RLIM_NLIMITS; i++) { >> if (rlim[i].rlim_cur == RLIM_INFINITY) >> @@ -647,9 +647,11 @@ static int proc_pid_limits(struct seq_file *m, >> struct pid_namespace *ns, >> seq_printf(m, "%-20lu ", rlim[i].rlim_max); >> >> if (lnames[i].unit) >> - seq_printf(m, "%-10s\n", lnames[i].unit); >> + seq_printf(m, "%-10s", lnames[i].unit); >> else >> - seq_putc(m, '\n'); >> + seq_printf(m, "%-10s", ""); >> + seq_printf(m, "%-20lu\n", >> + task->signal->rlim_curmax[i]); >> } >> >> return 0; >> diff --git a/include/linux/sched.h b/include/linux/sched.h >> index 9c48a08..0150380 100644 >> --- a/include/linux/sched.h >> +++ b/include/linux/sched.h >> @@ -782,6 +782,7 @@ struct signal_struct { >> * have no need to disable irqs. >> */ >> struct rlimit rlim[RLIM_NLIMITS]; >> + unsigned long rlim_curmax[RLIM_NLIMITS]; >> >> #ifdef CONFIG_BSD_PROCESS_ACCT >> struct pacct_struct pacct; /* per-process accounting >> information */ >> @@ -3376,6 +3377,12 @@ static inline unsigned long rlimit_max(unsigned >> int limit) >> return task_rlimit_max(current, limit); >> } >> >> +static inline void bump_rlimit(unsigned int limit, unsigned long r) >> +{ >> + if (READ_ONCE(current->signal->rlim_curmax[limit]) < r) >> + current->signal->rlim_curmax[limit] = r; >> +} >> + >> #ifdef CONFIG_CPU_FREQ >> struct update_util_data { >> void (*func)(struct update_util_data *data, >> > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 13, 2016 at 2:13 PM, Topi Miettinen <toiwoton@gmail.com> wrote: > On 06/13/16 20:40, Andy Lutomirski wrote: >> On 06/13/2016 12:44 PM, Topi Miettinen wrote: >>> Track maximum number of files for the process, present current maximum >>> in /proc/self/limits. >> >> The core part should be its own patch. >> >> Also, you have this weirdly named (and racy!) function bump_rlimit. > > I can change the name if you have better suggestions. rlimit_track_max? > > The max value is written often but read seldom, if ever. What kind of > locking should I use then? Possibly none, but WRITE_ONCE would be good as would a comment indicating that your code in intentionally racy. Or you could use atomic_cmpxchg if that won't kill performance. rlimit_track_max sounds like a better name to me. > >> Wouldn't this be nicer if you taught the rlimit code to track the >> *current* usage generically and to derive the max usage from that? > > Current rlimit code performs checks against current limits. These are > typically done early in the calling function and further checks could > also fail. Thus max should not be updated until much later. Maybe these > could be combined, but not easily if at all. I mean: why not actually show the current value in /proc/pid/limits and track the max via whatever teaches proc about the current value? > >> >>> diff --git a/fs/proc/base.c b/fs/proc/base.c >>> index a11eb71..227997b 100644 >>> --- a/fs/proc/base.c >>> +++ b/fs/proc/base.c >>> @@ -630,8 +630,8 @@ static int proc_pid_limits(struct seq_file *m, >>> struct pid_namespace *ns, >>> /* >>> * print the file header >>> */ >>> - seq_printf(m, "%-25s %-20s %-20s %-10s\n", >>> - "Limit", "Soft Limit", "Hard Limit", "Units"); >>> + seq_printf(m, "%-25s %-20s %-20s %-10s %-20s\n", >>> + "Limit", "Soft Limit", "Hard Limit", "Units", "Max"); >> >> What existing programs, if any, does this break? > > Using Debian codesearch for /limits" string, I'd check pam_limits and > rtkit. The max values could be put into a new file if you prefer. If it actually breaks them, then you need to change the patch so you don't break them. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/13/16 21:16, Andy Lutomirski wrote: > On Mon, Jun 13, 2016 at 2:13 PM, Topi Miettinen <toiwoton@gmail.com> wrote: >> On 06/13/16 20:40, Andy Lutomirski wrote: >>> On 06/13/2016 12:44 PM, Topi Miettinen wrote: >>>> Track maximum number of files for the process, present current maximum >>>> in /proc/self/limits. >>> >>> The core part should be its own patch. >>> >>> Also, you have this weirdly named (and racy!) function bump_rlimit. >> >> I can change the name if you have better suggestions. rlimit_track_max? >> >> The max value is written often but read seldom, if ever. What kind of >> locking should I use then? > > Possibly none, but WRITE_ONCE would be good as would a comment > indicating that your code in intentionally racy. Or you could use > atomic_cmpxchg if that won't kill performance. > > rlimit_track_max sounds like a better name to me. > >> >>> Wouldn't this be nicer if you taught the rlimit code to track the >>> *current* usage generically and to derive the max usage from that? >> >> Current rlimit code performs checks against current limits. These are >> typically done early in the calling function and further checks could >> also fail. Thus max should not be updated until much later. Maybe these >> could be combined, but not easily if at all. > > I mean: why not actually show the current value in /proc/pid/limits > and track the max via whatever teaches proc about the current value? > That could be interesting data too. In other comments, a new file was proposed and then your model would be good choice. >> >>> >>>> diff --git a/fs/proc/base.c b/fs/proc/base.c >>>> index a11eb71..227997b 100644 >>>> --- a/fs/proc/base.c >>>> +++ b/fs/proc/base.c >>>> @@ -630,8 +630,8 @@ static int proc_pid_limits(struct seq_file *m, >>>> struct pid_namespace *ns, >>>> /* >>>> * print the file header >>>> */ >>>> - seq_printf(m, "%-25s %-20s %-20s %-10s\n", >>>> - "Limit", "Soft Limit", "Hard Limit", "Units"); >>>> + seq_printf(m, "%-25s %-20s %-20s %-10s %-20s\n", >>>> + "Limit", "Soft Limit", "Hard Limit", "Units", "Max"); >>> >>> What existing programs, if any, does this break? >> >> Using Debian codesearch for /limits" string, I'd check pam_limits and >> rtkit. The max values could be put into a new file if you prefer. > > If it actually breaks them, then you need to change the patch so you > don't break them. > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/file.c b/fs/file.c index 6b1acdf..2d0d206 100644 --- a/fs/file.c +++ b/fs/file.c @@ -547,6 +547,8 @@ repeat: } #endif + bump_rlimit(RLIMIT_NOFILE, fd); + out: spin_unlock(&files->file_lock); return error; @@ -857,6 +859,8 @@ __releases(&files->file_lock) if (tofree) filp_close(tofree, files); + bump_rlimit(RLIMIT_NOFILE, fd); + return fd; Ebusy: diff --git a/fs/proc/base.c b/fs/proc/base.c index a11eb71..227997b 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -630,8 +630,8 @@ static int proc_pid_limits(struct seq_file *m, struct pid_namespace *ns, /* * print the file header */ - seq_printf(m, "%-25s %-20s %-20s %-10s\n", - "Limit", "Soft Limit", "Hard Limit", "Units"); + seq_printf(m, "%-25s %-20s %-20s %-10s %-20s\n", + "Limit", "Soft Limit", "Hard Limit", "Units", "Max"); for (i = 0; i < RLIM_NLIMITS; i++) { if (rlim[i].rlim_cur == RLIM_INFINITY) @@ -647,9 +647,11 @@ static int proc_pid_limits(struct seq_file *m, struct pid_namespace *ns, seq_printf(m, "%-20lu ", rlim[i].rlim_max); if (lnames[i].unit) - seq_printf(m, "%-10s\n", lnames[i].unit); + seq_printf(m, "%-10s", lnames[i].unit); else - seq_putc(m, '\n'); + seq_printf(m, "%-10s", ""); + seq_printf(m, "%-20lu\n", + task->signal->rlim_curmax[i]); } return 0; diff --git a/include/linux/sched.h b/include/linux/sched.h index 9c48a08..0150380 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -782,6 +782,7 @@ struct signal_struct { * have no need to disable irqs. */ struct rlimit rlim[RLIM_NLIMITS]; + unsigned long rlim_curmax[RLIM_NLIMITS]; #ifdef CONFIG_BSD_PROCESS_ACCT struct pacct_struct pacct; /* per-process accounting information */ @@ -3376,6 +3377,12 @@ static inline unsigned long rlimit_max(unsigned int limit) return task_rlimit_max(current, limit); } +static inline void bump_rlimit(unsigned int limit, unsigned long r) +{ + if (READ_ONCE(current->signal->rlim_curmax[limit]) < r) + current->signal->rlim_curmax[limit] = r; +} + #ifdef CONFIG_CPU_FREQ struct update_util_data { void (*func)(struct update_util_data *data,
Track maximum number of files for the process, present current maximum in /proc/self/limits. Signed-off-by: Topi Miettinen <toiwoton@gmail.com> --- fs/file.c | 4 ++++ fs/proc/base.c | 10 ++++++---- include/linux/sched.h | 7 +++++++ 3 files changed, 17 insertions(+), 4 deletions(-)