Received: by 2002:a25:5b86:0:0:0:0:0 with SMTP id p128csp1063068ybb; Thu, 28 Mar 2019 18:50:58 -0700 (PDT) X-Google-Smtp-Source: APXvYqwHQrOo7A1V4NWqYLi6MHJcY49KVdaENgH0QssjwnEocUjiJ61kFuWbT9MgLbgqaNbJ0cNo X-Received: by 2002:aa7:884b:: with SMTP id k11mr9392254pfo.49.1553824258132; Thu, 28 Mar 2019 18:50:58 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1553824258; cv=none; d=google.com; s=arc-20160816; b=KhTY1kO2Jo8EMosJuSA70KhSSzQnLjweVJLWS1KBdKVZt8U3apoz9uboqFgkhjekAg dGPvf44mTtn5yh8I/l9dRbABn066w8VW4/tAqXMqVHRQ92YFTuH1COhNOI+ZC6UZedTR Tund9v2k7F/8o0HJvGVUPDB2FiSNvGYvN3DhLE5L/Ss2q1hc3Bkiof0+VUeSLTQYOCXY Ye2wPt1mrQFGt38OGpMdPwwlgqIMotx3ZQ0stFYnrZTuDO5fGoqts7b4egeYS7mCBG+5 T2vyKwhPM4mF8b3XzXMz6bhgCGVt436QpGsD3WQJBW09hZqoaoa8PuU+hzKyCqiiVX9T dP/g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-transfer-encoding:content-disposition:mime-version :references:message-id:subject:cc:to:from:date; bh=qN6Rc7XEv8QwbjagVRoV68366L9/qfMOiNe9AqfUlJU=; b=nBYyHef0Y4q6KtJnRk9b7HGxK/wn4z3IyI3L2WhB/PgBD9wLhjNvMGRZDLzT7fj7ln UDDEp2tWwcg40yTeHFeUilzBIudQwyrWE/BhCUp1wKev7hO9k1VG35CaH8D+YyV3qbdx LU73Z37ZX+6/7BFLdcXHmFUWWhNVqWRBnNYI62UkxombrzwCYkrjnbXWwoRg7r0urawL OZjcURZzPWiuYmNOuPu2qtR5fqCTtglU2migEbmKWD6cwXuwJJX4GOM1K2BdllUVC5XC QihOYm7RJcaKO5m0E+pnokL37nNsnt+LhyXFcAnvDzbsrmfA+ccnETZMR2h4CocwtEhl K9mQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id j8si706655plb.86.2019.03.28.18.50.42; Thu, 28 Mar 2019 18:50:58 -0700 (PDT) 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; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728038AbfC2BuH (ORCPT + 99 others); Thu, 28 Mar 2019 21:50:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47344 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727658AbfC2BuH (ORCPT ); Thu, 28 Mar 2019 21:50:07 -0400 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 345833099FD0; Fri, 29 Mar 2019 01:50:07 +0000 (UTC) Received: from redhat.com (ovpn-121-118.rdu2.redhat.com [10.10.121.118]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 02F606FDDE; Fri, 29 Mar 2019 01:50:05 +0000 (UTC) Date: Thu, 28 Mar 2019 21:50:03 -0400 From: Jerome Glisse To: John Hubbard Cc: Ira Weiny , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Dan Williams Subject: Re: [PATCH v2 02/11] mm/hmm: use reference counting for HMM struct v2 Message-ID: <20190329015003.GE16680@redhat.com> References: <20190325144011.10560-1-jglisse@redhat.com> <20190325144011.10560-3-jglisse@redhat.com> <20190328110719.GA31324@iweiny-DESK2.sc.intel.com> <20190328191122.GA5740@redhat.com> <20190328212145.GA13560@redhat.com> <20190328165708.GH31324@iweiny-DESK2.sc.intel.com> <20190329010059.GB16680@redhat.com> <55dd8607-c91b-12ab-e6d7-adfe6d9cb5e2@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <55dd8607-c91b-12ab-e6d7-adfe6d9cb5e2@nvidia.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.41]); Fri, 29 Mar 2019 01:50:07 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >>> [...] > >>>>>>>> @@ -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