[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YkNTvFqWI5F5w+DW@google.com>
Date: Tue, 29 Mar 2022 18:45:16 +0000
From: Sean Christopherson <seanjc@...gle.com>
To: Chao Peng <chao.p.peng@...ux.intel.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mm@...ck.org, linux-fsdevel@...r.kernel.org,
linux-api@...r.kernel.org, qemu-devel@...gnu.org,
Paolo Bonzini <pbonzini@...hat.com>,
Jonathan Corbet <corbet@....net>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
Wanpeng Li <wanpengli@...cent.com>,
Jim Mattson <jmattson@...gle.com>,
Joerg Roedel <joro@...tes.org>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
x86@...nel.org, "H . Peter Anvin" <hpa@...or.com>,
Hugh Dickins <hughd@...gle.com>,
Jeff Layton <jlayton@...nel.org>,
"J . Bruce Fields" <bfields@...ldses.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Mike Rapoport <rppt@...nel.org>,
Steven Price <steven.price@....com>,
"Maciej S . Szmigiero" <mail@...iej.szmigiero.name>,
Vlastimil Babka <vbabka@...e.cz>,
Vishal Annapurve <vannapurve@...gle.com>,
Yu Zhang <yu.c.zhang@...ux.intel.com>,
"Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
luto@...nel.org, jun.nakajima@...el.com, dave.hansen@...el.com,
ak@...ux.intel.com, david@...hat.com
Subject: Re: [PATCH v5 02/13] mm: Introduce memfile_notifier
On Thu, Mar 10, 2022, Chao Peng wrote:
> diff --git a/mm/Makefile b/mm/Makefile
> index 70d4309c9ce3..f628256dce0d 100644
> +void memfile_notifier_invalidate(struct memfile_notifier_list *list,
> + pgoff_t start, pgoff_t end)
> +{
> + struct memfile_notifier *notifier;
> + int id;
> +
> + id = srcu_read_lock(&srcu);
> + list_for_each_entry_srcu(notifier, &list->head, list,
> + srcu_read_lock_held(&srcu)) {
> + if (notifier->ops && notifier->ops->invalidate)
Any reason notifier->ops isn't mandatory?
> + notifier->ops->invalidate(notifier, start, end);
> + }
> + srcu_read_unlock(&srcu, id);
> +}
> +
> +void memfile_notifier_fallocate(struct memfile_notifier_list *list,
> + pgoff_t start, pgoff_t end)
> +{
> + struct memfile_notifier *notifier;
> + int id;
> +
> + id = srcu_read_lock(&srcu);
> + list_for_each_entry_srcu(notifier, &list->head, list,
> + srcu_read_lock_held(&srcu)) {
> + if (notifier->ops && notifier->ops->fallocate)
> + notifier->ops->fallocate(notifier, start, end);
> + }
> + srcu_read_unlock(&srcu, id);
> +}
> +
> +void memfile_register_backing_store(struct memfile_backing_store *bs)
> +{
> + BUG_ON(!bs || !bs->get_notifier_list);
> +
> + list_add_tail(&bs->list, &backing_store_list);
> +}
> +
> +void memfile_unregister_backing_store(struct memfile_backing_store *bs)
> +{
> + list_del(&bs->list);
Allowing unregistration of a backing store is broken. Using the _safe() variant
is not sufficient to guard against concurrent modification. I don't see any reason
to support this out of the gate, the only reason to support unregistering a backing
store is if the backing store is implemented as a module, and AFAIK none of the
backing stores we plan on supporting initially support being built as a module.
These aren't exported, so it's not like that's even possible. Registration would
also be broken if modules are allowed, I'm pretty sure module init doesn't run
under a global lock.
We can always add this complexity if it's needed in the future, but for now the
easiest thing would be to tag memfile_register_backing_store() with __init and
make backing_store_list __ro_after_init.
> +}
> +
> +static int memfile_get_notifier_info(struct inode *inode,
> + struct memfile_notifier_list **list,
> + struct memfile_pfn_ops **ops)
> +{
> + struct memfile_backing_store *bs, *iter;
> + struct memfile_notifier_list *tmp;
> +
> + list_for_each_entry_safe(bs, iter, &backing_store_list, list) {
> + tmp = bs->get_notifier_list(inode);
> + if (tmp) {
> + *list = tmp;
> + if (ops)
> + *ops = &bs->pfn_ops;
> + return 0;
> + }
> + }
> + return -EOPNOTSUPP;
> +}
> +
> +int memfile_register_notifier(struct inode *inode,
Taking an inode is a bit odd from a user perspective. Any reason not to take a
"struct file *" and get the inode here? That would give callers a hint that they
need to hold a reference to the file for the lifetime of the registration.
> + struct memfile_notifier *notifier,
> + struct memfile_pfn_ops **pfn_ops)
> +{
> + struct memfile_notifier_list *list;
> + int ret;
> +
> + if (!inode || !notifier | !pfn_ops)
Bitwise | instead of logical ||. But IMO taking in a pfn_ops pointer is silly.
More below.
> + return -EINVAL;
> +
> + ret = memfile_get_notifier_info(inode, &list, pfn_ops);
> + if (ret)
> + return ret;
> +
> + spin_lock(&list->lock);
> + list_add_rcu(¬ifier->list, &list->head);
> + spin_unlock(&list->lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(memfile_register_notifier);
> +
> +void memfile_unregister_notifier(struct inode *inode,
> + struct memfile_notifier *notifier)
> +{
> + struct memfile_notifier_list *list;
> +
> + if (!inode || !notifier)
> + return;
> +
> + BUG_ON(memfile_get_notifier_info(inode, &list, NULL));
Eww. Rather than force the caller to provide the inode/file and the notifier,
what about grabbing the backing store itself in the notifier?
struct memfile_notifier {
struct list_head list;
struct memfile_notifier_ops *ops;
struct memfile_backing_store *bs;
};
That also helps avoid confusing between "ops" and "pfn_ops". IMO, exposing
memfile_backing_store to the caller isn't a big deal, and is preferable to having
to rewalk multiple lists just to delete a notifier.
Then this can become:
void memfile_unregister_notifier(struct memfile_notifier *notifier)
{
spin_lock(¬ifier->bs->list->lock);
list_del_rcu(¬ifier->list);
spin_unlock(¬ifier->bs->list->lock);
synchronize_srcu(&srcu);
}
and registration can be:
int memfile_register_notifier(const struct file *file,
struct memfile_notifier *notifier)
{
struct memfile_notifier_list *list;
struct memfile_backing_store *bs;
int ret;
if (!file || !notifier)
return -EINVAL;
list_for_each_entry(bs, &backing_store_list, list) {
list = bs->get_notifier_list(file_inode(file));
if (list) {
notifier->bs = bs;
spin_lock(&list->lock);
list_add_rcu(¬ifier->list, &list->head);
spin_unlock(&list->lock);
return 0;
}
}
return -EOPNOTSUPP;
}
Powered by blists - more mailing lists