Re: [RFC PATCH 05/16] mm/madvise: define and use madvise_behavior struct for madvise_do_behavior()

From: Shakeel Butt
Date: Wed Mar 05 2025 - 16:43:06 EST

On Wed, Mar 05, 2025 at 01:02:15PM -0800, Shakeel Butt wrote:
> On Wed, Mar 05, 2025 at 10:16:00AM -0800, SeongJae Park wrote:
> > To implement batched tlb flushes for MADV_DONTNEED[_LOCKED] and
> > MADV_FREE, an mmu_gather object in addition to the behavior integer need
> > to be passed to the internal logics. Define a struct for passing such
> > information together with the behavior value without increasing number
> > of parameters of all code paths towards the internal logic.
> >
> > Signed-off-by: SeongJae Park <sj@xxxxxxxxxx>
> > ---
> > mm/madvise.c | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index c5e1a4d1df72..3346e593e07d 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -1665,9 +1665,15 @@ static bool is_memory_populate(int behavior)
> > }
> > }
> >
> > +struct madvise_behavior {
> > + int behavior;
> > +};
>
> I think the patch 5 to 8 are just causing churn and will be much better
> to be a single patch. Also why not make the above struct a general
> madvise visit param struct which can be used by both
> madvise_vma_anon_name() and madvise_vma_behavior()?

Something like following:

diff --git a/mm/madvise.c b/mm/madvise.c
index c5e1a4d1df72..cbc3817366a6 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -890,11 +890,17 @@ static bool madvise_dontneed_free_valid_vma(struct vm_area_struct *vma,
return true;
}

+struct madvise_walk_param {
+ int behavior;
+ struct anon_vma_name *anon_name;
+};
+
static long madvise_dontneed_free(struct vm_area_struct *vma,
struct vm_area_struct **prev,
unsigned long start, unsigned long end,
- int behavior)
+ struct madvise_walk_param *arg)
{
+ int behavior = arg->behavior;
struct mm_struct *mm = vma->vm_mm;

*prev = vma;
@@ -1249,8 +1255,9 @@ static long madvise_guard_remove(struct vm_area_struct *vma,
static int madvise_vma_behavior(struct vm_area_struct *vma,
struct vm_area_struct **prev,
unsigned long start, unsigned long end,
- unsigned long behavior)
+ struct madvise_walk_param *arg)
{
+ int behavior = arg->behavior;
int error;
struct anon_vma_name *anon_name;
unsigned long new_flags = vma->vm_flags;
@@ -1270,7 +1277,7 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
case MADV_FREE:
case MADV_DONTNEED:
case MADV_DONTNEED_LOCKED:
- return madvise_dontneed_free(vma, prev, start, end, behavior);
+ return madvise_dontneed_free(vma, prev, start, end, arg);
case MADV_NORMAL:
new_flags = new_flags & ~VM_RAND_READ & ~VM_SEQ_READ;
break;
@@ -1462,10 +1469,10 @@ static bool process_madvise_remote_valid(int behavior)
*/
static
int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
- unsigned long end, unsigned long arg,
+ unsigned long end, struct madvise_walk_param *arg,
int (*visit)(struct vm_area_struct *vma,
struct vm_area_struct **prev, unsigned long start,
- unsigned long end, unsigned long arg))
+ unsigned long end, struct madvise_walk_param *arg))
{
struct vm_area_struct *vma;
struct vm_area_struct *prev;
@@ -1523,7 +1530,7 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
static int madvise_vma_anon_name(struct vm_area_struct *vma,
struct vm_area_struct **prev,
unsigned long start, unsigned long end,
- unsigned long anon_name)
+ struct madvise_walk_param *arg)
{
int error;

@@ -1532,7 +1539,7 @@ static int madvise_vma_anon_name(struct vm_area_struct *vma,
return -EBADF;

error = madvise_update_vma(vma, prev, start, end, vma->vm_flags,
- (struct anon_vma_name *)anon_name);
+ arg->anon_name);

/*
* madvise() returns EAGAIN if kernel resources, such as
@@ -1548,6 +1555,7 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
{
unsigned long end;
unsigned long len;
+ struct madvise_walk_param param = { .anon_name = anon_name };

if (start & ~PAGE_MASK)
return -EINVAL;
@@ -1564,7 +1572,7 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
if (end == start)
return 0;

- return madvise_walk_vmas(mm, start, end, (unsigned long)anon_name,
+ return madvise_walk_vmas(mm, start, end, &param,
madvise_vma_anon_name);
}
#endif /* CONFIG_ANON_VMA_NAME */
@@ -1666,8 +1674,10 @@ static bool is_memory_populate(int behavior)
}

static int madvise_do_behavior(struct mm_struct *mm,
- unsigned long start, size_t len_in, int behavior)
+ unsigned long start, size_t len_in,
+ struct madvise_walk_param *arg)
{
+ int behavior = arg->behavior;
struct blk_plug plug;
unsigned long end;
int error;
@@ -1681,7 +1691,7 @@ static int madvise_do_behavior(struct mm_struct *mm,
if (is_memory_populate(behavior))
error = madvise_populate(mm, start, end, behavior);
else
- error = madvise_walk_vmas(mm, start, end, behavior,
+ error = madvise_walk_vmas(mm, start, end, arg,
madvise_vma_behavior);
blk_finish_plug(&plug);
return error;
@@ -1762,13 +1772,14 @@ static int madvise_do_behavior(struct mm_struct *mm,
int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int behavior)
{
int error;
+ struct madvise_walk_param arg = {.behavior = behavior};

if (madvise_should_skip(start, len_in, behavior, &error))
return error;
error = madvise_lock(mm, behavior);
if (error)
return error;
- error = madvise_do_behavior(mm, start, len_in, behavior);
+ error = madvise_do_behavior(mm, start, len_in, &arg);
madvise_unlock(mm, behavior);

return error;
@@ -1785,6 +1796,7 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
{
ssize_t ret = 0;
size_t total_len;
+ struct madvise_walk_param arg = {.behavior = behavior};

total_len = iov_iter_count(iter);

@@ -1800,7 +1812,7 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
if (madvise_should_skip(start, len_in, behavior, &error))
ret = error;
else
- ret = madvise_do_behavior(mm, start, len_in, behavior);
+ ret = madvise_do_behavior(mm, start, len_in, &arg);
/*
* An madvise operation is attempting to restart the syscall,
* but we cannot proceed as it would not be correct to repeat
--
2.43.5