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

[John Hubbard]

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@xxxxxxxxxx>
>>> [...]
>>>>>>>> @@ -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.

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

[1] https://lore.kernel.org/r/1ccab0d3-7e90-8e39-074d-02ffbfc68480@xxxxxxxxxx

thanks,
-- 
John Hubbard
NVIDIA