Re: [PATCH 3/5] mm/madvise: thread VMA range state through madvise_behavior

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Thu, Jun 19, 2025 at 09:54:11PM -0400, Zi Yan wrote:
> On 19 Jun 2025, at 16:26, Lorenzo Stoakes wrote:
>
> > Rather than updating start and a confusing local parameter 'tmp' in
> > madvise_walk_vmas(), instead store the current range being operated upon in
> > the struct madvise_behavior helper object in a range pair and use this
> > consistently in all operations.
> >
> > This makes it clearer what is going on and opens the door to further
> > cleanup now we store state regarding what is currently being operated upon
> > here.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> >  mm/madvise.c | 101 ++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 55 insertions(+), 46 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 47485653c2a1..6faa38b92111 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -58,17 +58,26 @@ enum madvise_lock_mode {
> >  	MADVISE_VMA_READ_LOCK,
> >  };
> >
> > +struct madvise_behavior_range {
> > +	unsigned long start, end;
> > +};
> > +
>
> Declare members separately?

Can do, but this is one of those subject things where everyone has different
views, if I did it the other way no doubt somebody else would comment about
declaring together :P

I think as a range here it's not a big deal unless you feel strongly about it?

>
> <snip>
>
> > @@ -1425,10 +1437,11 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> >  /*
> >   * Error injection support for memory error handling.
> >   */
> > -static int madvise_inject_error(unsigned long start, unsigned long end,
> > -		struct madvise_behavior *madv_behavior)
> > +static int madvise_inject_error(struct madvise_behavior *madv_behavior)
> >  {
> >  	unsigned long size;
> > +	unsigned long start = madv_behavior->range.start;
> > +	unsigned long end = madv_behavior->range.end;
> >
> >  	if (!capable(CAP_SYS_ADMIN))
> >  		return -EPERM;
> > @@ -1482,8 +1495,7 @@ static bool is_memory_failure(struct madvise_behavior *madv_behavior)
> >
> >  #else
> >
> > -static int madvise_inject_error(unsigned long start, unsigned long end,
> > -		struct madvise_behavior *madv_behavior)
> > +static int madvise_inject_error(struct madvise_behavior *madv_behavior)
> >  {
> >  	return 0;
> >  }
>
> OK, now I get why you pass struct madvise_behavior to madvise_inject_error()
> in Patch 2. The changes make sense to me now. Maybe delay that conversation
> in this one.

I think it's valuable there because otherwise all the function invocations were
inconsistent, but after 2/5 become completely consistent. I mention this in the
commit message and I think it's valuable so you're not doing:

if (foo)
	bar(x, y, z)

if (blah)
	baz(y, x, z)

etc.

When you quickly read through it's easy to get confused/lost as to what's going
on, whereas if they all have the same signatures it's very clear you're
offloading the heavy lifting to each function.

>
>
>
> > @@ -1565,20 +1577,20 @@ static bool process_madvise_remote_valid(int behavior)
> >   * If a VMA read lock could not be acquired, we return NULL and expect caller to
> >   * fallback to mmap lock behaviour.
> >   */
> > -static struct vm_area_struct *try_vma_read_lock(struct mm_struct *mm,
> > -		struct madvise_behavior *madv_behavior,
> > -		unsigned long start, unsigned long end)
> > +static
> > +struct vm_area_struct *try_vma_read_lock(struct madvise_behavior *madv_behavior)
> >  {
> > +	struct mm_struct *mm = madv_behavior->mm;
>
> Is the struct mm_struct removal missed in Patch 2?

Yeah, I will go back and put it in on respin.

>
>
> <snip>
>
> > @@ -1846,22 +1854,23 @@ static int madvise_do_behavior(unsigned long start, size_t len_in,
> >  		struct madvise_behavior *madv_behavior)
> >  {
> >  	struct blk_plug plug;
> > -	unsigned long end;
> >  	int error;
> > +	struct madvise_behavior_range *range = &madv_behavior->range;
> >
> >  	if (is_memory_failure(madv_behavior)) {
> > -		end = start + len_in;
> > -		return madvise_inject_error(start, end, madv_behavior);
> > +		range->start = start;
> > +		range->end = start + len_in;
> > +		return madvise_inject_error(madv_behavior);
> >  	}
> >
> > -	start = get_untagged_addr(madv_behavior->mm, start);
> > -	end = start + PAGE_ALIGN(len_in);
> > +	range->start = get_untagged_addr(madv_behavior->mm, start);
> > +	range->end = range->start + PAGE_ALIGN(len_in);
> >
> >  	blk_start_plug(&plug);
> >  	if (is_madvise_populate(madv_behavior))
> > -		error = madvise_populate(start, end, madv_behavior);
> > +		error = madvise_populate(madv_behavior);
> >  	else
> > -		error = madvise_walk_vmas(start, end, madv_behavior);
> > +		error = madvise_walk_vmas(madv_behavior);
> >  	blk_finish_plug(&plug);
> >  	return error;
> >  }
>
> We almost can pass just struct madvise_behavior to madvise_do_behavior().
> I wonder why memory_failure behaves differently.

There's complexity around the start, end stuff (Barry bumped into some of this)
and I don't want to mess with that in this series. This series is meant to have
no functional changes.

>
> --
> Best Regards,
> Yan, Zi