Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <ZV5zGROLefrsEcHJ@r13-u19.micron.com> <ZV6Uhsg6WLBtNqU3@memverge.com>
In-Reply-To: <ZV6Uhsg6WLBtNqU3@memverge.com>
From:   Vinicius Petrucci <vpetrucci@gmail.com>
Date:   Thu, 23 Nov 2023 09:21:08 -0600
Message-ID: <CAEZ6=UNLuOYom1Qng28F2y6XocJM4cnbDG1yq3m1p8btuQ4tRQ@mail.gmail.com>
Subject: Re: [RFC PATCH] mm/mbind: Introduce process_mbind() syscall for
 external memory binding
To:     Gregory Price <gregory.price@memverge.com>, linux-mm@kvack.org
Cc:     akpm@linux-foundation.org, linux-cxl@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org,
        linux-api@vger.kernel.org, minchan@kernel.org,
        dave.hansen@linux.intel.com, x86@kernel.org,
        Jonathan.Cameron@huawei.com, aneesh.kumar@linux.ibm.com,
        ying.huang@intel.com, dan.j.williams@intel.com,
        hezhongkun.hzk@bytedance.com, fvdl@google.com, surenb@google.com,
        rientjes@google.com, hannes@cmpxchg.org, mhocko@suse.com,
        Hasan.Maruf@amd.com, jgroves@micron.com, ravis.opensrc@micron.com,
        sthanneeru@micron.com, emirakhur@micron.com,
        vtavarespetr@micron.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Greg!

Thanks a lot for quickly looking into this and sharing your notes here.

On Wed, Nov 22, 2023 at 5:53=E2=80=AFPM Gregory Price
<gregory.price@memverge.com> wrote:
>
> > Please note the initial `maxnode` parameter from `mbind` was omitted
> > to ensure the API doesn't exceed 6 arguments. Instead, the constant
> > MAX_NUMNODES was utilized.
> >
>
> I don't think this will work, users have traditionally been allowed to
> shorten their nodemasks, and also for some level of portability.
>
> We may want to consider an arg structure, rather than just chopping an
> argument off.
>

Yes, good point... that should be considered as a more complete
long-term approach beyond the MVP.

> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index 10a590ee1c89..91ee300fa728 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -1215,11 +1215,10 @@ static struct folio *alloc_migration_target_by_=
mpol(struct folio *src,
> >  }
> >  #endif
> >
> > -static long do_mbind(unsigned long start, unsigned long len,
> > +static long do_mbind(struct mm_struct *mm, unsigned long start, unsign=
ed long len,
> >                    unsigned short mode, unsigned short mode_flags,
> >                    nodemask_t *nmask, unsigned long flags)
> >  {
> > -     struct mm_struct *mm =3D current->mm;
> >       struct vm_area_struct *vma, *prev;
> >       struct vma_iterator vmi;
> >       struct migration_mpol mmpol;
> > @@ -1465,10 +1464,84 @@ static inline int sanitize_mpol_flags(int *mode=
, unsigned short *flags)
> >       return 0;
> >  }
>
> This is a completely insufficient change to do_mbind.  do_mbind utilizes
> `current` in a variety of places for nodemask (cpuset) validation and to
> acquire the task's lock.  This will not work the way you intend it to,
> you end up mixing up node masks between current and target task.
>

Oh oh. True! Good catch!

> see here:
> https://lore.kernel.org/all/20231122211200.31620-7-gregory.price@memverge=
.com/
>

Let me go over this... Thanks!

> We may want to combine this change and with my change so that your iovec
> changes can be re-used, because that is a very nice feature.
>

Sounds good. Thanks again!

Best,

Vinicius