Skip to contentSkip to conversation

mm/msync: tweak tmpfs patch for syscall msync

PATCH Superseded Kang Kai 3 participants 6 messages Apr 25, 2012 – Apr 26, 2012

Subsystems (2)
Memory management Memory mapping

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Kang Kai

author
#0 lore raw

PATCH 1/1 mm/msync: tweak tmpfs patch for syscall msync

Commit 1c3ae5441 fixes MIPS CPU cache alias problem. But it makes posix test cases mlockall/3-6 3-7 fail.
Tweak the patch to:
1 check the vma and its flags first
2 don't quit the function when meet first vma belongs to tmpfs file

Signed-off-by Kang Kai <[email protected]>
---
 mm/msync.c |   30 +++++++++++++-----------------
 1 files changed, 13 insertions(+), 17 deletions(-)
mm/msync.c +13 -17
index ced6215..31cd311 100644
@@ -60,23 +60,6 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
 	down_read(&mm->mmap_sem);
 	vma = find_vma(mm, start);
 
-#ifdef CONFIG_TMPFS
-	/*
-	 * For tmpfs, no matter which flag(ASYNC or SYNC) gets from msync,
-	 * there is not so much thing to do for CPUs without cache alias,
-	 * But for some CPUs with cache alias, msync has to flush cache
-	 * explicitly, which makes sure the data coherency between memory
-	 * file and cache.
-	 */
-	file =  vma->vm_file;
-	if (file && (file->f_op == &shmem_file_operations)) {
-		if(CPU_HAS_CACHE_ALIAS)
-			flush_cache_range(vma, start, start+len);
-		error = 0;
-		goto out_unlock;
-	}
-#endif
-
 	for (;;) {
 
 		/* Still start < end. */

@@ -97,6 +80,19 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
 			goto out_unlock;
 		}
 		file = vma->vm_file;
+#ifdef CONFIG_TMPFS
+		/*
+		 * For tmpfs, no matter which flag(ASYNC or SYNC) gets from msync,
+		 * there is not so much thing to do for CPUs without cache alias,
+		 * But for some CPUs with cache alias, msync has to flush cache
+		 * explicitly, which makes sure the data coherency between memory
+		 * file and cache.
+		 */
+		if (file && (file->f_op == &shmem_file_operations)) {
+			if(CPU_HAS_CACHE_ALIAS)
+				flush_cache_range(vma, start, start+len);
+		}
+#endif
 		start = vma->vm_end;
 		if ((flags & MS_SYNC) && file &&
 				(vma->vm_flags & VM_SHARED)) {
Signature · 1 line
1.7.5.4
#1 lore raw
Quoted 1 line · Kang Kai

Commit 1c3ae5441 fixes MIPS CPU cache alias problem. But it makes

Put a short log after the commit ID, that way we can read what the old commit was trying to do right in this commit.

Quoted 1 line

posix test cases mlockall/3-6 3-7 fail.

Can we expand on what the test is doing ? That way the commit header is stand alone. I know that I haven't memorized what all the posix tests do :)

Quoted 2 lines

Tweak the patch to:
1 check the vma and its flags first

Can you expand here as well. Do you really mean that you've moved the CONFIG_TMPFS block down in the function so that the normal vma flag checking will occur ?

Quoted 1 line

2 don't quit the function when meet first vma belongs to tmpfs file

And for this part, can you (or Zumeng) explain why we used to need to exit immediately and now we don't ?

The patch looks good though, thanks for looking into this tricky area of code.

Cheers,

Bruce

Quoted 54 lines
Signed-off-by Kang Kai<[email protected]>
---
  mm/msync.c |   30 +++++++++++++-----------------
  1 files changed, 13 insertions(+), 17 deletions(-)
mm/msync.c +13 -17
index ced6215..31cd311 100644
@@ -60,23 +60,6 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
  	down_read(&mm->mmap_sem);
  	vma = find_vma(mm, start);
 
-#ifdef CONFIG_TMPFS
-	/*
-	 * For tmpfs, no matter which flag(ASYNC or SYNC) gets from msync,
-	 * there is not so much thing to do for CPUs without cache alias,
-	 * But for some CPUs with cache alias, msync has to flush cache
-	 * explicitly, which makes sure the data coherency between memory
-	 * file and cache.
-	 */
-	file =  vma->vm_file;
-	if (file&&  (file->f_op ==&shmem_file_operations)) {
-		if(CPU_HAS_CACHE_ALIAS)
-			flush_cache_range(vma, start, start+len);
-		error = 0;
-		goto out_unlock;
-	}
-#endif
-
  	for (;;) {
 
  		/* Still start<  end. */

@@ -97,6 +80,19 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
  			goto out_unlock;
  		}
  		file = vma->vm_file;
+#ifdef CONFIG_TMPFS
+		/*
+		 * For tmpfs, no matter which flag(ASYNC or SYNC) gets from msync,
+		 * there is not so much thing to do for CPUs without cache alias,
+		 * But for some CPUs with cache alias, msync has to flush cache
+		 * explicitly, which makes sure the data coherency between memory
+		 * file and cache.
+		 */
+		if (file&&  (file->f_op ==&shmem_file_operations)) {
+			if(CPU_HAS_CACHE_ALIAS)
+				flush_cache_range(vma, start, start+len);
+		}
+#endif
  		start = vma->vm_end;
  		if ((flags&  MS_SYNC)&&  file&&
  				(vma->vm_flags&  VM_SHARED)) {
#2 lore raw

That was my last version, you can google to get by "msync zumeng" So feel free to merge it.

Regards,
Zumeng

Quoted 88 lines · Bruce Ashfield
Quoted 1 line · Kang Kai

Commit 1c3ae5441 fixes MIPS CPU cache alias problem. But it makes

Put a short log after the commit ID, that way we can read what the old commit was trying to do right in this commit.

Quoted 1 line

posix test cases mlockall/3-6 3-7 fail.

Can we expand on what the test is doing ? That way the commit header is stand alone. I know that I haven't memorized what all the posix tests do :)

Quoted 2 lines

Tweak the patch to:
1 check the vma and its flags first

Can you expand here as well. Do you really mean that you've moved the CONFIG_TMPFS block down in the function so that the normal vma flag checking will occur ?

Quoted 1 line

2 don't quit the function when meet first vma belongs to tmpfs file

And for this part, can you (or Zumeng) explain why we used to need to exit immediately and now we don't ?

The patch looks good though, thanks for looking into this tricky area of code.

Cheers,

Bruce

Quoted 56 lines
Signed-off-by Kang Kai<[email protected]>

---
mm/msync.c | 30 +++++++++++++-----------------

1 files changed, 13 insertions(+), 17 deletions(-)
mm/msync.c +0 -17
index ced6215..31cd311 100644
@@ -60,23 +60,6 @@ SYSCALL_DEFINE3(msync, unsigned long, start, 
 size_t, len, int, flags)
 down_read(&mm->mmap_sem);
 vma = find_vma(mm, start);
 
-#ifdef CONFIG_TMPFS
- /*
- * For tmpfs, no matter which flag(ASYNC or SYNC) gets from msync,
- * there is not so much thing to do for CPUs without cache alias,
- * But for some CPUs with cache alias, msync has to flush cache
- * explicitly, which makes sure the data coherency between memory
- * file and cache.
- */
- file = vma->vm_file;
- if (file&& (file->f_op ==&shmem_file_operations)) {
- if(CPU_HAS_CACHE_ALIAS)
- flush_cache_range(vma, start, start+len);
- error = 0;
- goto out_unlock;
- }
-#endif
-
 for (;;) {

/* Still start< end. */

mm/msync.c +13 -0
@@ -97,6 +80,19 @@ SYSCALL_DEFINE3(msync, unsigned long, start, 
 size_t, len, int, flags)
 goto out_unlock;
 }
 file = vma->vm_file;
+#ifdef CONFIG_TMPFS
+ /*
+ * For tmpfs, no matter which flag(ASYNC or SYNC) gets from msync,
+ * there is not so much thing to do for CPUs without cache alias,
+ * But for some CPUs with cache alias, msync has to flush cache
+ * explicitly, which makes sure the data coherency between memory
+ * file and cache.
+ */
+ if (file&& (file->f_op ==&shmem_file_operations)) {
+ if(CPU_HAS_CACHE_ALIAS)
+ flush_cache_range(vma, start, start+len);
+ }
+#endif
 start = vma->vm_end;

if ((flags& MS_SYNC)&& file&&
(vma->vm_flags& VM_SHARED)) {

#3 lore raw
Quoted 2 lines · Zumeng Chen

That was my last version, you can google to get by "msync zumeng" So feel free to merge it.

Sounds good, but we should still get the header updates, since the first version merged into the tree already.

Bruce

Quoted 93 lines

Regards,
Zumeng

Quoted 88 lines · Bruce Ashfield
Quoted 1 line · Kang Kai

Commit 1c3ae5441 fixes MIPS CPU cache alias problem. But it makes

Put a short log after the commit ID, that way we can read what the old commit was trying to do right in this commit.

Quoted 1 line

posix test cases mlockall/3-6 3-7 fail.

Can we expand on what the test is doing ? That way the commit header is stand alone. I know that I haven't memorized what all the posix tests do :)

Quoted 2 lines

Tweak the patch to:
1 check the vma and its flags first

Can you expand here as well. Do you really mean that you've moved the CONFIG_TMPFS block down in the function so that the normal vma flag checking will occur ?

Quoted 1 line

2 don't quit the function when meet first vma belongs to tmpfs file

And for this part, can you (or Zumeng) explain why we used to need to exit immediately and now we don't ?

The patch looks good though, thanks for looking into this tricky area of code.

Cheers,

Bruce

Quoted 56 lines
Signed-off-by Kang Kai<[email protected]>

---
mm/msync.c | 30 +++++++++++++-----------------

1 files changed, 13 insertions(+), 17 deletions(-)
mm/msync.c +0 -17
index ced6215..31cd311 100644
@@ -60,23 +60,6 @@ SYSCALL_DEFINE3(msync, unsigned long, start,
 size_t, len, int, flags)
 down_read(&mm->mmap_sem);
 vma = find_vma(mm, start);
 
-#ifdef CONFIG_TMPFS
- /*
- * For tmpfs, no matter which flag(ASYNC or SYNC) gets from msync,
- * there is not so much thing to do for CPUs without cache alias,
- * But for some CPUs with cache alias, msync has to flush cache
- * explicitly, which makes sure the data coherency between memory
- * file and cache.
- */
- file = vma->vm_file;
- if (file&& (file->f_op ==&shmem_file_operations)) {
- if(CPU_HAS_CACHE_ALIAS)
- flush_cache_range(vma, start, start+len);
- error = 0;
- goto out_unlock;
- }
-#endif
-
 for (;;) {

/* Still start< end. */

mm/msync.c +13 -0
@@ -97,6 +80,19 @@ SYSCALL_DEFINE3(msync, unsigned long, start,
 size_t, len, int, flags)
 goto out_unlock;
 }
 file = vma->vm_file;
+#ifdef CONFIG_TMPFS
+ /*
+ * For tmpfs, no matter which flag(ASYNC or SYNC) gets from msync,
+ * there is not so much thing to do for CPUs without cache alias,
+ * But for some CPUs with cache alias, msync has to flush cache
+ * explicitly, which makes sure the data coherency between memory
+ * file and cache.
+ */
+ if (file&& (file->f_op ==&shmem_file_operations)) {
+ if(CPU_HAS_CACHE_ALIAS)
+ flush_cache_range(vma, start, start+len);
+ }
+#endif
 start = vma->vm_end;

if ((flags& MS_SYNC)&& file&&
(vma->vm_flags& VM_SHARED)) {

#4 lore raw
Quoted 6 lines · Bruce Ashfield
Quoted 2 lines · Zumeng Chen

That was my last version, you can google to get by "msync zumeng" So feel free to merge it.

Sounds good, but we should still get the header updates, since the first version merged into the tree already.

Yes, NP, I'm OK with all changes. :)

Regards,
Zumeng

Quoted 97 lines

Bruce

Quoted 93 lines

Regards,
Zumeng

Quoted 88 lines · Bruce Ashfield
Quoted 1 line · Kang Kai

Commit 1c3ae5441 fixes MIPS CPU cache alias problem. But it makes

Put a short log after the commit ID, that way we can read what the old commit was trying to do right in this commit.

Quoted 1 line

posix test cases mlockall/3-6 3-7 fail.

Can we expand on what the test is doing ? That way the commit header is stand alone. I know that I haven't memorized what all the posix tests do :)

Quoted 2 lines

Tweak the patch to:
1 check the vma and its flags first

Can you expand here as well. Do you really mean that you've moved the CONFIG_TMPFS block down in the function so that the normal vma flag checking will occur ?

Quoted 1 line

2 don't quit the function when meet first vma belongs to tmpfs file

And for this part, can you (or Zumeng) explain why we used to need to exit immediately and now we don't ?

The patch looks good though, thanks for looking into this tricky area of code.

Cheers,

Bruce

Quoted 56 lines
Signed-off-by Kang Kai<[email protected]>

---
mm/msync.c | 30 +++++++++++++-----------------

1 files changed, 13 insertions(+), 17 deletions(-)
mm/msync.c +0 -17
index ced6215..31cd311 100644
@@ -60,23 +60,6 @@ SYSCALL_DEFINE3(msync, unsigned long, start,
 size_t, len, int, flags)
 down_read(&mm->mmap_sem);
 vma = find_vma(mm, start);
 
-#ifdef CONFIG_TMPFS
- /*
- * For tmpfs, no matter which flag(ASYNC or SYNC) gets from msync,
- * there is not so much thing to do for CPUs without cache alias,
- * But for some CPUs with cache alias, msync has to flush cache
- * explicitly, which makes sure the data coherency between memory
- * file and cache.
- */
- file = vma->vm_file;
- if (file&& (file->f_op ==&shmem_file_operations)) {
- if(CPU_HAS_CACHE_ALIAS)
- flush_cache_range(vma, start, start+len);
- error = 0;
- goto out_unlock;
- }
-#endif
-
 for (;;) {

/* Still start< end. */

mm/msync.c +13 -0
@@ -97,6 +80,19 @@ SYSCALL_DEFINE3(msync, unsigned long, start,
 size_t, len, int, flags)
 goto out_unlock;
 }
 file = vma->vm_file;
+#ifdef CONFIG_TMPFS
+ /*
+ * For tmpfs, no matter which flag(ASYNC or SYNC) gets from msync,
+ * there is not so much thing to do for CPUs without cache alias,
+ * But for some CPUs with cache alias, msync has to flush cache
+ * explicitly, which makes sure the data coherency between memory
+ * file and cache.
+ */
+ if (file&& (file->f_op ==&shmem_file_operations)) {
+ if(CPU_HAS_CACHE_ALIAS)
+ flush_cache_range(vma, start, start+len);
+ }
+#endif
 start = vma->vm_end;

if ((flags& MS_SYNC)&& file&&
(vma->vm_flags& VM_SHARED)) {

author
#5 lore raw
Quoted 8 lines · Zumeng Chen
Quoted 6 lines · Bruce Ashfield
Quoted 2 lines · Zumeng Chen

That was my last version, you can google to get by "msync zumeng" So feel free to merge it.

Sounds good, but we should still get the header updates, since the first version merged into the tree already.

Yes, NP, I'm OK with all changes. :)

Thanks.
I'll send version 2 with header updates.

Quoted 101 lines

Regards,
Zumeng

Quoted 97 lines

Bruce

Quoted 93 lines

Regards,
Zumeng

Quoted 88 lines · Bruce Ashfield
Quoted 1 line · Kang Kai

Commit 1c3ae5441 fixes MIPS CPU cache alias problem. But it makes

Put a short log after the commit ID, that way we can read what the old commit was trying to do right in this commit.

Quoted 1 line

posix test cases mlockall/3-6 3-7 fail.

Can we expand on what the test is doing ? That way the commit header is stand alone. I know that I haven't memorized what all the posix tests do :)

Quoted 2 lines

Tweak the patch to:
1 check the vma and its flags first

Can you expand here as well. Do you really mean that you've moved the CONFIG_TMPFS block down in the function so that the normal vma flag checking will occur ?

Quoted 1 line

2 don't quit the function when meet first vma belongs to tmpfs file

And for this part, can you (or Zumeng) explain why we used to need to exit immediately and now we don't ?

The patch looks good though, thanks for looking into this tricky area of code.

Cheers,

Bruce

Quoted 56 lines
Signed-off-by Kang Kai<[email protected]>

---
mm/msync.c | 30 +++++++++++++-----------------

1 files changed, 13 insertions(+), 17 deletions(-)
mm/msync.c +0 -17
index ced6215..31cd311 100644
@@ -60,23 +60,6 @@ SYSCALL_DEFINE3(msync, unsigned long, start,
 size_t, len, int, flags)
 down_read(&mm->mmap_sem);
 vma = find_vma(mm, start);
 
-#ifdef CONFIG_TMPFS
- /*
- * For tmpfs, no matter which flag(ASYNC or SYNC) gets from msync,
- * there is not so much thing to do for CPUs without cache alias,
- * But for some CPUs with cache alias, msync has to flush cache
- * explicitly, which makes sure the data coherency between memory
- * file and cache.
- */
- file = vma->vm_file;
- if (file&& (file->f_op ==&shmem_file_operations)) {
- if(CPU_HAS_CACHE_ALIAS)
- flush_cache_range(vma, start, start+len);
- error = 0;
- goto out_unlock;
- }
-#endif
-
 for (;;) {

/* Still start< end. */

mm/msync.c +13 -0
@@ -97,6 +80,19 @@ SYSCALL_DEFINE3(msync, unsigned long, start,
 size_t, len, int, flags)
 goto out_unlock;
 }
 file = vma->vm_file;
+#ifdef CONFIG_TMPFS
+ /*
+ * For tmpfs, no matter which flag(ASYNC or SYNC) gets from msync,
+ * there is not so much thing to do for CPUs without cache alias,
+ * But for some CPUs with cache alias, msync has to flush cache
+ * explicitly, which makes sure the data coherency between memory
+ * file and cache.
+ */
+ if (file&& (file->f_op ==&shmem_file_operations)) {
+ if(CPU_HAS_CACHE_ALIAS)
+ flush_cache_range(vma, start, start+len);
+ }
+#endif
 start = vma->vm_end;

if ((flags& MS_SYNC)&& file&&
(vma->vm_flags& VM_SHARED)) {