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

[Ira Weiny <ira.weiny@intel.com>]

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.

Ira

> 
> > 
> > > 
> > > The hmm_put() is just releasing the reference on the hmm struct.
> > > 
> > > Here i feel i am getting contradicting requirement from different people.
> > > I don't think there is a way to please everyone here.
> > > 
> > 
> > That's not a true conflict: you're comparing your actual implementation
> > to Ira's request, rather than comparing my request to Ira's request.
> > 
> > I think there's a way forward. Ira and I are actually both asking for the
> > same thing:
> > 
> > a) clear, concise get/put routines
> > 
> > b) avoiding odd side effects in functions that have one name, but do
> > additional surprising things.
> 
> Please show me code because i do not see any other way to do it then
> how i did.
> 
> Cheers,
> Jérôme