On Wed, 25 Oct 2023 at 02:01, Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> rcu_read_lock();
> - file = get_file_rcu(&i915->gem.mmap_singleton);
> + file = get_file_rcu_once(&i915->gem.mmap_singleton);
> rcu_read_unlock();
> if (file)
> return file;
Honestly, the get_file_rcu_once() function seems a bit pointless.
The above doesn't want a loop at all. Yet your "once" still does loop,
because "even get_file_rcu_once() is trying to deal with the whole
"the pointer itself changed".
And the i915 code is actually designed to just depend on the atomicity
of the mmap_singleton access itself, in how it uses
cmpxchg(&i915->gem.mmap_singleton, file, NULL);
...
file = READ_ONCE(i915->gem.mmap_singleton);
and literally says "I'll remove my singleton as it is released". The
important part there is that the 'map_singleton' pointer itself isn't
actually reference-counted - it's the reverse, where
reference-counting *other* instances will then auto-clear it.
And that's what then makes that get_file_rcu() model not work with it,
because get_file_rcu() kind of assumes that the argument it gets is
*part* of the reference counting, not a cached *result* of the
reference counting that gets cleared as a result of the ref going down
to zero.
I may explain my objections badly, but hopefully you get what I mean.
And I think that also means that using that new get_file_rcu_once()
may be hiding the actual problem, but it's still conceptually wrong,
because it still has that conceptual model of "the pointer I'm getting
is part of the reference counting", when it really isn't.
So I think we'd actually be better off with something that is more
clearly different from get_file_rcu() in naming, so make that
conceptual difference clearer. Make it be something like
"get_active_file(struct file **)", and make the implementation be just
exactly what your current __get_file_rcu() is with no loops at all.
Then thew i915 code ends up being
rcu_read_lock();
file = get_active_file(&i915->gem.mmap_singleton);
rcu_read_unlock();
if (!IS_ERR_OR_NULL(file))
return file;
.. create new mmap_singleton ..
and that's it.
If you don't want to expose __get_file_rcu() as-is, you could maybe just do
struct file *get_active_file(struct file **fp)
{
struct file *file;
rcu_read_lock();
file = __get_file_rcu(fp);
rcu_read_unlock();
return file;
}
and then the i916 code would just drop the RCU locking that it has no
business even knowing about.
I realize that my complaints are a bit conceptual, and that the
practical end result is pretty much the same, but I do think that it
is worth noting this conceptual difference between "file pointer is
ref-counted" and "file counter is *controlled* by ref-counting".
Add a comment to the effect at get_active_file() users.
The old i915 code is already racy, in that it's called a "singleton",
but if you have multiple concurrent callers to mmap_singleton(), they
all might see a NULL file at first, and then they all create
*separate* new "singleton" files, and they *all* do that
smp_store_mb(i915->gem.mmap_singleton, file);
and one random case of them happens to win the race and set *its* file
as "THE singleton" file.
So your "let's loop if it changes" is not fixing anything as-is, and
it's just actually hiding what is going on.
If the i915 code wants to be consistent and really have just one
singleton, it needs to do the looping with some cmpxchg or whatever
itself. Doing the loop in some get_file_rcu_once() function for when
the file pointer changed isn't going to fix the race.
Am I missing something?
Linus
