Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Tue, 26 Sep 2023 14:19:19 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Matthew Wilcox <willy@infradead.org>
cc:     Hugh Dickins <hughd@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andi Kleen <ak@linux.intel.com>,
        Christoph Lameter <cl@linux.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        David Hildenbrand <david@redhat.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Yang Shi <shy828301@gmail.com>,
        Sidhartha Kumar <sidhartha.kumar@oracle.com>,
        Vishal Moola <vishal.moola@gmail.com>,
        Kefeng Wang <wangkefeng.wang@huawei.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Tejun Heo <tj@kernel.org>,
        Mel Gorman <mgorman@techsingularity.net>,
        Michal Hocko <mhocko@suse.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Subject: Re: [PATCH 06/12] mempolicy trivia: use pgoff_t in shared mempolicy
 tree
In-Reply-To: <ZRIKTFaPOmD8RFDQ@casper.infradead.org>
Message-ID: <7191425-f87-99c7-58b9-d54169e3fe0@google.com>
References: <2d872cef-7787-a7ca-10e-9d45a64c80b4@google.com> <d09b9c35-2856-f749-6a13-64dfe6415ef1@google.com> <ZRIKTFaPOmD8RFDQ@casper.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 25 Sep 2023, Matthew Wilcox wrote:
> On Mon, Sep 25, 2023 at 01:28:14AM -0700, Hugh Dickins wrote:
> > Prefer the more explicit "pgoff_t" to "unsigned long" when dealing with
> > a shared mempolicy tree.  Delete confusing comment about pseudo mm vmas.
> 
> Yes, with three quibbles
> 
> >  struct sp_node {
> >  	struct rb_node nd;
> > -	unsigned long start, end;
> > +	pgoff_t start, end;
> >  	struct mempolicy *policy;
> >  };
> > -
> >  struct shared_policy {
> 
> Did you intend to delete the blank line between these two structs?
> That's not our normal style.

I think I did intend it actually, to join both of those structs to the
"Tree of shared policies" comment above them.  But now that I'm looking
again, I think what I'd most like to do (and going against all of your
suggestions e.g. move sp_node to mempolicy.c: good observation, but isn't
keeping them together more helpful to the reader?) is swap those structs
around - struct shared_policy first for the root of the tree, then
struct sp_node showing the nodes of the tree (and still without blank
line).  Wouldn't that be the most helpful way to present them?

I'll knuckle down and do exactly as you have suggested, if you say so:
but above is my own preference.

> 
> > +++ b/mm/mempolicy.c
> > @@ -2444,7 +2444,7 @@ bool __mpol_equal(struct mempolicy *a, struct mempolicy *b)
> >   * reading or for writing
> >   */
> >  static struct sp_node *
> > -sp_lookup(struct shared_policy *sp, unsigned long start, unsigned long end)
> > +sp_lookup(struct shared_policy *sp, pgoff_t start, pgoff_t end)
> 
> While you're reformatting anyway, mind joining these two lines?
> 
> > @@ -2499,7 +2499,7 @@ static void sp_insert(struct shared_policy *sp, struct sp_node *new)
> >  
> >  /* Find shared policy intersecting idx */
> >  struct mempolicy *
> > -mpol_shared_policy_lookup(struct shared_policy *sp, unsigned long idx)
> > +mpol_shared_policy_lookup(struct shared_policy *sp, pgoff_t idx)
> 
> Ditto

Sure, will do (I recall Linus much prefers them as you suggest).
But we have different indentation habits: I think it's spaced exactitude
which irritates you the most, would the style below be okay?
Or maybe you'd prefer to go over-80 with these two.

struct mempolicy *mpol_shared_policy_lookup(struct shared_policy *sp,
						pgoff_t idx)

Hugh