| Message ID | d09b9c35-2856-f749-6a13-64dfe6415ef1@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mempolicy: cleanups leading to NUMA mpol without vma | expand |
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
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?
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 --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;
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(-)