Messages in this thread Patch in this message | | | Date | Mon, 2 Oct 2023 12:03:17 +0100 | From | Jonathan Cameron <> | Subject | Re: [RFC PATCH 1/3] mm/mempolicy: refactor do_set_mempolicy for code re-use |
| |
On Thu, 14 Sep 2023 19:54:55 -0400 Gregory Price <gourry.memverge@gmail.com> wrote:
> Refactors do_set_mempolicy into swap_mempolicy and do_set_mempolicy > so that swap_mempolicy can be re-used with set_mempolicy2. > > Signed-off-by: Gregory Price <gregory.price@memverge.com>
Obviously this is an RFC, so you probably didn't give it the polish a finished patch might have. Still I was curious and reading it and I can't resist pointing out trivial stuff.. So....
> --- > mm/mempolicy.c | 44 +++++++++++++++++++++++++++++--------------- > 1 file changed, 29 insertions(+), 15 deletions(-) > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index 42b5567e3773..f49337f6f300 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -855,28 +855,21 @@ static int mbind_range(struct vma_iterator *vmi, struct vm_area_struct *vma, > return vma_replace_policy(vma, new_pol); > } > > -/* Set the process memory policy */ > -static long do_set_mempolicy(unsigned short mode, unsigned short flags, > - nodemask_t *nodes) > +/* Swap in a new mempolicy, release the old one if successful */
Not really swapping. More replacing given we don't get the old one back to do something else with it.
> +static long swap_mempolicy(struct mempolicy *new, > + nodemask_t *nodes)
Excessive wrapping.
> { > - struct mempolicy *new, *old; > - NODEMASK_SCRATCH(scratch); > + struct mempolicy *old = NULL; > int ret; > + NODEMASK_SCRATCH(scratch);
I'd avoid the reordering as makes it look like slightly more is happening in this change than is actually the case.
> > if (!scratch) > return -ENOMEM; > > - new = mpol_new(mode, flags, nodes); > - if (IS_ERR(new)) { > - ret = PTR_ERR(new); > - goto out; > - } > - > task_lock(current); > ret = mpol_set_nodemask(new, nodes, scratch); > if (ret) { > task_unlock(current); > - mpol_put(new); > goto out; > } > > @@ -884,14 +877,35 @@ static long do_set_mempolicy(unsigned short mode, unsigned short flags, > current->mempolicy = new; > if (new && new->mode == MPOL_INTERLEAVE) > current->il_prev = MAX_NUMNODES-1; > - task_unlock(current); > - mpol_put(old); > - ret = 0; > out: > + task_unlock(current); > + if (old) > + mpol_put(old); It's protected against NULL parameter internally, so mpol_put(old);
which has advantage that a block of diff will hopefully disappear making this patch easier to read.
> + > NODEMASK_SCRATCH_FREE(scratch); > return ret; > } > > +/* Set the process memory policy */ > +static long do_set_mempolicy(unsigned short mode, unsigned short flags, > + nodemask_t *nodes) > +{ > + struct mempolicy *new; > + int ret; > + > + new = mpol_new(mode, flags, nodes); > + if (IS_ERR(new)) { > + ret = PTR_ERR(new); > + goto out;
Given nothing to do at out lable, in keeping with at least some local style, you could do direct returns on errors.
if (IS_ERR(new)) return PTR_ERR(new)
ret = swap_mempolicy(new, nodes); if (ret) { mpol_put(new); return ret; }
return 0;
> + } > + > + ret = swap_mempolicy(new, nodes); > + if (ret) > + mpol_put(new); > +out: > + return ret; > +} > + > /* > * Return nodemask for policy for get_mempolicy() query > *
| |