Re: [KJ] [PATCH] unified spinlock

Re: [KJ] [PATCH] unified spinlock

[Randy.Dunlap]

Matthew Wilcox wrote:
> On Thu, Jan 20, 2005 at 08:56:18PM +0530, Amit Gud wrote:
> 
>>Unify the spinlock initialization as far as possible.
> 
> 
> Why would you want to replace a statically initialised spinlock with
> one that's initialised at runtime?

I wondered that also, since I prefer the compile-time inits myself.
so I looked at the KJ TODO list
<http://janitor.kernelnewbies.org/TODO> and it says:

From: Jonathan Corbet

Unified spinlock initialization:
convert all explicit lock initializations to spin_lock_init() or
rwlock_init().  (Besides consistency this also helps automatic lock
validators and debugging code.)

http://lwn.net/Articles/109505/
http://linux.bkbits.net:8080/linux-2.6/cset@419a6f292wHnthuDzw7VfgECNLmvLg?nav=index.html|ChangeSet@-8w
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I don't find the consistency part compelling at all.
If the change helps some static source analyzers, however, that would
be a good thing, but not a strong one (IMO).

-- 
~Randy
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] unified spinlock

[Randy.Dunlap]

Matthew Wilcox wrote:
> On Thu, Jan 20, 2005 at 07:35:56AM -0800, Randy.Dunlap wrote:
> 
>>Matthew Wilcox wrote:
>>
>>>On Thu, Jan 20, 2005 at 08:56:18PM +0530, Amit Gud wrote:
>>>
>>>
>>>>Unify the spinlock initialization as far as possible.
>>>
>>>
>>>Why would you want to replace a statically initialised spinlock with
>>>one that's initialised at runtime?
>>
>>I wondered that also, since I prefer the compile-time inits myself.
>>so I looked at the KJ TODO list
>><http://janitor.kernelnewbies.org/TODO> and it says:
>>
>>From: Jonathan Corbet
>>
>>Unified spinlock initialization:
>>convert all explicit lock initializations to spin_lock_init() or
>>rwlock_init().  (Besides consistency this also helps automatic lock
>>validators and debugging code.)
>>
>>http://lwn.net/Articles/109505/
>>http://linux.bkbits.net:8080/linux-2.6/cset@419a6f292wHnthuDzw7VfgECNLmvLg?nav=index.html|ChangeSet@-8w
>>~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>I don't find the consistency part compelling at all.
>>If the change helps some static source analyzers, however, that would
>>be a good thing, but not a strong one (IMO).
> 
> 
> I think this is referring to initialisation of spinlocks that are
> allocated dynamically, not statically.  If a lock validator can't cope
> with that, it needs to be fixed, IMO.
> 
Having just looked at the LWN.net article, is this (still) true?

<quote>
The stated reasons for this change include consistency and making life 
easier for automatic lock validators. There is also an unstated, but 
evident reason: the assignment form of lock initialization gets in the 
way of the realtime preemption patches. Those patches change most 
spinlocks in the kernel to a different, mutex type, and that breaks 
the initializers. As a result, the preemption patches must change all 
of those initializations throughout the kernel. By putting those 
specific changes into the mainline, it is possible to make the 
realtime patches smaller, less intrusive, and a little bit less scary.
</quote>

-- 
~Randy
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] unified spinlock

[Matthew Wilcox]

[-- Attachment #1: Type: text/plain, Size: 1693 bytes --]

On Thu, Jan 20, 2005 at 07:35:56AM -0800, Randy.Dunlap wrote:
> Matthew Wilcox wrote:
> >On Thu, Jan 20, 2005 at 08:56:18PM +0530, Amit Gud wrote:
> >
> >>Unify the spinlock initialization as far as possible.
> >
> >
> >Why would you want to replace a statically initialised spinlock with
> >one that's initialised at runtime?
> 
> I wondered that also, since I prefer the compile-time inits myself.
> so I looked at the KJ TODO list
> <http://janitor.kernelnewbies.org/TODO> and it says:
> 
> From: Jonathan Corbet
> 
> Unified spinlock initialization:
> convert all explicit lock initializations to spin_lock_init() or
> rwlock_init().  (Besides consistency this also helps automatic lock
> validators and debugging code.)
> 
> http://lwn.net/Articles/109505/
> http://linux.bkbits.net:8080/linux-2.6/cset@419a6f292wHnthuDzw7VfgECNLmvLg?nav=index.html|ChangeSet@-8w
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> I don't find the consistency part compelling at all.
> If the change helps some static source analyzers, however, that would
> be a good thing, but not a strong one (IMO).

I think this is referring to initialisation of spinlocks that are
allocated dynamically, not statically.  If a lock validator can't cope
with that, it needs to be fixed, IMO.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] unified spinlock

[Greg KH]

[-- Attachment #1: Type: text/plain, Size: 1132 bytes --]

On Thu, Jan 20, 2005 at 07:44:24AM -0800, Randy.Dunlap wrote:
> Matthew Wilcox wrote:
> >I think this is referring to initialisation of spinlocks that are
> >allocated dynamically, not statically.  If a lock validator can't cope
> >with that, it needs to be fixed, IMO.
> >
> Having just looked at the LWN.net article, is this (still) true?
> 
> <quote>
> The stated reasons for this change include consistency and making life 
> easier for automatic lock validators. There is also an unstated, but 
> evident reason: the assignment form of lock initialization gets in the 
> way of the realtime preemption patches. Those patches change most 
> spinlocks in the kernel to a different, mutex type, and that breaks 
> the initializers. As a result, the preemption patches must change all 
> of those initializations throughout the kernel. By putting those 
> specific changes into the mainline, it is possible to make the 
> realtime patches smaller, less intrusive, and a little bit less scary.
> </quote>

Yes, it is.  See the number of patches that have been flowing into the
kernel lately to fix this issue up.

thanks,

greg k-h

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] unified spinlock

[Matthew Wilcox]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

On Thu, Jan 20, 2005 at 08:50:47AM -0800, Greg KH wrote:
> On Thu, Jan 20, 2005 at 07:44:24AM -0800, Randy.Dunlap wrote:
> > Matthew Wilcox wrote:
> > >I think this is referring to initialisation of spinlocks that are
> > >allocated dynamically, not statically.  If a lock validator can't cope
> > >with that, it needs to be fixed, IMO.
> 
> Yes, it is.  See the number of patches that have been flowing into the
> kernel lately to fix this issue up.

However, the correct way to fix this is to use DEFINE_SPIN_LOCK, not to
make static locks dynamically initialised.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] unified spinlock

[Greg KH]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

On Thu, Jan 20, 2005 at 08:04:13PM +0000, Matthew Wilcox wrote:
> On Thu, Jan 20, 2005 at 08:50:47AM -0800, Greg KH wrote:
> > On Thu, Jan 20, 2005 at 07:44:24AM -0800, Randy.Dunlap wrote:
> > > Matthew Wilcox wrote:
> > > >I think this is referring to initialisation of spinlocks that are
> > > >allocated dynamically, not statically.  If a lock validator can't cope
> > > >with that, it needs to be fixed, IMO.
> > 
> > Yes, it is.  See the number of patches that have been flowing into the
> > kernel lately to fix this issue up.
> 
> However, the correct way to fix this is to use DEFINE_SPIN_LOCK, not to
> make static locks dynamically initialised.

Where ever possible, yes.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] unified spinlock

[Domen Puncer]

[-- Attachment #1: Type: text/plain, Size: 834 bytes --]

On 20/01/05 12:09 -0800, Greg KH wrote:
> On Thu, Jan 20, 2005 at 08:04:13PM +0000, Matthew Wilcox wrote:
> > On Thu, Jan 20, 2005 at 08:50:47AM -0800, Greg KH wrote:
> > > On Thu, Jan 20, 2005 at 07:44:24AM -0800, Randy.Dunlap wrote:
> > > > Matthew Wilcox wrote:
> > > > >I think this is referring to initialisation of spinlocks that are
> > > > >allocated dynamically, not statically.  If a lock validator can't cope
> > > > >with that, it needs to be fixed, IMO.
> > > 
> > > Yes, it is.  See the number of patches that have been flowing into the
> > > kernel lately to fix this issue up.
> > 
> > However, the correct way to fix this is to use DEFINE_SPIN_LOCK, not to
> > make static locks dynamically initialised.
> 
> Where ever possible, yes.

Is "static DEFINE_SPINLOCK(lock);" (which seems right in this case) ok?


	Domen

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors