Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 30 Jul 2020 13:23:19 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Kirill Tkhai <ktkhai@virtuozzo.com>
Cc:     viro@zeniv.linux.org.uk, adobriyan@gmail.com, davem@davemloft.net,
        ebiederm@xmission.com, akpm@linux-foundation.org,
        christian.brauner@ubuntu.com, areber@redhat.com, serge@hallyn.com,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 09/23] ns: Introduce ns_idr to be able to iterate all
 allocated namespaces in the system
Message-ID: <20200730122319.GC23808@casper.infradead.org>
References: <159611007271.535980.15362304262237658692.stgit@localhost.localdomain>
 <159611040870.535980.13460189038999722608.stgit@localhost.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <159611040870.535980.13460189038999722608.stgit@localhost.localdomain>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jul 30, 2020 at 03:00:08PM +0300, Kirill Tkhai wrote:
> This patch introduces a new IDR and functions to add/remove and iterate
> registered namespaces in the system. It will be used to list namespaces
> in /proc/namespaces/... in next patches.

Looks like you could use an XArray for this and it would be fewer lines of
code.

>  
>  static struct vfsmount *nsfs_mnt;
> +static DEFINE_SPINLOCK(ns_lock);
> +static DEFINE_IDR(ns_idr);

XArray includes its own spinlock.

> +/*
> + * Add a newly created ns to ns_idr. The ns must be fully
> + * initialized since it becomes available for ns_get_next()
> + * right after we exit this function.
> + */
> +int ns_idr_register(struct ns_common *ns)
> +{
> +	int ret, id = ns->inum - PROC_NS_MIN_INO;
> +
> +	if (WARN_ON(id < 0))
> +		return -EINVAL;
> +
> +	idr_preload(GFP_KERNEL);
> +	spin_lock_irq(&ns_lock);
> +	ret = idr_alloc(&ns_idr, ns, id, id + 1, GFP_ATOMIC);
> +	spin_unlock_irq(&ns_lock);
> +	idr_preload_end();
> +	return ret < 0 ? ret : 0;

This would simply be return xa_insert_irq(...);

> +}
> +
> +/*
> + * Remove a dead ns from ns_idr. Note, that ns memory must
> + * be freed not earlier then one RCU grace period after
> + * this function, since ns_get_next() uses RCU to iterate the IDR.
> + */
> +void ns_idr_unregister(struct ns_common *ns)
> +{
> +	int id = ns->inum - PROC_NS_MIN_INO;
> +	unsigned long flags;
> +
> +	if (WARN_ON(id < 0))
> +		return;
> +
> +	spin_lock_irqsave(&ns_lock, flags);
> +	idr_remove(&ns_idr, id);
> +	spin_unlock_irqrestore(&ns_lock, flags);
> +}

xa_erase_irqsave();

> +
> +/*
> + * This returns ns with inum greater than @id or NULL.
> + * @id is updated to refer the ns inum.
> + */
> +struct ns_common *ns_get_next(unsigned int *id)
> +{
> +	struct ns_common *ns;
> +
> +	if (*id < PROC_NS_MIN_INO - 1)
> +		*id = PROC_NS_MIN_INO - 1;
> +
> +	*id += 1;
> +	*id -= PROC_NS_MIN_INO;
> +
> +	rcu_read_lock();
> +	do {
> +		ns = idr_get_next(&ns_idr, id);
> +		if (!ns)
> +			break;

xa_find_after();

You'll want a temporary unsigned long to work with ...

> +		if (!refcount_inc_not_zero(&ns->count)) {
> +			ns = NULL;
> +			*id += 1;

you won't need this increment.