On Tue, Feb 11, 2025 at 10:20:53AM -0800, SeongJae Park wrote: > On Tue, 11 Feb 2025 10:51:41 +0000 Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> wrote: > > > +cc Naresh, Arnd for another reports/discussion of the same issue [0] while > > lore/lei is broken. > > > > Hi, > > > > Lore breaking means I missed this :) thankfully you cc'd me (_this_ is why I am > > so adament about people following get_maintainer.pl procedure btw) so I was able > > to now notice + reply :) > > > > This is totally my bad for missing this on review, so mea culpa. > > No worry, your reviews are always very helpful! Very kind of you to say :>) > > > > > [0]:https://lwn.net/ml/linux-mm/CA+G9fYt5QwJ4_F8fJj7jx9_0Le9kOVSeG38ox9qnKqwsrDdvHQ@xxxxxxxxxxxxxx/ > > > > On Mon, Feb 10, 2025 at 10:32:01PM -0800, SeongJae Park wrote: > > > madvise_lock() does nothing for MADV_HWPOSION and MADV_SOFT_OFFLINE > > > behavior, but madvise_unlock() does mmap_lock unlocking regardless of > > > the behavior. > [...] > > > mm/madvise.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/mm/madvise.c b/mm/madvise.c > > > index b5ef8e03d8b0..b8969457f3ef 100644 > > > --- a/mm/madvise.c > > > +++ b/mm/madvise.c > > > @@ -1577,7 +1577,6 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start, > > > > > > static int madvise_lock(struct mm_struct *mm, int behavior) > > > { > > > - > > > #ifdef CONFIG_MEMORY_FAILURE > > > if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE) > > > return 0; > > > @@ -1595,6 +1594,11 @@ static int madvise_lock(struct mm_struct *mm, int behavior) > > > > > > static void madvise_unlock(struct mm_struct *mm, int behavior) > > > { > > > +#ifdef CONFIG_MEMORY_FAILURE > > > + if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE) > > > + return; > > > +#endif > > > > I agree this fixes the issue but this is horrible. let's abstract this please > > rather than doing the same crap that already existed, only now twice. > > I agree abstracting this is a better idea. Thanks! > > > > > > + > > > if (madvise_need_mmap_write(behavior)) > > > mmap_write_unlock(mm); > > > else > > > > > > base-commit: 8bf30f9d23eb5040d37e6e712789cee8e71e1577 > > > -- > > > 2.39.5 > > > > I attach a fix-patch concept for something I think that'd be nicer, do with > > it what thy wilt! :P sorry I don't mean to be 'one of those' maintainers > > who copy/pastes code + demands somebody do it (by no means do I do so), but > > since this is so small I feel it's kind of quicker for me to do it this > > way. > > > > Obviously take it or leave it/adapt it/etc. This is compile-tested only... > > I further ran the repro program and confirmed this fixes the issue :) Great, perfect! > > > > > ----8<---- > > From 9fce3e47bf0fff2a2291be66002af346cdbca665 Mon Sep 17 00:00:00 2001 > > From: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> > > Date: Tue, 11 Feb 2025 10:44:26 +0000 > > Subject: [PATCH] mm/madvise: fix madvise_[un]lock() issue > > > > We are asymmetric in our locking/unlocking in the case of memory failure > > madvise() behaviour options, correct this and abstract the memory failure > > check. > > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> > > Reviewed-by: SeongJae Park <sj@xxxxxxxxxx> > Tested-by: SeongJae Park <sj@xxxxxxxxxx> Thanks :) I accidentally seem to have included some whitespace errors which Andrew kindly fixed up :>) so apologies for that! > > > Thanks, > SJ > > [...] Cheers, Lorenzo
