Re: [PATCH v2 02/11] mm/hmm: use reference counting for HMM struct v2

[Jerome Glisse <jglisse@redhat.com>]

On Thu, Mar 28, 2019 at 11:21:00AM -0700, Ira Weiny wrote:
> On Thu, Mar 28, 2019 at 09:50:03PM -0400, Jerome Glisse wrote:
> > On Thu, Mar 28, 2019 at 06:18:35PM -0700, John Hubbard wrote:
> > > On 3/28/19 6:00 PM, Jerome Glisse wrote:
> > > > On Thu, Mar 28, 2019 at 09:57:09AM -0700, Ira Weiny wrote:
> > > >> On Thu, Mar 28, 2019 at 05:39:26PM -0700, John Hubbard wrote:
> > > >>> On 3/28/19 2:21 PM, Jerome Glisse wrote:
> > > >>>> On Thu, Mar 28, 2019 at 01:43:13PM -0700, John Hubbard wrote:
> > > >>>>> On 3/28/19 12:11 PM, Jerome Glisse wrote:
> > > >>>>>> On Thu, Mar 28, 2019 at 04:07:20AM -0700, Ira Weiny wrote:
> > > >>>>>>> On Mon, Mar 25, 2019 at 10:40:02AM -0400, Jerome Glisse wrote:
> > > >>>>>>>> From: Jérôme Glisse <jglisse@redhat.com>
> > > >>> [...]
> > > >>>>>>>> @@ -67,14 +78,9 @@ struct hmm {
> > > >>>>>>>>   */
> > > >>>>>>>>  static struct hmm *hmm_register(struct mm_struct *mm)
> > > >>>>>>>>  {
> > > >>>>>>>> -	struct hmm *hmm = READ_ONCE(mm->hmm);
> > > >>>>>>>> +	struct hmm *hmm = mm_get_hmm(mm);
> > > >>>>>>>
> > > >>>>>>> FWIW: having hmm_register == "hmm get" is a bit confusing...
> > > >>>>>>
> > > >>>>>> The thing is that you want only one hmm struct per process and thus
> > > >>>>>> if there is already one and it is not being destroy then you want to
> > > >>>>>> reuse it.
> > > >>>>>>
> > > >>>>>> Also this is all internal to HMM code and so it should not confuse
> > > >>>>>> anyone.
> > > >>>>>>
> > > >>>>>
> > > >>>>> Well, it has repeatedly come up, and I'd claim that it is quite 
> > > >>>>> counter-intuitive. So if there is an easy way to make this internal 
> > > >>>>> HMM code clearer or better named, I would really love that to happen.
> > > >>>>>
> > > >>>>> And we shouldn't ever dismiss feedback based on "this is just internal
> > > >>>>> xxx subsystem code, no need for it to be as clear as other parts of the
> > > >>>>> kernel", right?
> > > >>>>
> > > >>>> Yes but i have not seen any better alternative that present code. If
> > > >>>> there is please submit patch.
> > > >>>>
> > > >>>
> > > >>> Ira, do you have any patch you're working on, or a more detailed suggestion there?
> > > >>> If not, then I might (later, as it's not urgent) propose a small cleanup patch 
> > > >>> I had in mind for the hmm_register code. But I don't want to duplicate effort 
> > > >>> if you're already thinking about it.
> > > >>
> > > >> No I don't have anything.
> > > >>
> > > >> I was just really digging into these this time around and I was about to
> > > >> comment on the lack of "get's" for some "puts" when I realized that
> > > >> "hmm_register" _was_ the get...
> > > >>
> > > >> :-(
> > > >>
> > > > 
> > > > The get is mm_get_hmm() were you get a reference on HMM from a mm struct.
> > > > John in previous posting complained about me naming that function hmm_get()
> > > > and thus in this version i renamed it to mm_get_hmm() as we are getting
> > > > a reference on hmm from a mm struct.
> > > 
> > > Well, that's not what I recommended, though. The actual conversation went like
> > > this [1]:
> > > 
> > > ---------------------------------------------------------------
> > > >> So for this, hmm_get() really ought to be symmetric with
> > > >> hmm_put(), by taking a struct hmm*. And the null check is
> > > >> not helping here, so let's just go with this smaller version:
> > > >>
> > > >> static inline struct hmm *hmm_get(struct hmm *hmm)
> > > >> {
> > > >>     if (kref_get_unless_zero(&hmm->kref))
> > > >>         return hmm;
> > > >>
> > > >>     return NULL;
> > > >> }
> > > >>
> > > >> ...and change the few callers accordingly.
> > > >>
> > > >
> > > > What about renaning hmm_get() to mm_get_hmm() instead ?
> > > >
> > > 
> > > For a get/put pair of functions, it would be ideal to pass
> > > the same argument type to each. It looks like we are passing
> > > around hmm*, and hmm retains a reference count on hmm->mm,
> > > so I think you have a choice of using either mm* or hmm* as
> > > the argument. I'm not sure that one is better than the other
> > > here, as the lifetimes appear to be linked pretty tightly.
> > > 
> > > Whichever one is used, I think it would be best to use it
> > > in both the _get() and _put() calls. 
> > > ---------------------------------------------------------------
> > > 
> > > Your response was to change the name to mm_get_hmm(), but that's not
> > > what I recommended.
> > 
> > Because i can not do that, hmm_put() can _only_ take hmm struct as
> > input while hmm_get() can _only_ get mm struct as input.
> > 
> > hmm_put() can only take hmm because the hmm we are un-referencing
> > might no longer be associated with any mm struct and thus i do not
> > have a mm struct to use.
> > 
> > hmm_get() can only get mm as input as we need to be careful when
> > accessing the hmm field within the mm struct and thus it is better
> > to have that code within a function than open coded and duplicated
> > all over the place.
> 
> The input value is not the problem.  The problem is in the naming.
> 
> obj = get_obj( various parameters );
> put_obj(obj);
> 
> 
> The problem is that the function is named hmm_register() either "gets" a
> reference to _or_ creates and gets a reference to the hmm object.
> 
> What John is probably ready to submit is something like.
> 
> struct hmm *get_create_hmm(struct mm *mm);
> void put_hmm(struct hmm *hmm);
> 
> 
> So when you are reading the code you see...
> 
> foo(...) {
> 	struct hmm *hmm = get_create_hmm(mm);
> 
> 	if (!hmm)
> 		error...
> 
> 	do stuff...
> 
> 	put_hmm(hmm);
> }
> 
> Here I can see a very clear get/put pair.  The name also shows that the hmm is
> created if need be as well as getting a reference.
> 

You only need to create HMM when you either register a mirror or
register a range. So they two pattern:

    average_foo() {
        struct hmm *hmm = mm_get_hmm(mm);
        ...
        hmm_put(hmm);
    }

    register_foo() {
        struct hmm *hmm = hmm_register(mm);
        ...
        return 0;
    error:
        ...
        hmm_put(hmm);
    }

Cheers,
Jérôme