[2/2] ipc,namespace: batch free ipc_namespace structures
Commit Message
Instead of waiting for an RCU grace period between each ipc_namespace
structure that is being freed, wait an RCU grace period for every batch
of ipc_namespace structures.
Thanks to Al Viro for the suggestion of the helper function.
This speeds up the run time of the test case that allocates ipc_namespaces
in a loop from 6 minutes, to a little over 1 second:
real 0m1.192s
user 0m0.038s
sys 0m1.152s
Signed-off-by: Rik van Riel <riel@surriel.com>
Reported-by: Chris Mason <clm@meta.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/namespace.c | 10 ++++++++++
include/linux/mount.h | 1 +
ipc/namespace.c | 13 ++++++++++---
3 files changed, 21 insertions(+), 3 deletions(-)
Comments
Rik van Riel <riel@surriel.com> writes:
> Instead of waiting for an RCU grace period between each ipc_namespace
> structure that is being freed, wait an RCU grace period for every batch
> of ipc_namespace structures.
>
> Thanks to Al Viro for the suggestion of the helper function.
>
> This speeds up the run time of the test case that allocates ipc_namespaces
> in a loop from 6 minutes, to a little over 1 second:
>
> real 0m1.192s
> user 0m0.038s
> sys 0m1.152s
>
> Signed-off-by: Rik van Riel <riel@surriel.com>
> Reported-by: Chris Mason <clm@meta.com>
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> fs/namespace.c | 10 ++++++++++
> include/linux/mount.h | 1 +
> ipc/namespace.c | 13 ++++++++++---
> 3 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index ab467ee58341..296432ba3716 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1397,6 +1397,16 @@ struct vfsmount *mntget(struct vfsmount *mnt)
> }
> EXPORT_SYMBOL(mntget);
>
> +/*
> + * Make a mount point inaccessible to new lookups.
> + * Because there may still be current users, the caller MUST WAIT
> + * for an RCU grace period before destroying the mount point.
> + */
> +void mnt_make_shortterm(struct vfsmount *mnt)
> +{
> + real_mount(mnt)->mnt_ns = NULL;
> +}
> +
> /**
> * path_is_mountpoint() - Check if path is a mount in the current namespace.
> * @path: path to check
> diff --git a/include/linux/mount.h b/include/linux/mount.h
> index 62475996fac6..ec55a031aa8c 100644
> --- a/include/linux/mount.h
> +++ b/include/linux/mount.h
> @@ -88,6 +88,7 @@ extern void mnt_drop_write(struct vfsmount *mnt);
> extern void mnt_drop_write_file(struct file *file);
> extern void mntput(struct vfsmount *mnt);
> extern struct vfsmount *mntget(struct vfsmount *mnt);
> +extern void mnt_make_shortterm(struct vfsmount *mnt);
> extern struct vfsmount *mnt_clone_internal(const struct path *path);
> extern bool __mnt_is_readonly(struct vfsmount *mnt);
> extern bool mnt_may_suid(struct vfsmount *mnt);
> diff --git a/ipc/namespace.c b/ipc/namespace.c
> index a26860a41dac..6ecc30effd3e 100644
> --- a/ipc/namespace.c
> +++ b/ipc/namespace.c
> @@ -145,10 +145,11 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
>
> static void free_ipc_ns(struct ipc_namespace *ns)
> {
> - /* mq_put_mnt() waits for a grace period as kern_unmount()
> - * uses synchronize_rcu().
> + /*
> + * Caller needs to wait for an RCU grace period to have passed
> + * after making the mount point inaccessible to new accesses.
> */
> - mq_put_mnt(ns);
mq_put_mnt() is not needed anymore, should it be removed?
> + mntput(ns->mq_mnt);
> sem_exit_ns(ns);
> msg_exit_ns(ns);
> shm_exit_ns(ns);
> @@ -168,6 +169,12 @@ static void free_ipc(struct work_struct *unused)
> struct llist_node *node = llist_del_all(&free_ipc_list);
> struct ipc_namespace *n, *t;
>
> + llist_for_each_entry_safe(n, t, node, mnt_llist)
> + mnt_make_shortterm(n->mq_mnt);
> +
> + /* Wait for any last users to have gone away. */
> + synchronize_rcu();
> +
> llist_for_each_entry_safe(n, t, node, mnt_llist)
> free_ipc_ns(n);
> }
On Fri, 2023-01-27 at 12:03 +0100, Giuseppe Scrivano wrote:
> Rik van Riel <riel@surriel.com> writes:
>
> >
> > +++ b/ipc/namespace.c
> > @@ -145,10 +145,11 @@ void free_ipcs(struct ipc_namespace *ns,
> > struct ipc_ids *ids,
> >
> > static void free_ipc_ns(struct ipc_namespace *ns)
> > {
> > - /* mq_put_mnt() waits for a grace period as kern_unmount()
> > - * uses synchronize_rcu().
> > + /*
> > + * Caller needs to wait for an RCU grace period to have
> > passed
> > + * after making the mount point inaccessible to new
> > accesses.
> > */
> > - mq_put_mnt(ns);
>
> mq_put_mnt() is not needed anymore, should it be removed?
Yes, indeed. Thank you!
I'll send a v3 of the series :)
@@ -1397,6 +1397,16 @@ struct vfsmount *mntget(struct vfsmount *mnt)
}
EXPORT_SYMBOL(mntget);
+/*
+ * Make a mount point inaccessible to new lookups.
+ * Because there may still be current users, the caller MUST WAIT
+ * for an RCU grace period before destroying the mount point.
+ */
+void mnt_make_shortterm(struct vfsmount *mnt)
+{
+ real_mount(mnt)->mnt_ns = NULL;
+}
+
/**
* path_is_mountpoint() - Check if path is a mount in the current namespace.
* @path: path to check
@@ -88,6 +88,7 @@ extern void mnt_drop_write(struct vfsmount *mnt);
extern void mnt_drop_write_file(struct file *file);
extern void mntput(struct vfsmount *mnt);
extern struct vfsmount *mntget(struct vfsmount *mnt);
+extern void mnt_make_shortterm(struct vfsmount *mnt);
extern struct vfsmount *mnt_clone_internal(const struct path *path);
extern bool __mnt_is_readonly(struct vfsmount *mnt);
extern bool mnt_may_suid(struct vfsmount *mnt);
@@ -145,10 +145,11 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
static void free_ipc_ns(struct ipc_namespace *ns)
{
- /* mq_put_mnt() waits for a grace period as kern_unmount()
- * uses synchronize_rcu().
+ /*
+ * Caller needs to wait for an RCU grace period to have passed
+ * after making the mount point inaccessible to new accesses.
*/
- mq_put_mnt(ns);
+ mntput(ns->mq_mnt);
sem_exit_ns(ns);
msg_exit_ns(ns);
shm_exit_ns(ns);
@@ -168,6 +169,12 @@ static void free_ipc(struct work_struct *unused)
struct llist_node *node = llist_del_all(&free_ipc_list);
struct ipc_namespace *n, *t;
+ llist_for_each_entry_safe(n, t, node, mnt_llist)
+ mnt_make_shortterm(n->mq_mnt);
+
+ /* Wait for any last users to have gone away. */
+ synchronize_rcu();
+
llist_for_each_entry_safe(n, t, node, mnt_llist)
free_ipc_ns(n);
}