On Thu, 14 Sep 2023 19:54:55 -0400
Gregory Price <gourry.memverge@xxxxxxxxx> 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@xxxxxxxxxxxx>
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
> *
