Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 28 Mar 2019 22:25:19 -0400
From:   Jerome Glisse <jglisse@redhat.com>
To:     Ira Weiny <ira.weiny@intel.com>
Cc:     John Hubbard <jhubbard@nvidia.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Dan Williams <dan.j.williams@intel.com>
Subject: Re: [PATCH v2 02/11] mm/hmm: use reference counting for HMM struct v2
Message-ID: <20190329022519.GJ16680@redhat.com>
References: <20190328110719.GA31324@iweiny-DESK2.sc.intel.com>
 <20190328191122.GA5740@redhat.com>
 <c8fd897f-b9d3-a77b-9898-78e20221ba44@nvidia.com>
 <20190328212145.GA13560@redhat.com>
 <fcb7be01-38c1-ed1f-70a0-d03dc9260473@nvidia.com>
 <20190328165708.GH31324@iweiny-DESK2.sc.intel.com>
 <20190329010059.GB16680@redhat.com>
 <55dd8607-c91b-12ab-e6d7-adfe6d9cb5e2@nvidia.com>
 <20190329015003.GE16680@redhat.com>
 <20190328182100.GJ31324@iweiny-DESK2.sc.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190328182100.GJ31324@iweiny-DESK2.sc.intel.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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

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