Re: [PATCH 2/5] mm/madvise: thread mm_struct through madvise_behavior

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

On Thu, Jun 19, 2025 at 09:40:34PM -0400, Zi Yan wrote:
> On 19 Jun 2025, at 16:26, Lorenzo Stoakes wrote:
>
> > There's no need to thread a pointer to the mm_struct nor have different
> > functions signatures for each behaviour, instead store state in the struct
> > madvise_behavior object consistently and use it for all madvise() actions.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> >  mm/madvise.c | 105 ++++++++++++++++++++++++++-------------------------
> >  1 file changed, 54 insertions(+), 51 deletions(-)
> >
>
> <snip>
>
> > @@ -1422,15 +1425,14 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> >  /*
> >   * Error injection support for memory error handling.
> >   */
> > -static int madvise_inject_error(int behavior,
> > -		unsigned long start, unsigned long end)
> > +static int madvise_inject_error(unsigned long start, unsigned long end,
> > +		struct madvise_behavior *madv_behavior)
> >  {
> >  	unsigned long size;
> >
> >  	if (!capable(CAP_SYS_ADMIN))
> >  		return -EPERM;
> >
> > -
> >  	for (; start < end; start += size) {
> >  		unsigned long pfn;
> >  		struct page *page;
> > @@ -1448,7 +1450,7 @@ static int madvise_inject_error(int behavior,
> >  		 */
> >  		size = page_size(compound_head(page));
> >
> > -		if (behavior == MADV_SOFT_OFFLINE) {
> > +		if (madv_behavior->behavior == MADV_SOFT_OFFLINE) {
> >  			pr_info("Soft offlining pfn %#lx at process virtual address %#lx\n",
> >  				 pfn, start);
> >  			ret = soft_offline_page(pfn, MF_COUNT_INCREASED);
> > @@ -1467,9 +1469,9 @@ static int madvise_inject_error(int behavior,
> >  	return 0;
> >  }
> >
>
> Is this necessary? madvise_inject_error() only cares about behavior.

As you notice in a subsequent patch, we go further. It's also useful
signature-wise for all functions to have the exact same signature for
consistency, something sorely lacking in madvise.c for some time.

This is covered under the 'nor have different functions signatures for each
behaviour' bit in the commit message :)

>
> > -static bool is_memory_failure(int behavior)
> > +static bool is_memory_failure(struct madvise_behavior *madv_behavior)
> >  {
> > -	switch (behavior) {
> > +	switch (madv_behavior->behavior) {
> >  	case MADV_HWPOISON:
> >  	case MADV_SOFT_OFFLINE:
> >  		return true;
> > @@ -1480,13 +1482,13 @@ static bool is_memory_failure(int behavior)
> >
> >  #else
> >
> > -static int madvise_inject_error(int behavior,
> > -		unsigned long start, unsigned long end)
> > +static int madvise_inject_error(unsigned long start, unsigned long end,
> > +		struct madvise_behavior *madv_behavior)
> >  {
> >  	return 0;
> >  }
> >
> > -static bool is_memory_failure(int behavior)
> > +static bool is_memory_failure(struct madvise_behavior *madv_behavior)
> >  {
> >  	return false;
> >  }
>
> Same here. Your is_anon_vma_name() still takes int behavior, why
> would is_memory_failure() take struct madvise_behavior?

See above.

>
> <snip>
>
> > -static bool is_madvise_populate(int behavior)
> > +static bool is_madvise_populate(struct madvise_behavior *madv_behavior)
> >  {
> > -	switch (behavior) {
> > +	switch (madv_behavior->behavior) {
> >  	case MADV_POPULATE_READ:
> >  	case MADV_POPULATE_WRITE:
> >  		return true;
>
> Ditto.

Doing it this way makes life easier (esp later) and is more consistent with
other invocations.

>
> The rest looks good to me.

Thanks!

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