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 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 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