lkml.org 
[lkml]   [2009]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: No more bits in vm_area_struct's vm_flags.
On Mon, 28 Sep 2009 22:00:20 +0100 (BST)
Hugh Dickins <hugh.dickins@tiscali.co.uk> wrote:

>> On Tue, 29 Sep 2009, KAMEZAWA Hiroyuki wrote:
> >
> > Every time I see long long in the kernel, my concern is
> > "do I need spinlock to access this for 32bit arch ? is it safe ?".
> > (And it makes binary=>disassemble=>C (by eyes) a bit difficult)
> > Then, I don't like long long personally.
> >
> > Another reason is some other calls like test_bit() cannot be used against
> > long long. (even if it's not used _now_)
> >
> > Maybe vm->vm_flags will not require extra locks because
> > it can be protected by bigger lock as mmap_sem.
>
> I think that even as you wrote, you guessed I wouldn't be persuaded ;)
> It sounds like you've had a bad experience with a long long in the past.
>
yes ;)


> We already have to have locking for vm_flags, of course we do: it's
> mmap_sem, yes, though I think you'll find some exceptions which know
> they have exclusive access without it.
>
> We use ordinary logical operations on vm_flags, we don't need it to
> be atomic, we don't need an additional spinlock, we don't need to use
> test_bit(). It's very easy! (But irritating in a few places which
> have to down_write mmap_sem for no other reason than to update vm_flags.)
>
Okay, I'll have no objections.

Just a notice from lines stripped by grep

(1) using "int" will be never correct even on 32bit.
==
vm_flags 242 arch/mips/mm/c-r3k.c int exec = vma->vm_flags & VM_EXEC;
vm_flags 293 drivers/char/mem.c return vma->vm_flags & VM_MAYSHARE;
vm_flags 44 mm/madvise.c int new_flags = vma->vm_flags;
vm_flags 547 mm/memory.c unsigned long vm_flags = vma->vm_flags;

But yes, it will be not a terrible bug for a while.

(2) All vm macros should be defined with ULL suffix. for supporing ~
==
vm_flags 30 arch/x86/mm/hugetlbpage.c unsigned long vm_flags = vma->vm_flags & ~VM_LOCKED;

(3) vma_merge()'s vm_flags should be ULL.


Not so many places as I thought..




> > Then, please make it to be long long if its's recommended.
> >
> > keeping vm_flags to be 32bit may makes vma_merge() ugly.
> > If so, long long is a choice.
>
> unsigned long long is certainly the natural choice: that way leaves
> freedom for people to add more flags in future without worrying about
> which flags variable to put them into. I'd better explain some of my
> objections to Nigel's patch in a reply to him rather than here.
>
> I have made up a patch to convert it to unsigned long long (not gone
> through all the arches yet though), mainly to try a build to see how
> it works out in practice. I used a config which built most of the
> non-debugging objects in mm/, things like migration and mempolicy
> and memcg and ksm and so forth, but not kmemleak.
>
> And I have to admit that the 834 bytes it added to i386 kernel text
> is more than I was expecting, more than I can just brush away as "in
> the noise". I don't fully understand it yet. There's a few silly
> "andl $0xffffffff"s from the compiler (4.3.2), but not enough to
> worry about. Typically enlarged objects grow by 4 bytes, presumably
> clearing the upper half when setting vma->vm_flags, fair enough.
>
> 300 bytes of growth is in mmap.o, 100 bytes of that in do_mmap_pgoff();
> yet I don't see why it needed to grow by more than, say, 12 bytes.
>
> My current feeling is that unsigned long long is the right way to
> go, but given the bloat, we shouldn't convert over until we need to:
> right now we should look to save a few flags instead.

Okay,

Thanks,
-Kame




\
 
 \ /
  Last update: 2009-09-29 04:03    [W:0.257 / U:0.572 seconds]
©2003-2014 Jasper Spaans. Advertise on this site