Re: No more bits in vm_area_struct's vm_flags.

[KAMEZAWA Hiroyuki]

On Mon, 28 Sep 2009 22:00:20 +0100 (BST)
Hugh Dickins <hugh.dickins@xxxxxxxxxxxxx> 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
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/