MADV_DONTNEED[_LOCKED] and MADV_FREE internal logics for
[process_]madvise() can be invoked with batched tlb flushes. Update
vector_madvise() and do_madvise(), which are called for the two system
calls respectively, to use those in the efficient way. Initialize an
mmu_gather object before starting the internal works, and flush the
gathered tlb entries at once after all the internal works are done.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
mm/madvise.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 47 insertions(+), 4 deletions(-)
diff --git a/mm/madvise.c b/mm/madvise.c
index d7ea71c6422c..d5f4ce3041a4 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -905,6 +905,7 @@ static bool madvise_dontneed_free_valid_vma(struct vm_area_struct *vma,
struct madvise_behavior {
int behavior;
+ struct mmu_gather *tlb;
};
static long madvise_dontneed_free(struct vm_area_struct *vma,
@@ -964,9 +965,11 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
}
if (behavior == MADV_DONTNEED || behavior == MADV_DONTNEED_LOCKED)
- return madvise_dontneed_single_vma(NULL, vma, start, end);
+ return madvise_dontneed_single_vma(
+ madv_behavior->tlb, vma, start, end);
else if (behavior == MADV_FREE)
- return madvise_free_single_vma(NULL, vma, start, end);
+ return madvise_free_single_vma(
+ madv_behavior->tlb, vma, start, end);
else
return -EINVAL;
}
@@ -1639,6 +1642,32 @@ static void madvise_unlock(struct mm_struct *mm, int behavior)
mmap_read_unlock(mm);
}
+static bool madvise_batch_tlb_flush(int behavior)
+{
+ switch (behavior) {
+ case MADV_DONTNEED:
+ case MADV_DONTNEED_LOCKED:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static void madvise_init_tlb(struct madvise_behavior *madv_behavior,
+ struct mm_struct *mm)
+{
+ if (!madvise_batch_tlb_flush(madv_behavior->behavior))
+ return;
+ tlb_gather_mmu(madv_behavior->tlb, mm);
+}
+
+static void madvise_finish_tlb(struct madvise_behavior *madv_behavior)
+{
+ if (!madvise_batch_tlb_flush(madv_behavior->behavior))
+ return;
+ tlb_finish_mmu(madv_behavior->tlb);
+}
+
static bool is_valid_madvise(unsigned long start, size_t len_in, int behavior)
{
size_t len;
@@ -1791,14 +1820,20 @@ 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_behavior madv_behavior = {.behavior = behavior};
+ struct mmu_gather tlb;
+ struct madvise_behavior madv_behavior = {
+ .behavior = behavior,
+ .tlb = &tlb,
+ };
if (madvise_should_skip(start, len_in, behavior, &error))
return error;
error = madvise_lock(mm, behavior);
if (error)
return error;
+ madvise_init_tlb(&madv_behavior, mm);
error = madvise_do_behavior(mm, start, len_in, &madv_behavior);
+ madvise_finish_tlb(&madv_behavior);
madvise_unlock(mm, behavior);
return error;
@@ -1815,13 +1850,18 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
{
ssize_t ret = 0;
size_t total_len;
- struct madvise_behavior madv_behavior = {.behavior = behavior};
+ struct mmu_gather tlb;
+ struct madvise_behavior madv_behavior = {
+ .behavior = behavior,
+ .tlb = &tlb,
+ };
total_len = iov_iter_count(iter);
ret = madvise_lock(mm, behavior);
if (ret)
return ret;
+ madvise_init_tlb(&madv_behavior, mm);
while (iov_iter_count(iter)) {
unsigned long start = (unsigned long)iter_iov_addr(iter);
@@ -1850,14 +1890,17 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
}
/* Drop and reacquire lock to unwind race. */
+ madvise_finish_tlb(&madv_behavior);
madvise_unlock(mm, behavior);
madvise_lock(mm, behavior);
+ madvise_init_tlb(&madv_behavior, mm);
continue;
}
if (ret < 0)
break;
iov_iter_advance(iter, iter_iov_len(iter));
}
+ madvise_finish_tlb(&madv_behavior);
madvise_unlock(mm, behavior);
ret = (total_len - iov_iter_count(iter)) ? : ret;
--
2.39.5On Mon, 10 Mar 2025 10:23:17 -0700 SeongJae Park <sj@kernel.org> wrote:
> MADV_DONTNEED[_LOCKED] and MADV_FREE internal logics for
> [process_]madvise() can be invoked with batched tlb flushes. Update
> vector_madvise() and do_madvise(), which are called for the two system
> calls respectively, to use those in the efficient way. Initialize an
> mmu_gather object before starting the internal works, and flush the
> gathered tlb entries at once after all the internal works are done.
>
> Signed-off-by: SeongJae Park <sj@kernel.org>
> ---
> mm/madvise.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index d7ea71c6422c..d5f4ce3041a4 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
[...]
> @@ -1639,6 +1642,32 @@ static void madvise_unlock(struct mm_struct *mm, int behavior)
> mmap_read_unlock(mm);
> }
>
> +static bool madvise_batch_tlb_flush(int behavior)
> +{
> + switch (behavior) {
> + case MADV_DONTNEED:
> + case MADV_DONTNEED_LOCKED:
> + return true;
> + default:
> + return false;
> + }
> +}
The above function should return true for MADV_FREE, too. I will make so in the
next spin.
Thanks,
SJ
[...]
+cc Rik on this, as he's working on TLB flush-related stuff. Maybe worth
cc-ing him on series respins too? Unless Rik objects of course :P
Again, nit, but your subject line/first line of commit message is
definitely too long here! :)
On Mon, Mar 10, 2025 at 10:23:17AM -0700, SeongJae Park wrote:
> MADV_DONTNEED[_LOCKED] and MADV_FREE internal logics for
> [process_]madvise() can be invoked with batched tlb flushes. Update
> vector_madvise() and do_madvise(), which are called for the two system
> calls respectively, to use those in the efficient way. Initialize an
> mmu_gather object before starting the internal works, and flush the
> gathered tlb entries at once after all the internal works are done.
super nit but logics -> logic and works -> work :)
I think we need more here as to why you're restricting to
MADV_DONTNEED_LOCKED and MADV_FREE. I see pageout initialises a tlb gather
object, so does cold, etc. etc.?
>
> Signed-off-by: SeongJae Park <sj@kernel.org>
This is really nice, I love how we're able to evolve this towards batching
flushes.
Overall though I'd like you to address some of the concerns here before
giving tags... :)
> ---
> mm/madvise.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index d7ea71c6422c..d5f4ce3041a4 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -905,6 +905,7 @@ static bool madvise_dontneed_free_valid_vma(struct vm_area_struct *vma,
>
> struct madvise_behavior {
> int behavior;
> + struct mmu_gather *tlb;
> };
Aha! Good :)
I see in 9/9 you actually pull the caller_tlb stuff out, I still feel like
we should be threading this state through further, if possible, rather than
passing in behavior->tlb as a parameter.
But this is nitty I suppose!
>
> static long madvise_dontneed_free(struct vm_area_struct *vma,
> @@ -964,9 +965,11 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
> }
>
> if (behavior == MADV_DONTNEED || behavior == MADV_DONTNEED_LOCKED)
> - return madvise_dontneed_single_vma(NULL, vma, start, end);
> + return madvise_dontneed_single_vma(
> + madv_behavior->tlb, vma, start, end);
> else if (behavior == MADV_FREE)
> - return madvise_free_single_vma(NULL, vma, start, end);
> + return madvise_free_single_vma(
> + madv_behavior->tlb, vma, start, end);
Yeah as I said above be nice to just pass madv_behavior, makes things more
flexible to pass a pointer to the helper struct through, as you can
> else
> return -EINVAL;
> }
> @@ -1639,6 +1642,32 @@ static void madvise_unlock(struct mm_struct *mm, int behavior)
> mmap_read_unlock(mm);
> }
>
> +static bool madvise_batch_tlb_flush(int behavior)
> +{
> + switch (behavior) {
> + case MADV_DONTNEED:
> + case MADV_DONTNEED_LOCKED:
> + return true;
> + default:
> + return false;
> + }
> +}
I kind of hate this madvise_ prefix stuff, like we're in mm/madvise.c, it's
pretty obvious static functions are related to madvise :) but this is a
pre-existing thing, not your fault, and it's actually right to maintain
consistency with this.
So this is purely a whine that can be >/dev/null.
> +
> +static void madvise_init_tlb(struct madvise_behavior *madv_behavior,
> + struct mm_struct *mm)
> +{
> + if (!madvise_batch_tlb_flush(madv_behavior->behavior))
> + return;
> + tlb_gather_mmu(madv_behavior->tlb, mm);
> +}
> +
> +static void madvise_finish_tlb(struct madvise_behavior *madv_behavior)
> +{
> + if (!madvise_batch_tlb_flush(madv_behavior->behavior))
> + return;
> + tlb_finish_mmu(madv_behavior->tlb);
> +}
> +
Nitty, but for both of these, usually I like the guard clause pattern, but
since it's such a trivial thing I think it reads better as:
if (madvise_batch_tlb_flush(madv_behavior->behavior))
tlb_gather_mmu(madv_behavior->tlb, mm);
and:
if (madvise_batch_tlb_flush(madv_behavior->behavior))
tlb_finish_mmu(madv_behavior->tlb);
> static bool is_valid_madvise(unsigned long start, size_t len_in, int behavior)
> {
> size_t len;
> @@ -1791,14 +1820,20 @@ 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_behavior madv_behavior = {.behavior = behavior};
> + struct mmu_gather tlb;
> + struct madvise_behavior madv_behavior = {
> + .behavior = behavior,
> + .tlb = &tlb,
> + };
>
> if (madvise_should_skip(start, len_in, behavior, &error))
> return error;
> error = madvise_lock(mm, behavior);
> if (error)
> return error;
> + madvise_init_tlb(&madv_behavior, mm);
> error = madvise_do_behavior(mm, start, len_in, &madv_behavior);
> + madvise_finish_tlb(&madv_behavior);
> madvise_unlock(mm, behavior);
>
> return error;
> @@ -1815,13 +1850,18 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
> {
> ssize_t ret = 0;
> size_t total_len;
> - struct madvise_behavior madv_behavior = {.behavior = behavior};
> + struct mmu_gather tlb;
> + struct madvise_behavior madv_behavior = {
> + .behavior = behavior,
> + .tlb = &tlb,
> + };
>
> total_len = iov_iter_count(iter);
>
> ret = madvise_lock(mm, behavior);
> if (ret)
> return ret;
> + madvise_init_tlb(&madv_behavior, mm);
>
> while (iov_iter_count(iter)) {
> unsigned long start = (unsigned long)iter_iov_addr(iter);
> @@ -1850,14 +1890,17 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
> }
>
> /* Drop and reacquire lock to unwind race. */
> + madvise_finish_tlb(&madv_behavior);
> madvise_unlock(mm, behavior);
> madvise_lock(mm, behavior);
> + madvise_init_tlb(&madv_behavior, mm);
Fun, but I guess necessary. I strongly suspect this code path will never
actually happen, but we need to account for it.
> continue;
> }
> if (ret < 0)
> break;
> iov_iter_advance(iter, iter_iov_len(iter));
> }
> + madvise_finish_tlb(&madv_behavior);
> madvise_unlock(mm, behavior);
>
> ret = (total_len - iov_iter_count(iter)) ? : ret;
> --
> 2.39.5
On Tue, 11 Mar 2025 13:59:10 +0000 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> +cc Rik on this, as he's working on TLB flush-related stuff. Maybe worth
> cc-ing him on series respins too? Unless Rik objects of course :P
>
> Again, nit, but your subject line/first line of commit message is
> definitely too long here! :)
I will reduce.
>
> On Mon, Mar 10, 2025 at 10:23:17AM -0700, SeongJae Park wrote:
> > MADV_DONTNEED[_LOCKED] and MADV_FREE internal logics for
> > [process_]madvise() can be invoked with batched tlb flushes. Update
> > vector_madvise() and do_madvise(), which are called for the two system
> > calls respectively, to use those in the efficient way. Initialize an
> > mmu_gather object before starting the internal works, and flush the
> > gathered tlb entries at once after all the internal works are done.
>
> super nit but logics -> logic and works -> work :)
>
> I think we need more here as to why you're restricting to
> MADV_DONTNEED_LOCKED and MADV_FREE. I see pageout initialises a tlb gather
> object, so does cold, etc. etc.?
Good point. I'm just trying to start from small things. I will clarify this
on the next spin.
>
> >
> > Signed-off-by: SeongJae Park <sj@kernel.org>
>
> This is really nice, I love how we're able to evolve this towards batching
> flushes.
>
> Overall though I'd like you to address some of the concerns here before
> giving tags... :)
Thank you for nice comments! :)
>
> > ---
> > mm/madvise.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 47 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index d7ea71c6422c..d5f4ce3041a4 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -905,6 +905,7 @@ static bool madvise_dontneed_free_valid_vma(struct vm_area_struct *vma,
> >
> > struct madvise_behavior {
> > int behavior;
> > + struct mmu_gather *tlb;
> > };
>
> Aha! Good :)
>
> I see in 9/9 you actually pull the caller_tlb stuff out, I still feel like
> we should be threading this state through further, if possible, rather than
> passing in behavior->tlb as a parameter.
Yes, I will do so.
>
> But this is nitty I suppose!
>
> >
> > static long madvise_dontneed_free(struct vm_area_struct *vma,
> > @@ -964,9 +965,11 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
> > }
> >
> > if (behavior == MADV_DONTNEED || behavior == MADV_DONTNEED_LOCKED)
> > - return madvise_dontneed_single_vma(NULL, vma, start, end);
> > + return madvise_dontneed_single_vma(
> > + madv_behavior->tlb, vma, start, end);
> > else if (behavior == MADV_FREE)
> > - return madvise_free_single_vma(NULL, vma, start, end);
> > + return madvise_free_single_vma(
> > + madv_behavior->tlb, vma, start, end);
>
> Yeah as I said above be nice to just pass madv_behavior, makes things more
> flexible to pass a pointer to the helper struct through, as you can
Yes.
>
> > else
> > return -EINVAL;
> > }
> > @@ -1639,6 +1642,32 @@ static void madvise_unlock(struct mm_struct *mm, int behavior)
> > mmap_read_unlock(mm);
> > }
> >
> > +static bool madvise_batch_tlb_flush(int behavior)
> > +{
> > + switch (behavior) {
> > + case MADV_DONTNEED:
> > + case MADV_DONTNEED_LOCKED:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
>
> I kind of hate this madvise_ prefix stuff, like we're in mm/madvise.c, it's
> pretty obvious static functions are related to madvise :) but this is a
> pre-existing thing, not your fault, and it's actually right to maintain
> consistency with this.
>
> So this is purely a whine that can be >/dev/null.
Thank you for understanding :)
>
> > +
> > +static void madvise_init_tlb(struct madvise_behavior *madv_behavior,
> > + struct mm_struct *mm)
> > +{
> > + if (!madvise_batch_tlb_flush(madv_behavior->behavior))
> > + return;
> > + tlb_gather_mmu(madv_behavior->tlb, mm);
> > +}
> > +
> > +static void madvise_finish_tlb(struct madvise_behavior *madv_behavior)
> > +{
> > + if (!madvise_batch_tlb_flush(madv_behavior->behavior))
> > + return;
> > + tlb_finish_mmu(madv_behavior->tlb);
> > +}
> > +
>
> Nitty, but for both of these, usually I like the guard clause pattern, but
> since it's such a trivial thing I think it reads better as:
>
> if (madvise_batch_tlb_flush(madv_behavior->behavior))
> tlb_gather_mmu(madv_behavior->tlb, mm);
>
> and:
>
> if (madvise_batch_tlb_flush(madv_behavior->behavior))
> tlb_finish_mmu(madv_behavior->tlb);
Totally agreed, thank you for catching this.
Thanks,
SJ
[...]
© 2016 - 2026 Red Hat, Inc.