diff mbox series

[06/12] mempolicy trivia: use pgoff_t in shared mempolicy tree

Message ID d09b9c35-2856-f749-6a13-64dfe6415ef1@google.com
State New
Headers
Series [01/12] hugetlbfs: drop shared NUMA mempolicy pretence |

Commit Message

Hugh Dickins Sept. 25, 2023, 8:28 a.m. UTC 
  Prefer the more explicit "pgoff_t" to "unsigned long" when dealing with
a shared mempolicy tree.  Delete confusing comment about pseudo mm vmas.

Signed-off-by: Hugh Dickins <hughd@google.com>
---
 include/linux/mempolicy.h | 12 +++---------
 mm/mempolicy.c            |  8 ++++----
 2 files changed, 7 insertions(+), 13 deletions(-)

Comments

Matthew Wilcox Sept. 25, 2023, 10:31 p.m. UTC | #1 
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.

> +++ 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
Matthew Wilcox Sept. 25, 2023, 10:38 p.m. UTC | #2 
On Mon, Sep 25, 2023 at 11:31:40PM +0100, 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

Actually, a fourth has occurred to me

> >  struct sp_node {
> >  	struct rb_node nd;
> > -	unsigned long start, end;
> > +	pgoff_t start, end;
> >  	struct mempolicy *policy;
> >  };

This data structure is unused outside mempolicy.c today, and you don't
add any.  Perhaps we could move it from mempolicy.h to mempolicy.c?
Hugh Dickins Sept. 26, 2023, 9:19 p.m. UTC | #3 
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
diff mbox series

Patch

diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
index 8013d716dc46..12f7dc74a457 100644
--- a/include/linux/mempolicy.h
+++ b/include/linux/mempolicy.h
@@ -107,18 +107,12 @@  static inline bool mpol_equal(struct mempolicy *a, struct mempolicy *b)
 
 /*
  * Tree of shared policies for a shared memory region.
- * Maintain the policies in a pseudo mm that contains vmas. The vmas
- * carry the policy. As a special twist the pseudo mm is indexed in pages, not
- * bytes, so that we can work with shared memory segments bigger than
- * unsigned long.
  */
-
 struct sp_node {
 	struct rb_node nd;
-	unsigned long start, end;
+	pgoff_t start, end;
 	struct mempolicy *policy;
 };
-
 struct shared_policy {
 	struct rb_root root;
 	rwlock_t lock;
@@ -130,7 +124,7 @@  int mpol_set_shared_policy(struct shared_policy *sp,
 			   struct vm_area_struct *vma, struct mempolicy *mpol);
 void mpol_free_shared_policy(struct shared_policy *sp);
 struct mempolicy *mpol_shared_policy_lookup(struct shared_policy *sp,
-					    unsigned long idx);
+					    pgoff_t idx);
 
 struct mempolicy *get_task_policy(struct task_struct *p);
 struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
@@ -216,7 +210,7 @@  static inline void mpol_free_shared_policy(struct shared_policy *sp)
 }
 
 static inline struct mempolicy *
-mpol_shared_policy_lookup(struct shared_policy *sp, unsigned long idx)
+mpol_shared_policy_lookup(struct shared_policy *sp, pgoff_t idx)
 {
 	return NULL;
 }
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 121bb490481b..065e886ec9b6 100644
--- a/mm/mempolicy.c
+++ 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)
 {
 	struct rb_node *n = sp->root.rb_node;
 
@@ -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)
 {
 	struct mempolicy *pol = NULL;
 	struct sp_node *sn;
@@ -2665,8 +2665,8 @@  static struct sp_node *sp_alloc(unsigned long start, unsigned long end,
 }
 
 /* Replace a policy range. */
-static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
-				 unsigned long end, struct sp_node *new)
+static int shared_policy_replace(struct shared_policy *sp, pgoff_t start,
+				 pgoff_t end, struct sp_node *new)
 {
 	struct sp_node *n;
 	struct sp_node *n_new = NULL;