[patch 1/1] unified spinlock initialization arch/um/drivers/port_kern.c

[patch 1/1] unified spinlock initialization arch/um/drivers/port_kern.c

[blaisorblade]

From: <domen@coderock.org>
Cc: <user-mode-linux-devel@lists.sourceforge.net>, <domen@coderock.org>, <amitg@calsoftinc.com>, <gud@eth.net>

Unify the spinlock initialization as far as possible.

Signed-off-by: Amit Gud <gud@eth.net>
Signed-off-by: Domen Puncer <domen@coderock.org>
Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 linux-2.6.11-paolo/arch/um/drivers/port_kern.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

diff -puN arch/um/drivers/port_kern.c~uml-switch-spinlock-init arch/um/drivers/port_kern.c
--- linux-2.6.11/arch/um/drivers/port_kern.c~uml-switch-spinlock-init	2005-03-09 10:35:28.786848080 +0100
+++ linux-2.6.11-paolo/arch/um/drivers/port_kern.c	2005-03-09 10:35:28.789847624 +0100
@@ -185,11 +185,11 @@ void *port_data(int port_num)
 		  .has_connection 	= 0,
 		  .sem 			= __SEMAPHORE_INITIALIZER(port->sem, 
 								  0),
-		  .lock 		= SPIN_LOCK_UNLOCKED,
 		  .port 	 	= port_num,
 		  .fd  			= fd,
 		  .pending 		= LIST_HEAD_INIT(port->pending),
 		  .connections 		= LIST_HEAD_INIT(port->connections) });
+	spin_lock_init(&port->lock);
 	list_add(&port->list, &ports);
 
  found:
_

Re: [patch 1/1] unified spinlock initialization arch/um/drivers/port_kern.c

[Russell King]

On Wed, Mar 09, 2005 at 10:42:33AM +0100, blaisorblade@yahoo.it wrote:
> 
> From: <domen@coderock.org>
> Cc: <user-mode-linux-devel@lists.sourceforge.net>, <domen@coderock.org>, <amitg@calsoftinc.com>, <gud@eth.net>
> 
> Unify the spinlock initialization as far as possible.

Are you sure this is really the best option in this instance?
Sometimes, static data initialisation is more efficient than
code-based manual initialisation, especially when the memory
is written to anyway.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [patch 1/1] unified spinlock initialization arch/um/drivers/port_kern.c

[Blaisorblade]

On Wednesday 09 March 2005 18:12, Russell King wrote:
> On Wed, Mar 09, 2005 at 10:42:33AM +0100, blaisorblade@yahoo.it wrote:
> > From: <domen@coderock.org>
> > Cc: <user-mode-linux-devel@lists.sourceforge.net>, <domen@coderock.org>,
> > <amitg@calsoftinc.com>, <gud@eth.net>
> >
> > Unify the spinlock initialization as far as possible.

> Are you sure this is really the best option in this instance?
> Sometimes, static data initialisation is more efficient than
> code-based manual initialisation, especially when the memory
> is written to anyway.
Agreed, theoretically, but this was done for multiple reasons globally, for 
instance as a preparation to Ingo Molnar's preemption patches. There was 
mention of this on lwn.net about this:

http://lwn.net/Articles/108719/

Ok?
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729
http://www.user-mode-linux.org/~blaisorblade

Re: [patch 1/1] unified spinlock initialization arch/um/drivers/port_kern.c

[Russell King]

On Wed, Mar 09, 2005 at 08:52:24PM +0100, Blaisorblade wrote:
> On Wednesday 09 March 2005 18:12, Russell King wrote:
> > On Wed, Mar 09, 2005 at 10:42:33AM +0100, blaisorblade@yahoo.it wrote:
> > > From: <domen@coderock.org>
> > > Cc: <user-mode-linux-devel@lists.sourceforge.net>, <domen@coderock.org>,
> > > <amitg@calsoftinc.com>, <gud@eth.net>
> > >
> > > Unify the spinlock initialization as far as possible.
> 
> > Are you sure this is really the best option in this instance?
> > Sometimes, static data initialisation is more efficient than
> > code-based manual initialisation, especially when the memory
> > is written to anyway.
> Agreed, theoretically, but this was done for multiple reasons globally, for 
> instance as a preparation to Ingo Molnar's preemption patches. There was 
> mention of this on lwn.net about this:
> 
> http://lwn.net/Articles/108719/

Was this announced on linux-kernel as well?  I don't remember seeing it.

I'm not convinced about the practicality of converting all static
initialisations to code-based initialisations though - I can see
that the number of initialisation functions scattered throughout
the kernel is going to increase dramatically to achieve this.

With a 2.4 to 2.6 kernel bloat already on the order of 140% for
similar functionality, I can only see the kernel getting more and
more bloated _for the same feature level_ because we're trying to
add more features to the kernel.

I'm not entirely convinced that is an all round sane approach.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [patch 1/1] unified spinlock initialization arch/um/drivers/port_kern.c

[Andrew Morton]

Russell King <rmk+lkml@arm.linux.org.uk> wrote:
>
>  I'm not convinced about the practicality of converting all static
>  initialisations to code-based initialisations though

This is the first one I recall seeing.  All the other conversions were replacing

	static spinlock_t lock = SPIN_LOCK_UNLOCKED;

with
	static DEFINE_SPINLOCK(lock);

and replacing

	{
		lock = SPIN_LOCK_UNLOCKED;
	}

with

	{
		spin_lock_init(lock);
	}

Re: [patch 1/1] unified spinlock initialization arch/um/drivers/port_kern.c

[linux-os]

On Wed, 9 Mar 2005, Andrew Morton wrote:

> Russell King <rmk+lkml@arm.linux.org.uk> wrote:
>>
>>  I'm not convinced about the practicality of converting all static
>>  initialisations to code-based initialisations though
>
> This is the first one I recall seeing.  All the other conversions were replacing
>
> 	static spinlock_t lock = SPIN_LOCK_UNLOCKED;
>
> with
> 	static DEFINE_SPINLOCK(lock);
>
> and replacing
>
> 	{
> 		lock = SPIN_LOCK_UNLOCKED;
> 	}
>
> with
>
> 	{
> 		spin_lock_init(lock);
> 	}

We need to retain the spin_lock_init(&lock) because not all spin-locks
are allocated at compile-time. They might be allocated from kmalloc()
on startup, probably in a structure, along with other so-called
global data.

Cheers,
Dick Johnson
Penguin : Linux version 2.6.10 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by Dictator Bush.
                  98.36% of all statistics are fiction.

Re: [patch 1/1] unified spinlock initialization arch/um/drivers/port_kern.c

[Zwane Mwaikambo]

On Wed, 9 Mar 2005, linux-os wrote:

> We need to retain the spin_lock_init(&lock) because not all spin-locks
> are allocated at compile-time. They might be allocated from kmalloc()
> on startup, probably in a structure, along with other so-called
> global data.

Not to worry my good man, it's not going anywhere.

Re: [patch 1/1] unified spinlock initialization arch/um/drivers/port_kern.c

[Thomas Gleixner]

On Wed, 2005-03-09 at 20:52 +0100, Blaisorblade wrote:
> > Are you sure this is really the best option in this instance?
> > Sometimes, static data initialisation is more efficient than
> > code-based manual initialisation, especially when the memory
> > is written to anyway.
> Agreed, theoretically, but this was done for multiple reasons globally, for 
> instance as a preparation to Ingo Molnar's preemption patches. There was 
> mention of this on lwn.net about this:
> 
> http://lwn.net/Articles/108719/

Those patches did only the conversion of

static spinlock_t lock = SPIN_LOCK_UNLOCKED;
       lock = SPIN_LOCK_UNLOCKED;
to
static DEFINE_SPINLOCK(lock);
       spin_lock_init(lock);

If you want to do static initialization inside of structures, then you
have to define a seperate MACRO similar to the static initialization of
list_head's inside of structures:

static struct sysfs_dirent sysfs_root = {
        .s_sibling      = LIST_HEAD_INIT(sysfs_root.s_sibling),

tglx

Re: [uml-devel] Re: [patch 1/1] unified spinlock initialization arch/um/drivers/port_kern.c

[Blaisorblade]

On Thursday 10 March 2005 09:12, Thomas Gleixner wrote:
> On Wed, 2005-03-09 at 20:52 +0100, Blaisorblade wrote:
> > > Are you sure this is really the best option in this instance?
> > > Sometimes, static data initialisation is more efficient than
> > > code-based manual initialisation, especially when the memory
> > > is written to anyway.
> >
> > Agreed, theoretically, but this was done for multiple reasons globally,
> > for instance as a preparation to Ingo Molnar's preemption patches. There
> > was mention of this on lwn.net about this:
> >
> > http://lwn.net/Articles/108719/
>
> Those patches did only the conversion of
>
> static spinlock_t lock = SPIN_LOCK_UNLOCKED;
>        lock = SPIN_LOCK_UNLOCKED;
> to
> static DEFINE_SPINLOCK(lock);
>        spin_lock_init(lock);
First: I didn't write the patch, only forwarded it, so I just guessed why it 
was done.

The latter is spin_lock_init(&lock); (since someone got confused about this).

However, this is a .lock = SPIN_LOCK_UNLOCKED  -> 
spin_lock_init(&struct.lock),

so I don't understand what changes... the structure is initialized inside a 
function, so there's no change.

> If you want to do static initialization inside of structures, then you
> have to define a seperate MACRO similar to the static initialization of
> list_head's inside of structures:

> static struct sysfs_dirent sysfs_root = {
>         .s_sibling      = LIST_HEAD_INIT(sysfs_root.s_sibling),

I don't see the need here... and the initialization is not in static code; it 
changes this code snippet:

void *port_data(int port_num)
{
//...
        *port = ((struct port_list)
                { .list                 = LIST_HEAD_INIT(port->list),
                  .has_connection       = 0,
                  .sem                  = __SEMAPHORE_INITIALIZER(port->sem,
                                                                  0),
                  .lock                 = SPIN_LOCK_UNLOCKED,
                  .port                 = port_num,
                  .fd                   = fd,
                  .pending              = LIST_HEAD_INIT(port->pending),
                  .connections          = 
LIST_HEAD_INIT(port->connections) });

So you are all doing some confusion (in fact I guess Andrew realized this when 
he merged this anyway).

-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729
http://www.user-mode-linux.org/~blaisorblade