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