On Sat, Jun 15, 2019 at 07:12:11AM -0700, Christoph Hellwig wrote:
> > + spin_lock(&mm->page_table_lock);
> > + if (mm->hmm) {
> > + if (kref_get_unless_zero(&mm->hmm->kref)) {
> > + spin_unlock(&mm->page_table_lock);
> > + return mm->hmm;
> > + }
> > + }
> > + spin_unlock(&mm->page_table_lock);
>
> This could become:
>
> spin_lock(&mm->page_table_lock);
> hmm = mm->hmm
> if (hmm && kref_get_unless_zero(&hmm->kref))
> goto out_unlock;
> spin_unlock(&mm->page_table_lock);
>
> as the last two lines of the function already drop the page_table_lock
> and then return hmm. Or drop the "hmm = mm->hmm" asignment above and
> return mm->hmm as that should be always identical to hmm at the end
> to save another line.
Yeah, I can fuss it some more.
> > + /*
> > + * The mm->hmm pointer is kept valid while notifier ops can be running
> > + * so they don't have to deal with a NULL mm->hmm value
> > + */
>
> The comment confuses me. How does the page_table_lock relate to
> possibly running notifiers, as I can't find that we take
> page_table_lock? Or is it just about the fact that we only clear
> mm->hmm in the free callback, and not in hmm_free?
It was late when I wrote this fixup, the comment is faulty, and there
is no reason to delay this until the SRCU cleanup at this point in the
series.
The ops all get their struct hmm from container_of, the only thing
that refers to mm->hmm is hmm_get_or_create().
I'll revise it tomorrow, the comment will go away and the =NULL will
go to the release callback
Jason
