[PATCH] Define a UNIQUE value for AS_UNEVICTABLE flag

[PATCH] Define a UNIQUE value for AS_UNEVICTABLE flag

[Lee Schermerhorn]

[PATCH] Define UNIQUE value of AS_UNEVICTABLE

Needed in 2.6.28, 2.6.29, ...

A new "address_space flag"--AS_MM_ALL_LOCKS--was defined to use the next
available AS flag while the Unevictable LRU was under development.  The
Unevictable LRU was using the same flag and "no one" noticed.  Current 
mainline, since 2.6.28, has same value for two symbolic flag names.

So, define a unique flag value for AS_UNEVICTABLE--up close to the other
flags, [at the cost of an additional #ifdef] so we'll notice next time.
Note that #ifdef is not actually required, if we don't mind having the 
unused flag value defined.

Replace #defines with an enum.

Signed-off-by: Lee Schermerhorn <lee.schermerhorn@hp.com>

 include/linux/pagemap.h |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

Index: linux-2.6.29/include/linux/pagemap.h
===================================================================
--- linux-2.6.29.orig/include/linux/pagemap.h	2009-03-23 19:12:14.000000000 -0400
+++ linux-2.6.29/include/linux/pagemap.h	2009-04-02 11:56:27.000000000 -0400
@@ -18,9 +18,14 @@
  * Bits in mapping->flags.  The lower __GFP_BITS_SHIFT bits are the page
  * allocation mode flags.
  */
-#define	AS_EIO		(__GFP_BITS_SHIFT + 0)	/* IO error on async write */
-#define AS_ENOSPC	(__GFP_BITS_SHIFT + 1)	/* ENOSPC on async write */
-#define AS_MM_ALL_LOCKS	(__GFP_BITS_SHIFT + 2)	/* under mm_take_all_locks() */
+enum mapping_flags {
+	AS_EIO		= __GFP_BITS_SHIFT + 0,	/* IO error on async write */
+	AS_ENOSPC	= __GFP_BITS_SHIFT + 1,	/* ENOSPC on async write */
+	AS_MM_ALL_LOCKS	= __GFP_BITS_SHIFT + 2,	/* under mm_take_all_locks() */
+#ifdef CONFIG_UNEVICTABLE_LRU
+	AS_UNEVICTABLE	= __GFP_BITS_SHIFT + 3,	/* e.g., ramdisk, SHM_LOCK */
+#endif
+};
 
 static inline void mapping_set_error(struct address_space *mapping, int error)
 {
@@ -33,7 +38,6 @@ static inline void mapping_set_error(str
 }
 
 #ifdef CONFIG_UNEVICTABLE_LRU
-#define AS_UNEVICTABLE	(__GFP_BITS_SHIFT + 2)	/* e.g., ramdisk, SHM_LOCK */
 
 static inline void mapping_set_unevictable(struct address_space *mapping)
 {

Re: [PATCH] Define a UNIQUE value for AS_UNEVICTABLE flag

[Andrew Morton]

On Thu, 02 Apr 2009 12:47:15 -0400 Lee Schermerhorn <Lee.Schermerhorn@hp.com> wrote:

> [PATCH] Define UNIQUE value of AS_UNEVICTABLE
> 
> Needed in 2.6.28, 2.6.29, ...
> 
> A new "address_space flag"--AS_MM_ALL_LOCKS--was defined to use the next
> available AS flag while the Unevictable LRU was under development.  The
> Unevictable LRU was using the same flag and "no one" noticed.  Current 
> mainline, since 2.6.28, has same value for two symbolic flag names.

argh.

What are the user-observable effects of the bug, and why didn't anyone
notice it until now?

Re: [PATCH] Define a UNIQUE value for AS_UNEVICTABLE flag

[KOSAKI Motohiro]

(cc to Avi)

>> [PATCH] Define UNIQUE value of AS_UNEVICTABLE
>>
>> Needed in 2.6.28, 2.6.29, ...
>>
>> A new "address_space flag"--AS_MM_ALL_LOCKS--was defined to use the next
>> available AS flag while the Unevictable LRU was under development.  The
>> Unevictable LRU was using the same flag and "no one" noticed.  Current
>> mainline, since 2.6.28, has same value for two symbolic flag names.
>
> argh.
>
> What are the user-observable effects of the bug, and why didn't anyone
> notice it until now?

AS_MM_ALL_LOCKS is used by mmu_notifier. it mean it is used by only kvm.
In the other hand,  AS_UNEVICTABLE mean unevictable shmem or ramfs.

Then, if shmem opend process use ioctl(KVM_CREATE_VM), unevictable
flag on the shmem accidentally turn off.
but, fortunatelly, In modern desktop environment, only KVM control
program use above ioctl. then, we can assume this doesn't use shmem
and ramfs.

Am I missing anything?

Re: [PATCH] Define a UNIQUE value for AS_UNEVICTABLE flag

[Avi Kivity]

KOSAKI Motohiro wrote:
> (cc to Avi)
>
>   
>>> [PATCH] Define UNIQUE value of AS_UNEVICTABLE
>>>
>>> Needed in 2.6.28, 2.6.29, ...
>>>
>>> A new "address_space flag"--AS_MM_ALL_LOCKS--was defined to use the next
>>> available AS flag while the Unevictable LRU was under development.  The
>>> Unevictable LRU was using the same flag and "no one" noticed.  Current
>>> mainline, since 2.6.28, has same value for two symbolic flag names.
>>>       
>> argh.
>>
>> What are the user-observable effects of the bug, and why didn't anyone
>> notice it until now?
>>     
>
> AS_MM_ALL_LOCKS is used by mmu_notifier. it mean it is used by only kvm.
> In the other hand,  AS_UNEVICTABLE mean unevictable shmem or ramfs.
>
> Then, if shmem opend process use ioctl(KVM_CREATE_VM), unevictable
> flag on the shmem accidentally turn off.
> but, fortunatelly, In modern desktop environment, only KVM control
> program use above ioctl. then, we can assume this doesn't use shmem
> and ramfs.
>
> Am I missing anything?
>   

Qemu, the main kvm user, indeed doesn't use shm or ramfs.  However it is 
not inconceivable that one day it will, as a way to share memory among 
guests.

It isn't surprising that no bug was observed, but let's fix it.

(If hugetlbfs is considered unevictable, then qemu can trigger this bug 
today)

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] Define a UNIQUE value for AS_UNEVICTABLE flag

[Lee Schermerhorn]

On Thu, 2009-04-02 at 09:53 -0700, Andrew Morton wrote:
> On Thu, 02 Apr 2009 12:47:15 -0400 Lee Schermerhorn <Lee.Schermerhorn@hp.com> wrote:
> 
> > [PATCH] Define UNIQUE value of AS_UNEVICTABLE
> > 
> > Needed in 2.6.28, 2.6.29, ...
> > 
> > A new "address_space flag"--AS_MM_ALL_LOCKS--was defined to use the next
> > available AS flag while the Unevictable LRU was under development.  The
> > Unevictable LRU was using the same flag and "no one" noticed.  Current 
> > mainline, since 2.6.28, has same value for two symbolic flag names.
> 
> argh.
> 
> What are the user-observable effects of the bug, and why didn't anyone
> notice it until now?


Well, AS_MM_ALL_LOCKS seems to be used for mmu notifiers.  So, I expect
I've never enabled it.  However, I think that if it got set, all of the
pages in all of the vmas that had it set will appear to be unevictable.
This would only matter if/when one tried to reclaim/evict them.  The
pages would probably get stranded on the unevictable lru [until freed]
in that case as there would be no scan to rescue them when
AS_MM_ALL_LOCKS is cleared.

Not sure about the other way around:  flag set as AS_UNEVICTABLE and
code examining AS_MM_ALL_LOCKS sees it...