| Message ID | ea413d84-8b43-91c2-feef-92998bc7c1e2@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:29:28AM -0700, Hugh Dickins wrote: > + /* alloc node covering entire file; adds ref to new */ This comment is confusing. sp_alloc initialises the refcount of 'n' to 1. Which is the same memory referred to by the name 'new' in __mpol_dup(), but in this function, the name "new" refers to the mempolicy called "old" in __mpol_dup(). > + n = sp_alloc(0, MAX_LFS_FILESIZE >> PAGE_SHIFT, new); > + if (n) > + sp_insert(sp, n); > put_new: > mpol_put(new); /* drop initial ref */ > free_scratch: This is all a bit inefficient, really. We call mpol_new() to get a new mpol, then we set it up, then we dup it, then we free it. It'd be nice if we could donate it instead of copying it. Maybe you'll do something like that later.
On Mon, 25 Sep 2023, Matthew Wilcox wrote: > On Mon, Sep 25, 2023 at 01:29:28AM -0700, Hugh Dickins wrote: > > + /* alloc node covering entire file; adds ref to new */ > > This comment is confusing. sp_alloc initialises the refcount of 'n' to 1. > Which is the same memory referred to by the name 'new' in __mpol_dup(), > but in this function, the name "new" refers to the mempolicy called > "old" in __mpol_dup(). No promises, but I'll see if I can make it look better in v2. > > > + n = sp_alloc(0, MAX_LFS_FILESIZE >> PAGE_SHIFT, new); > > + if (n) > > + sp_insert(sp, n); > > put_new: > > mpol_put(new); /* drop initial ref */ > > free_scratch: > > This is all a bit inefficient, really. We call mpol_new() to get a > new mpol, then we set it up, then we dup it, then we free it. It'd > be nice if we could donate it instead of copying it. Maybe you'll > do something like that later. "later" is probably the operative word. I do have an unincluded 2017 patch where I had that same realization, and wrote "I suspect that this series of commits may be adding to an absurdity of over-mpol_dup()ing: but that's for some other future cleanup, right now I'm just happy not to be corrupting or leaking mpols." Hugh
diff --git a/mm/mempolicy.c b/mm/mempolicy.c index 065e886ec9b6..a22b641cfd6b 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -2749,7 +2749,7 @@ void mpol_shared_policy_init(struct shared_policy *sp, struct mempolicy *mpol) rwlock_init(&sp->lock); if (mpol) { - struct vm_area_struct pvma; + struct sp_node *n; struct mempolicy *new; NODEMASK_SCRATCH(scratch); @@ -2766,11 +2766,10 @@ void mpol_shared_policy_init(struct shared_policy *sp, struct mempolicy *mpol) if (ret) goto put_new; - /* Create pseudo-vma that contains just the policy */ - vma_init(&pvma, NULL); - pvma.vm_end = TASK_SIZE; /* policy covers entire file */ - mpol_set_shared_policy(sp, &pvma, new); /* adds ref */ - + /* alloc node covering entire file; adds ref to new */ + n = sp_alloc(0, MAX_LFS_FILESIZE >> PAGE_SHIFT, new); + if (n) + sp_insert(sp, n); put_new: mpol_put(new); /* drop initial ref */ free_scratch:
mpol_shared_policy_init() does not need to use a pseudo-vma: it can use sp_alloc() and sp_insert() directly, since the object's shared policy tree is empty and inaccessible (needing no lock) at get_inode() time. Signed-off-by: Hugh Dickins <hughd@google.com> --- mm/mempolicy.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)