Many monitoring tools include open file count as a metric. Currently
the only way to get this number is to enumerate the files in /proc/pid/fd.
The problem with the current approach is that it does many things people
generally don't care about when they need one number for a metric.
In our tests for cadvisor, which reports open file counts per cgroup,
we observed that reading the number of open files is slow. Out of 35.23%
of CPU time spent in `proc_readfd_common`, we see 29.43% spent in
`proc_fill_cache`, which is responsible for filling dentry info.
Some of this extra time is spinlock contention, but it's a contention
for the lock we don't want to take to begin with.
We considered putting the number of open files in /proc/pid/status.
Unfortunately, counting the number of fds involves iterating the open_files
bitmap, which has a linear complexity in proportion with the number
of open files (bitmap slots really, but it's close). We don't want
to make /proc/pid/status any slower, so instead we put this info
in /proc/pid/fd as a size member of the stat syscall result.
Previously the reported number was zero, so there's very little
risk of breaking anything, while still providing a somewhat logical
way to count the open files with a fallback if it's zero.
RFC for this patch included iterating open fds under RCU. Thanks
to Frank Hofmann for the suggestion to use the bitmap instead.
Previously:
```
$ sudo stat /proc/1/fd | head -n2
File: /proc/1/fd
Size: 0 Blocks: 0 IO Block: 1024 directory
```
With this patch:
```
$ sudo stat /proc/1/fd | head -n2
File: /proc/1/fd
Size: 65 Blocks: 0 IO Block: 1024 directory
```
Correctness check:
```
$ sudo ls /proc/1/fd | wc -l
65
```
I added the docs for /proc/<pid>/fd while I'm at it.
Signed-off-by: Ivan Babrou <[email protected]>
---
v2: Added missing rcu_read_lock() / rcu_read_unlock(),
task_lock() / task_unlock() and put_task_struct().
---
Documentation/filesystems/proc.rst | 17 ++++++++++++
fs/proc/fd.c | 44 ++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
index e7aafc82be99..394548d26187 100644
--- a/Documentation/filesystems/proc.rst
+++ b/Documentation/filesystems/proc.rst
@@ -47,6 +47,7 @@ fixes/update part 1.1 Stefani Seibold <[email protected]> June 9 2009
3.10 /proc/<pid>/timerslack_ns - Task timerslack value
3.11 /proc/<pid>/patch_state - Livepatch patch operation state
3.12 /proc/<pid>/arch_status - Task architecture specific information
+ 3.13 /proc/<pid>/fd - List of symlinks to open files
4 Configuring procfs
4.1 Mount options
@@ -2145,6 +2146,22 @@ AVX512_elapsed_ms
the task is unlikely an AVX512 user, but depends on the workload and the
scheduling scenario, it also could be a false negative mentioned above.
+3.13 /proc/<pid>/fd - List of symlinks to open files
+-------------------------------------------------------
+This directory contains symbolic links which represent open files
+the process is maintaining. Example output::
+
+ lr-x------ 1 root root 64 Sep 20 17:53 0 -> /dev/null
+ l-wx------ 1 root root 64 Sep 20 17:53 1 -> /dev/null
+ lrwx------ 1 root root 64 Sep 20 17:53 10 -> 'socket:[12539]'
+ lrwx------ 1 root root 64 Sep 20 17:53 11 -> 'socket:[12540]'
+ lrwx------ 1 root root 64 Sep 20 17:53 12 -> 'socket:[12542]'
+
+The number of open files for the process is stored in 'size' member
+of stat() output for /proc/<pid>/fd for fast access.
+-------------------------------------------------------
+
+
Chapter 4: Configuring procfs
=============================
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 913bef0d2a36..ff526dfc5faa 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -279,6 +279,34 @@ static int proc_readfd_common(struct file *file, struct dir_context *ctx,
return 0;
}
+static int proc_readfd_count(struct inode *inode)
+{
+ struct task_struct *p = get_proc_task(inode);
+ struct fdtable *fdt;
+ unsigned int i, size, open_fds = 0;
+
+ if (!p)
+ return -ENOENT;
+
+ task_lock(p);
+ if (p->files) {
+ rcu_read_lock();
+
+ fdt = files_fdtable(p->files);
+ size = fdt->max_fds;
+
+ for (i = size / BITS_PER_LONG; i > 0;)
+ open_fds += hweight64(fdt->open_fds[--i]);
+
+ rcu_read_unlock();
+ }
+ task_unlock(p);
+
+ put_task_struct(p);
+
+ return open_fds;
+}
+
static int proc_readfd(struct file *file, struct dir_context *ctx)
{
return proc_readfd_common(file, ctx, proc_fd_instantiate);
@@ -319,9 +347,25 @@ int proc_fd_permission(struct user_namespace *mnt_userns,
return rv;
}
+static int proc_fd_getattr(struct user_namespace *mnt_userns,
+ const struct path *path, struct kstat *stat,
+ u32 request_mask, unsigned int query_flags)
+{
+ struct inode *inode = d_inode(path->dentry);
+
+ generic_fillattr(&init_user_ns, inode, stat);
+
+ /* If it's a directory, put the number of open fds there */
+ if (S_ISDIR(inode->i_mode))
+ stat->size = proc_readfd_count(inode);
+
+ return 0;
+}
+
const struct inode_operations proc_fd_inode_operations = {
.lookup = proc_lookupfd,
.permission = proc_fd_permission,
+ .getattr = proc_fd_getattr,
.setattr = proc_setattr,
};
--
2.37.2
On Thu, Sep 22, 2022 at 3:40 PM Ivan Babrou <[email protected]> wrote:
>
> Many monitoring tools include open file count as a metric. Currently
> the only way to get this number is to enumerate the files in /proc/pid/fd.
>
> The problem with the current approach is that it does many things people
> generally don't care about when they need one number for a metric.
> In our tests for cadvisor, which reports open file counts per cgroup,
> we observed that reading the number of open files is slow. Out of 35.23%
> of CPU time spent in `proc_readfd_common`, we see 29.43% spent in
> `proc_fill_cache`, which is responsible for filling dentry info.
> Some of this extra time is spinlock contention, but it's a contention
> for the lock we don't want to take to begin with.
>
> We considered putting the number of open files in /proc/pid/status.
> Unfortunately, counting the number of fds involves iterating the open_files
> bitmap, which has a linear complexity in proportion with the number
> of open files (bitmap slots really, but it's close). We don't want
> to make /proc/pid/status any slower, so instead we put this info
> in /proc/pid/fd as a size member of the stat syscall result.
> Previously the reported number was zero, so there's very little
> risk of breaking anything, while still providing a somewhat logical
> way to count the open files with a fallback if it's zero.
>
> RFC for this patch included iterating open fds under RCU. Thanks
> to Frank Hofmann for the suggestion to use the bitmap instead.
>
> Previously:
>
> ```
> $ sudo stat /proc/1/fd | head -n2
> File: /proc/1/fd
> Size: 0 Blocks: 0 IO Block: 1024 directory
> ```
>
> With this patch:
>
> ```
> $ sudo stat /proc/1/fd | head -n2
> File: /proc/1/fd
> Size: 65 Blocks: 0 IO Block: 1024 directory
> ```
>
> Correctness check:
>
> ```
> $ sudo ls /proc/1/fd | wc -l
> 65
> ```
>
> I added the docs for /proc/<pid>/fd while I'm at it.
>
> Signed-off-by: Ivan Babrou <[email protected]>
>
> ---
> v2: Added missing rcu_read_lock() / rcu_read_unlock(),
> task_lock() / task_unlock() and put_task_struct().
> ---
> Documentation/filesystems/proc.rst | 17 ++++++++++++
> fs/proc/fd.c | 44 ++++++++++++++++++++++++++++++
> 2 files changed, 61 insertions(+)
Now that Linux 6.1-rc1 is out, should this patch be looked at for
inclusion? I see that the net-next tree has opened, not sure if the
same rules apply here.
We've been running the v2 version of this patch in production
successfully for some time now.
On Thu, 22 Sep 2022 15:40:26 -0700 Ivan Babrou <[email protected]> wrote:
> Many monitoring tools include open file count as a metric. Currently
> the only way to get this number is to enumerate the files in /proc/pid/fd.
>
> The problem with the current approach is that it does many things people
> generally don't care about when they need one number for a metric.
> In our tests for cadvisor, which reports open file counts per cgroup,
> we observed that reading the number of open files is slow. Out of 35.23%
> of CPU time spent in `proc_readfd_common`, we see 29.43% spent in
> `proc_fill_cache`, which is responsible for filling dentry info.
> Some of this extra time is spinlock contention, but it's a contention
> for the lock we don't want to take to begin with.
>
> We considered putting the number of open files in /proc/pid/status.
> Unfortunately, counting the number of fds involves iterating the open_files
> bitmap, which has a linear complexity in proportion with the number
> of open files (bitmap slots really, but it's close). We don't want
> to make /proc/pid/status any slower, so instead we put this info
> in /proc/pid/fd as a size member of the stat syscall result.
That sounds awfully logical.
> Previously the reported number was zero, so there's very little
> risk of breaking anything, while still providing a somewhat logical
> way to count the open files with a fallback if it's zero.
>
> RFC for this patch included iterating open fds under RCU. Thanks
> to Frank Hofmann for the suggestion to use the bitmap instead.
>
> Previously:
>
> ```
> $ sudo stat /proc/1/fd | head -n2
> File: /proc/1/fd
> Size: 0 Blocks: 0 IO Block: 1024 directory
> ```
>
> With this patch:
>
> ```
> $ sudo stat /proc/1/fd | head -n2
> File: /proc/1/fd
> Size: 65 Blocks: 0 IO Block: 1024 directory
> ```
>
> Correctness check:
>
> ```
> $ sudo ls /proc/1/fd | wc -l
> 65
> ```
>
> I added the docs for /proc/<pid>/fd while I'm at it.
>
Dang that's a good changelog.
> index e7aafc82be99..394548d26187 100644
> --- a/Documentation/filesystems/proc.rst
> +++ b/Documentation/filesystems/proc.rst
> @@ -47,6 +47,7 @@ fixes/update part 1.1 Stefani Seibold <[email protected]> June 9 2009
> 3.10 /proc/<pid>/timerslack_ns - Task timerslack value
> 3.11 /proc/<pid>/patch_state - Livepatch patch operation state
> 3.12 /proc/<pid>/arch_status - Task architecture specific information
> + 3.13 /proc/<pid>/fd - List of symlinks to open files
>
> 4 Configuring procfs
> 4.1 Mount options
> @@ -2145,6 +2146,22 @@ AVX512_elapsed_ms
> the task is unlikely an AVX512 user, but depends on the workload and the
> scheduling scenario, it also could be a false negative mentioned above.
>
> +3.13 /proc/<pid>/fd - List of symlinks to open files
> +-------------------------------------------------------
> +This directory contains symbolic links which represent open files
> +the process is maintaining. Example output::
> +
> + lr-x------ 1 root root 64 Sep 20 17:53 0 -> /dev/null
> + l-wx------ 1 root root 64 Sep 20 17:53 1 -> /dev/null
> + lrwx------ 1 root root 64 Sep 20 17:53 10 -> 'socket:[12539]'
> + lrwx------ 1 root root 64 Sep 20 17:53 11 -> 'socket:[12540]'
> + lrwx------ 1 root root 64 Sep 20 17:53 12 -> 'socket:[12542]'
> +
> +The number of open files for the process is stored in 'size' member
> +of stat() output for /proc/<pid>/fd for fast access.
> +-------------------------------------------------------
> +
> +
> Chapter 4: Configuring procfs
> =============================
>
> diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> index 913bef0d2a36..ff526dfc5faa 100644
> --- a/fs/proc/fd.c
> +++ b/fs/proc/fd.c
> @@ -279,6 +279,34 @@ static int proc_readfd_common(struct file *file, struct dir_context *ctx,
> return 0;
> }
>
> +static int proc_readfd_count(struct inode *inode)
> +{
> + struct task_struct *p = get_proc_task(inode);
> + struct fdtable *fdt;
> + unsigned int i, size, open_fds = 0;
> +
> + if (!p)
> + return -ENOENT;
> +
> + task_lock(p);
> + if (p->files) {
> + rcu_read_lock();
> +
> + fdt = files_fdtable(p->files);
> + size = fdt->max_fds;
> +
> + for (i = size / BITS_PER_LONG; i > 0;)
> + open_fds += hweight64(fdt->open_fds[--i]);
Could BITMAP_WEIGHT() or __bitmap_weight() or bitmap_weight() be used here?
> + rcu_read_unlock();
> + }
> + task_unlock(p);
> +
> + put_task_struct(p);
> +
> + return open_fds;
> +}
> +
> static int proc_readfd(struct file *file, struct dir_context *ctx)
> {
> return proc_readfd_common(file, ctx, proc_fd_instantiate);
> @@ -319,9 +347,25 @@ int proc_fd_permission(struct user_namespace *mnt_userns,
> return rv;
> }
>
> +static int proc_fd_getattr(struct user_namespace *mnt_userns,
> + const struct path *path, struct kstat *stat,
> + u32 request_mask, unsigned int query_flags)
> +{
> + struct inode *inode = d_inode(path->dentry);
> +
> + generic_fillattr(&init_user_ns, inode, stat);
> +
> + /* If it's a directory, put the number of open fds there */
> + if (S_ISDIR(inode->i_mode))
> + stat->size = proc_readfd_count(inode);
> +
> + return 0;
> +}
> +
> const struct inode_operations proc_fd_inode_operations = {
> .lookup = proc_lookupfd,
> .permission = proc_fd_permission,
> + .getattr = proc_fd_getattr,
> .setattr = proc_setattr,
> };
>
> --
> 2.37.2
On Mon, Oct 17, 2022 at 6:47 PM Andrew Morton <[email protected]> wrote:
> > +
> > + fdt = files_fdtable(p->files);
> > + size = fdt->max_fds;
> > +
> > + for (i = size / BITS_PER_LONG; i > 0;)
> > + open_fds += hweight64(fdt->open_fds[--i]);
>
> Could BITMAP_WEIGHT() or __bitmap_weight() or bitmap_weight() be used here?
That's a great suggestion. I tested it with bitmap_weight() and it
looks much cleaner while providing the same result.
I just sent the v3 with this suggestion applied.
On Tue, Oct 18, 2022 at 6:02 AM Ivan Babrou <[email protected]> wrote:
>
> On Mon, Oct 17, 2022 at 6:47 PM Andrew Morton <[email protected]> wrote:
> > > +
> > > + fdt = files_fdtable(p->files);
> > > + size = fdt->max_fds;
> > > +
> > > + for (i = size / BITS_PER_LONG; i > 0;)
> > > + open_fds += hweight64(fdt->open_fds[--i]);
> >
> > Could BITMAP_WEIGHT() or __bitmap_weight() or bitmap_weight() be used here?
>
> That's a great suggestion. I tested it with bitmap_weight() and it
> looks much cleaner while providing the same result.
>
> I just sent the v3 with this suggestion applied.
+1 from me on using bitmap_weight() - good spotting that.
FrankH.
On Tue, Oct 18, 2022 at 9:25 AM David Laight <[email protected]> wrote:
>
> From: Frank Hofmann
> > Sent: 18 October 2022 09:13
> >
> > On Tue, Oct 18, 2022 at 6:02 AM Ivan Babrou <[email protected]> wrote:
> > >
> > > On Mon, Oct 17, 2022 at 6:47 PM Andrew Morton <[email protected]> wrote:
> > > > > +
> > > > > + fdt = files_fdtable(p->files);
> > > > > + size = fdt->max_fds;
> > > > > +
> > > > > + for (i = size / BITS_PER_LONG; i > 0;)
> > > > > + open_fds += hweight64(fdt->open_fds[--i]);
> > > >
> > > > Could BITMAP_WEIGHT() or __bitmap_weight() or bitmap_weight() be used here?
> > >
> > > That's a great suggestion. I tested it with bitmap_weight() and it
> > > looks much cleaner while providing the same result.
> > >
> > > I just sent the v3 with this suggestion applied.
> >
> > +1 from me on using bitmap_weight() - good spotting that.
>
> Does that have the optimisations for the value being 0, ~0u
> or 2**n-1 all of which are likely for the fd table.
> (Especially if there is no 'popcnt' instruction.)
>
> David
bitmap_weight() uses hweight_*() under the hood, which then falls
through to platform-specific popcnt where available.
Re, lib/bitmap.c and arch/.../asm/bitops.h or arch/.../hweight.S, for
the impl details.
FrankH.
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
From: Frank Hofmann
> Sent: 18 October 2022 09:13
>
> On Tue, Oct 18, 2022 at 6:02 AM Ivan Babrou <[email protected]> wrote:
> >
> > On Mon, Oct 17, 2022 at 6:47 PM Andrew Morton <[email protected]> wrote:
> > > > +
> > > > + fdt = files_fdtable(p->files);
> > > > + size = fdt->max_fds;
> > > > +
> > > > + for (i = size / BITS_PER_LONG; i > 0;)
> > > > + open_fds += hweight64(fdt->open_fds[--i]);
> > >
> > > Could BITMAP_WEIGHT() or __bitmap_weight() or bitmap_weight() be used here?
> >
> > That's a great suggestion. I tested it with bitmap_weight() and it
> > looks much cleaner while providing the same result.
> >
> > I just sent the v3 with this suggestion applied.
>
> +1 from me on using bitmap_weight() - good spotting that.
Does that have the optimisations for the value being 0, ~0u
or 2**n-1 all of which are likely for the fd table.
(Especially if there is no 'popcnt' instruction.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)