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
