Re: [PATCH v3] mm: use per_vma lock for MADV_DONTNEED

[Barry Song <21cnbao@gmail.com>]

> Sort of a nice-to-have/thought but:
>
> Actually, when I proposed the refactor I wondered whether we'd use more state in
> madv_behaviour here but turns out we don't so we may as well just switch back to
> using int behavior here?
>
> If we do that then we can adjust process_madvise_remote_valid() with:
>
>  static bool process_madvise_remote_valid(int behavior)
>  {
> +       /* Due to lack of address untag atomicity, we need mmap lock. */
> +       VM_WARN_ON_ONCE(madvise_lock(behavior) != MADVISE_VMA_READ_LOCK);


process_madvise_remote_valid() is called before vector_madvise(), so I'm not
sure what this code is supposed to do. Are you trying to do something like:

VM_WARN_ON_ONCE(get_lock_mode(behavior) == MADVISE_VMA_READ_LOCK);

If so, that seems problematic — the same madvise operation might be allowed
to use the per-VMA lock for local processes, but disallowed for remote ones.

I suppose this will only start to make sense after we support per-VMA locking
for remote madvise operations such as "case MADV_XXX":

diff --git a/mm/madvise.c b/mm/madvise.c
index 8382614b71d1..9815445284d5 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1641,7 +1641,8 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
  * take mmap_lock for writing. Others, which simply traverse vmas, need
  * to only take it for reading.
  */
-static enum madvise_lock_mode get_lock_mode(struct madvise_behavior *madv_behavior)
+static enum madvise_lock_mode get_lock_mode(struct mm_struct *mm,
+		struct madvise_behavior *madv_behavior)
 {
 	int behavior = madv_behavior->behavior;
 
@@ -1659,6 +1660,9 @@ static enum madvise_lock_mode get_lock_mode(struct madvise_behavior *madv_behavi
 	case MADV_COLLAPSE:
 	case MADV_GUARD_INSTALL:
 	case MADV_GUARD_REMOVE:
		...
+ 	case MADV_XXX:
+		return current->mm == mm ? MADVISE_VMA_READ_LOCK :
+				MADVISE_MMAP_READ_LOCK;
 	case MADV_DONTNEED:
 	case MADV_DONTNEED_LOCKED:

Thanks
Barry