[PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Chen Gang]

When CONFIG_LOCKDEP is not defined, spin_lock_irqsave() is not equal to
spin_lock() + local_irq_save().

In __mod_timer(), After call spin_lock_irqsave() with 'base->lock' in
lock_timer_base(), it may use spin_lock() with the 'new_base->lock'.

It may let original call do_raw_spin_lock_flags() with 'base->lock',
but new  call LOCK_CONTENDED() with 'new_base->lock'.

In fact, we need both of them call do_raw_spin_lock_flags(), so use
spin_lock_irqsave() instead of spin_lock() + local_irq_save().


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/timer.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index aa8b964..2550a62 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -754,9 +754,9 @@ __mod_timer(struct timer_list *timer, unsigned long expires,
 		if (likely(base->running_timer != timer)) {
 			/* See the comment in lock_timer_base() */
 			timer_set_base(timer, NULL);
-			spin_unlock(&base->lock);
+			spin_unlock_irqrestore(&base->lock, flags);
 			base = new_base;
-			spin_lock(&base->lock);
+			spin_lock_irqsave(&base->lock, flags);
 			timer_set_base(timer, base);
 		}
 	}
-- 
1.7.7.6

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Thomas Gleixner]

On Wed, 19 Jun 2013, Chen Gang wrote:

> 
> When CONFIG_LOCKDEP is not defined, spin_lock_irqsave() is not equal to
> spin_lock() + local_irq_save().
> 
> In __mod_timer(), After call spin_lock_irqsave() with 'base->lock' in
> lock_timer_base(), it may use spin_lock() with the 'new_base->lock'.
> 
> It may let original call do_raw_spin_lock_flags() with 'base->lock',
> but new  call LOCK_CONTENDED() with 'new_base->lock'.
> 
> In fact, we need both of them call do_raw_spin_lock_flags(), so use
> spin_lock_irqsave() instead of spin_lock() + local_irq_save().

Why do we need to do that? There is no reason to do so and it's
totally irrelevant whether CONFIG_LOCKDEP is enabled or not.

The code is written intentionally this way.

What's the difference between:

       spin_lock_irqsave(&l1, flags);
       spin_unlock(&l1);
       spin_lock(l2);
       spin_unlock_irqrestore(&l2, flags);

and

       spin_lock_irqsave(&l1, flags);
       spin_unlock_irqrestore(&l1);
       spin_lock_irqsave(l2, flags);
       spin_unlock_irqrestore(&l2, flags);

The difference is that we avoid to touch the interrupt disable in the
cpu, which might be an expensive operation depending on the cpu model.

There is no point in reenabling interrupts just to disable them
again a few instruction cycles later.

And lockdep is perfectly fine with that code. All lockdep cares about
is whether the lock context (interrupts disabled) is correct or
not.

Thanks,

	tglx

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Chen Gang]

On 06/19/2013 04:41 PM, Thomas Gleixner wrote:
> On Wed, 19 Jun 2013, Chen Gang wrote:
> 
>> > 
>> > When CONFIG_LOCKDEP is not defined, spin_lock_irqsave() is not equal to
>> > spin_lock() + local_irq_save().
>> > 
>> > In __mod_timer(), After call spin_lock_irqsave() with 'base->lock' in
>> > lock_timer_base(), it may use spin_lock() with the 'new_base->lock'.
>> > 
>> > It may let original call do_raw_spin_lock_flags() with 'base->lock',
>> > but new  call LOCK_CONTENDED() with 'new_base->lock'.
>> > 
>> > In fact, we need both of them call do_raw_spin_lock_flags(), so use
>> > spin_lock_irqsave() instead of spin_lock() + local_irq_save().
> Why do we need to do that? There is no reason to do so and it's
> totally irrelevant whether CONFIG_LOCKDEP is enabled or not.
>

Please see include/linux/spinlock_api_smp.h (or see bottom of this mail)

 
> The code is written intentionally this way.
> 
> What's the difference between:
> 
>        spin_lock_irqsave(&l1, flags);
>        spin_unlock(&l1);
>        spin_lock(l2);
>        spin_unlock_irqrestore(&l2, flags);
> 
> and
> 
>        spin_lock_irqsave(&l1, flags);
>        spin_unlock_irqrestore(&l1);
>        spin_lock_irqsave(l2, flags);
>        spin_unlock_irqrestore(&l2, flags);
> 

Yes


> The difference is that we avoid to touch the interrupt disable in the
> cpu, which might be an expensive operation depending on the cpu model.
> 
> There is no point in reenabling interrupts just to disable them
> again a few instruction cycles later.
> 
> And lockdep is perfectly fine with that code. All lockdep cares about
> is whether the lock context (interrupts disabled) is correct or
> not.

Please reference include/linux/spinlock_api_smp.h and include/linux/spinlock.h

  spin_lock_irqsave() ->
    raw_spin_lock_irqsave() ->
      _raw_spin_lock_irqsave() ->
        __raw_spin_lock_irqsave()

  spin_lock() ->
    raw_spin_lock() ->
      _raw_spin_lock() ->
        __raw_spin_lock()


104 static inline unsigned long __raw_spin_lock_irqsave(raw_spinlock_t *lock)
105 {
106         unsigned long flags;
107 
108         local_irq_save(flags);
109         preempt_disable();
110         spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
111         /*
112          * On lockdep we dont want the hand-coded irq-enable of
113          * do_raw_spin_lock_flags() code, because lockdep assumes
114          * that interrupts are not re-enabled during lock-acquire:
115          */
116 #ifdef CONFIG_LOCKDEP
117         LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
118 #else
119         do_raw_spin_lock_flags(lock, &flags);
120 #endif
121         return flags;
122 }
...

140 static inline void __raw_spin_lock(raw_spinlock_t *lock)
141 {
142         preempt_disable();
143         spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
144         LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
145 }



Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Thomas Gleixner]

On Wed, 19 Jun 2013, Chen Gang wrote:

> On 06/19/2013 04:41 PM, Thomas Gleixner wrote:
> > On Wed, 19 Jun 2013, Chen Gang wrote:
> > 
> >> > 
> >> > When CONFIG_LOCKDEP is not defined, spin_lock_irqsave() is not equal to
> >> > spin_lock() + local_irq_save().
> >> > 
> >> > In __mod_timer(), After call spin_lock_irqsave() with 'base->lock' in
> >> > lock_timer_base(), it may use spin_lock() with the 'new_base->lock'.
> >> > 
> >> > It may let original call do_raw_spin_lock_flags() with 'base->lock',
> >> > but new  call LOCK_CONTENDED() with 'new_base->lock'.
> >> > 
> >> > In fact, we need both of them call do_raw_spin_lock_flags(), so use
> >> > spin_lock_irqsave() instead of spin_lock() + local_irq_save().
> > Why do we need to do that? There is no reason to do so and it's
> > totally irrelevant whether CONFIG_LOCKDEP is enabled or not.
> >
> 
> Please see include/linux/spinlock_api_smp.h (or see bottom of this mail)
> 
>  
> > The code is written intentionally this way.
> > 
> > What's the difference between:
> > 
> >        spin_lock_irqsave(&l1, flags);
> >        spin_unlock(&l1);
> >        spin_lock(l2);
> >        spin_unlock_irqrestore(&l2, flags);
> > 
> > and
> > 
> >        spin_lock_irqsave(&l1, flags);
> >        spin_unlock_irqrestore(&l1);
> >        spin_lock_irqsave(l2, flags);
> >        spin_unlock_irqrestore(&l2, flags);
> > 
> 
> Yes
> 
> 
> > The difference is that we avoid to touch the interrupt disable in the
> > cpu, which might be an expensive operation depending on the cpu model.
> > 
> > There is no point in reenabling interrupts just to disable them
> > again a few instruction cycles later.
> > 
> > And lockdep is perfectly fine with that code. All lockdep cares about
> > is whether the lock context (interrupts disabled) is correct or
> > not.
> 
> Please reference include/linux/spinlock_api_smp.h and include/linux/spinlock.h
>
>   spin_lock_irqsave() ->
>     raw_spin_lock_irqsave() ->
>       _raw_spin_lock_irqsave() ->
>         __raw_spin_lock_irqsave()
> 
>   spin_lock() ->
>     raw_spin_lock() ->
>       _raw_spin_lock() ->
>         __raw_spin_lock()
> 

I'm well aware how that works. And there is no difference whether you
do:

	local_irq_save(flags);
	spin_lock(&lock);
or
	spin_lock_irqsave(&lock, flags);


Lockdep tracks lock ordering and the context in which a lock is
taken. The timer base lock can be taken in interrupt context, so it
always needs to be taken with interrupts disabled. That's what lockdep
cares about.

And 
	spin_lock_irqsave(&l1, flags);
	spin_unlock(&l1);
	spin_lock(&l2);
	spin_unlock_irqrestore(&l2, flags);

fulfils that for both l1 and l2.

It does not matter whether the code pathes are different, what matters
is that they are semantically the same. And that's the case.

Thanks,

	tglx

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Chen Gang]

On 06/19/2013 05:59 PM, Thomas Gleixner wrote:
> Lockdep tracks lock ordering and the context in which a lock is
> taken. The timer base lock can be taken in interrupt context, so it
> always needs to be taken with interrupts disabled. That's what lockdep
> cares about.
> 
> And 
> 	spin_lock_irqsave(&l1, flags);
> 	spin_unlock(&l1);
> 	spin_lock(&l2);
> 	spin_unlock_irqrestore(&l2, flags);
> 
> fulfils that for both l1 and l2.
> 
> It does not matter whether the code pathes are different, what matters
> is that they are semantically the same. And that's the case.

But I feel, they are not semantically the same.

if CONFIG_LOCKDEP is not defined, spin_lock_irqsave() should call
do_raw_spin_lock_flags(), not call LOCK_CONTENDED().



Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Thomas Gleixner]

On Wed, 19 Jun 2013, Chen Gang wrote:
> On 06/19/2013 05:59 PM, Thomas Gleixner wrote:
> > Lockdep tracks lock ordering and the context in which a lock is
> > taken. The timer base lock can be taken in interrupt context, so it
> > always needs to be taken with interrupts disabled. That's what lockdep
> > cares about.
> > 
> > And 
> > 	spin_lock_irqsave(&l1, flags);
> > 	spin_unlock(&l1);
> > 	spin_lock(&l2);
> > 	spin_unlock_irqrestore(&l2, flags);
> > 
> > fulfils that for both l1 and l2.
> > 
> > It does not matter whether the code pathes are different, what matters
> > is that they are semantically the same. And that's the case.
> 
> But I feel, they are not semantically the same.

This is not about feelings. This is about facts.
 
> if CONFIG_LOCKDEP is not defined, spin_lock_irqsave() should call
> do_raw_spin_lock_flags(), not call LOCK_CONTENDED().

That is what the code already does:

#ifdef CONFIG_LOCKDEP
        LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
#else
	do_raw_spin_lock_flags(lock, &flags);
#endif

If LOCKDEP is not defined it calls do_raw_spin_lock_flags(). Where is
your point?

And the reason why the LOCKDEP case and the !LOCKDEP case are
different is explained in the comment above that code. And both behave
the same way versus the spinlock semantics.

We must do this because some architectures implement
do_raw_spin_lock_flags() in the following way:

do_raw_spin_lock_flags(l, flags)
{
	while (!arch_spin_trylock(l)) {
	      if (!irq_disabled_flags(flags)) {
	      	      arch_irq_restore(flags);
		      cpu_relax();
		      arch_irq_disable();
	      }
	}
}

This is done to limit irq disabled time while we wait for the
lock. Though most architectures implement it as:

	while (!arch_spin_trylock(l))
	      cpu_relax();

In the lockdep case we CANNOT reenable interrupts in the spinning code
and therefor we use the 

	while (!arch_spin_trylock(l))
	      cpu_relax();

variant.

And again. Both are semantically the same.

spin_lock_irqsave() semantics are:

The function returns with the lock acquired, interrupts and preemption
disabled. Both variants do that.

The internal details whether an architecture reenables interrupts
while spinning on a contended lock or not are completely irrelevant
and do not affect the correctness of the code.

Thanks,

	tglx

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Chen Gang]

On 06/19/2013 06:49 PM, Thomas Gleixner wrote:
>> > But I feel, they are not semantically the same.
> This is not about feelings. This is about facts.
>  

Excuse me, my English is not quite well, the reason why I use 'feel' is
to want to say with more polite.

Maybe it is not the correct word for saying with more polite.


>> > if CONFIG_LOCKDEP is not defined, spin_lock_irqsave() should call
>> > do_raw_spin_lock_flags(), not call LOCK_CONTENDED().
> That is what the code already does:
> 
> #ifdef CONFIG_LOCKDEP
>         LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
> #else
> 	do_raw_spin_lock_flags(lock, &flags);
> #endif
> 
> If LOCKDEP is not defined it calls do_raw_spin_lock_flags(). Where is
> your point?
> 

Yes, it is my point.


> And the reason why the LOCKDEP case and the !LOCKDEP case are
> different is explained in the comment above that code. And both behave
> the same way versus the spinlock semantics.
> 
> We must do this because some architectures implement
> do_raw_spin_lock_flags() in the following way:
> 
> do_raw_spin_lock_flags(l, flags)
> {
> 	while (!arch_spin_trylock(l)) {
> 	      if (!irq_disabled_flags(flags)) {
> 	      	      arch_irq_restore(flags);
> 		      cpu_relax();
> 		      arch_irq_disable();
> 	      }
> 	}
> }
> 

For mn10300 and sparc64 (not space32), it doesn't like your demo above.

For powerpc and s390, it seems your demo above (although not quite
precious)

For x86 and parisc, it like your demo above.

And another will be no different between LOCKDEP and !LOCKDEP


> This is done to limit irq disabled time while we wait for the
> lock. Though most architectures implement it as:
> 
> 	while (!arch_spin_trylock(l))
> 	      cpu_relax();
> 

Yes.


> In the lockdep case we CANNOT reenable interrupts in the spinning code
> and therefor we use the 
> 
> 	while (!arch_spin_trylock(l))
> 	      cpu_relax();
> 
> variant.
> 
> And again. Both are semantically the same.
> 

I am not quite sure about mn10300 and sparc64.

Could you be sure about it ?

At least, for mn10300 and sparc64, they have no duty to make sure of
our using ways to be correct.


> spin_lock_irqsave() semantics are:
> 
> The function returns with the lock acquired, interrupts and preemption
> disabled. Both variants do that.
> 
> The internal details whether an architecture reenables interrupts
> while spinning on a contended lock or not are completely irrelevant
> and do not affect the correctness of the code.

For API definition, it has no duty to make it correct if the user call
them with informal ways, especially, the implementation is related with
various architectures.

At last if it is really correct now, it is still dangerous (the API has
no duty to let it always correct).



Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Thomas Gleixner]

On Thu, 20 Jun 2013, Chen Gang wrote:
> On 06/19/2013 06:49 PM, Thomas Gleixner wrote:
> > We must do this because some architectures implement
> > do_raw_spin_lock_flags() in the following way:
> > 
> > do_raw_spin_lock_flags(l, flags)
> > {
> > 	while (!arch_spin_trylock(l)) {
> > 	      if (!irq_disabled_flags(flags)) {
> > 	      	      arch_irq_restore(flags);
> > 		      cpu_relax();
> > 		      arch_irq_disable();
> > 	      }
> > 	}
> > }
> > 
> 
> For mn10300 and sparc64 (not space32), it doesn't like your demo above.

Sigh. You're an sparc64 and mn10300 assembler expert, right?
 
> For powerpc and s390, it seems your demo above (although not quite
> precious)

It does not matter at all whether the code is implemented exactly that
way. What matters is that the semantics are the same.
 
> For x86 and parisc, it like your demo above.

For parisc, yes.

For x86, no. 

static __always_inline void arch_spin_lock_flags(arch_spinlock_t *lock,
                                                  unsigned long flags)
{
        arch_spin_lock(lock);
}

> > And again. Both are semantically the same.
> > 
> 
> I am not quite sure about mn10300 and sparc64.
> 
> Could you be sure about it ?

I am sure, because I can read _and_ understand the asm code.

> At least, for mn10300 and sparc64, they have no duty to make sure of
> our using ways to be correct.

You think that architectures can implement these functions as they
want and see fit? No, they can't otherwise their kernel would not work
at all. Again the semantics are what we care about, not the
implementation. And it's totally irrelevant whether its implemented in
C or in assembler.

> > spin_lock_irqsave() semantics are:
> > 
> > The function returns with the lock acquired, interrupts and preemption
> > disabled. Both variants do that.
> > 
> > The internal details whether an architecture reenables interrupts
> > while spinning on a contended lock or not are completely irrelevant
> > and do not affect the correctness of the code.
> 
> For API definition, it has no duty to make it correct if the user call
> them with informal ways, especially, the implementation is related with
> various architectures.

Nonsense.

We define an API and the semantics and every implementation has to
follow these semantics. Period.

If there is an implementation, which does not follow the
specification, then it needs to be fixed. Period.

And this has nothing to do how this is called and whether various
architecture specific implementations exist or not.
 
> At last if it is really correct now, it is still dangerous (the API has
> no duty to let it always correct).

The API CANNOT make sure that the implementations are correct. It's
the duty of the person who implements the API to make sure it is
correct.

And there is nothing dangerous except your ongoing attempts to patch
code which you really do not understand at all.

Thanks,

	tglx

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Chen Gang]

On 06/20/2013 03:36 PM, Thomas Gleixner wrote:
> On Thu, 20 Jun 2013, Chen Gang wrote:
>> > On 06/19/2013 06:49 PM, Thomas Gleixner wrote:
>>> > > We must do this because some architectures implement
>>> > > do_raw_spin_lock_flags() in the following way:
>>> > > 
>>> > > do_raw_spin_lock_flags(l, flags)
>>> > > {
>>> > > 	while (!arch_spin_trylock(l)) {
>>> > > 	      if (!irq_disabled_flags(flags)) {
>>> > > 	      	      arch_irq_restore(flags);
>>> > > 		      cpu_relax();
>>> > > 		      arch_irq_disable();
>>> > > 	      }
>>> > > 	}
>>> > > }
>>> > > 
>> > 
>> > For mn10300 and sparc64 (not space32), it doesn't like your demo above.
> Sigh. You're an sparc64 and mn10300 assembler expert, right?
>  

No, do you mean: "only the related expert can discuss about it" ?


>> > For powerpc and s390, it seems your demo above (although not quite
>> > precious)
> It does not matter at all whether the code is implemented exactly that
> way. What matters is that the semantics are the same.
>  

>> > For x86 and parisc, it like your demo above.
> For parisc, yes.
> 
> For x86, no. 
> 
> static __always_inline void arch_spin_lock_flags(arch_spinlock_t *lock,
>                                                   unsigned long flags)
> {
>         arch_spin_lock(lock);
> }
> 

That is one of x86 implementation, not all (please see xen implementation)


>>> > > And again. Both are semantically the same.
>>> > > 
>> > 
>> > I am not quite sure about mn10300 and sparc64.
>> > 
>> > Could you be sure about it ?
> I am sure, because I can read _and_ understand the asm code.
> 

Are you expert of them ?  ;-)

But whether you stick to or not, I do not care about it.


>> > At least, for mn10300 and sparc64, they have no duty to make sure of
>> > our using ways to be correct.
> You think that architectures can implement these functions as they
> want and see fit? No, they can't otherwise their kernel would not work
> at all. Again the semantics are what we care about, not the
> implementation. And it's totally irrelevant whether its implemented in
> C or in assembler.
> 

Of cause, it is independent with language.

>>> > > spin_lock_irqsave() semantics are:
>>> > > 
>>> > > The function returns with the lock acquired, interrupts and preemption
>>> > > disabled. Both variants do that.
>>> > > 
>>> > > The internal details whether an architecture reenables interrupts
>>> > > while spinning on a contended lock or not are completely irrelevant
>>> > > and do not affect the correctness of the code.
>> > 
>> > For API definition, it has no duty to make it correct if the user call
>> > them with informal ways, especially, the implementation is related with
>> > various architectures.
> Nonsense.
>

The word 'Nonsense' seems not quite polite.  ;-)

At least, when some one see this usage below:

   spin_lock_irqsave(&l1, flags);
   spin_unlock(&l1);
   spin_lock(&l2);
   spin_unlock_irqrestore(&l2, flags);

most of them will be amazing.


Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Thomas Gleixner]

On Thu, 20 Jun 2013, Chen Gang wrote:
> On 06/20/2013 03:36 PM, Thomas Gleixner wrote:
> > On Thu, 20 Jun 2013, Chen Gang wrote:
> >> > On 06/19/2013 06:49 PM, Thomas Gleixner wrote:
> >>> > > We must do this because some architectures implement
> >>> > > do_raw_spin_lock_flags() in the following way:
> >>> > > 
> >>> > > do_raw_spin_lock_flags(l, flags)
> >>> > > {
> >>> > > 	while (!arch_spin_trylock(l)) {
> >>> > > 	      if (!irq_disabled_flags(flags)) {
> >>> > > 	      	      arch_irq_restore(flags);
> >>> > > 		      cpu_relax();
> >>> > > 		      arch_irq_disable();
> >>> > > 	      }
> >>> > > 	}
> >>> > > }
> >>> > > 
> >> > 
> >> > For mn10300 and sparc64 (not space32), it doesn't like your demo above.
> > Sigh. You're an sparc64 and mn10300 assembler expert, right?
> >  
> 
> No, do you mean: "only the related expert can discuss about it" ?

A discussion requires that the people who are discussing something are
familiar with the matter.

> >> > For API definition, it has no duty to make it correct if the user call
> >> > them with informal ways, especially, the implementation is related with
> >> > various architectures.
> > Nonsense.
> >
> 
> The word 'Nonsense' seems not quite polite.  ;-)

It might be not polite, but it's correct. And I really start to get
annoyed.
 
> At least, when some one see this usage below:
> 
>    spin_lock_irqsave(&l1, flags);
>    spin_unlock(&l1);
>    spin_lock(&l2);
>    spin_unlock_irqrestore(&l2, flags);
> 
> most of them will be amazing.

What's amazing about this?

It's the equivivalent to:

     local_irq_save(flags);
     spin_lock(&l1);
     spin_unlock(&l1);
     spin_lock(&l2);
     spin_unlock(&l2);
     local_irq_restore(flags);

The only difference is, that spin_lock_irqsave() implementations are
allowed to reenable interrupts while spinning, but again that's an
implementation detail which does not matter at all.

Thanks,

	tglx

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Chen Gang]

On 06/20/2013 05:02 PM, Thomas Gleixner wrote:
> On Thu, 20 Jun 2013, Chen Gang wrote:
>> > On 06/20/2013 03:36 PM, Thomas Gleixner wrote:
>>> > > On Thu, 20 Jun 2013, Chen Gang wrote:
>>>>> > >> > On 06/19/2013 06:49 PM, Thomas Gleixner wrote:
>>>>>>> > >>> > > We must do this because some architectures implement
>>>>>>> > >>> > > do_raw_spin_lock_flags() in the following way:
>>>>>>> > >>> > > 
>>>>>>> > >>> > > do_raw_spin_lock_flags(l, flags)
>>>>>>> > >>> > > {
>>>>>>> > >>> > > 	while (!arch_spin_trylock(l)) {
>>>>>>> > >>> > > 	      if (!irq_disabled_flags(flags)) {
>>>>>>> > >>> > > 	      	      arch_irq_restore(flags);
>>>>>>> > >>> > > 		      cpu_relax();
>>>>>>> > >>> > > 		      arch_irq_disable();
>>>>>>> > >>> > > 	      }
>>>>>>> > >>> > > 	}
>>>>>>> > >>> > > }
>>>>>>> > >>> > > 
>>>>> > >> > 
>>>>> > >> > For mn10300 and sparc64 (not space32), it doesn't like your demo above.
>>> > > Sigh. You're an sparc64 and mn10300 assembler expert, right?
>>> > >  
>> > 
>> > No, do you mean: "only the related expert can discuss about it" ?
> A discussion requires that the people who are discussing something are
> familiar with the matter.
> 

In fact, if every related member are familiar with the matter, it is
only a "work flow" (providing pach --> review --> apply), not need
'discussion'.


>>>>> > >> > For API definition, it has no duty to make it correct if the user call
>>>>> > >> > them with informal ways, especially, the implementation is related with
>>>>> > >> > various architectures.
>>> > > Nonsense.
>>> > >
>> > 
>> > The word 'Nonsense' seems not quite polite.  ;-)
> It might be not polite, but it's correct. And I really start to get
> annoyed.
>  

correct and polite are different things.

For cooperation, better with polite.


>> > At least, when some one see this usage below:
>> > 
>> >    spin_lock_irqsave(&l1, flags);
>> >    spin_unlock(&l1);
>> >    spin_lock(&l2);
>> >    spin_unlock_irqrestore(&l2, flags);
>> > 
>> > most of them will be amazing.
> What's amazing about this?
> 
> It's the equivivalent to:
> 
>      local_irq_save(flags);
>      spin_lock(&l1);
>      spin_unlock(&l1);
>      spin_lock(&l2);
>      spin_unlock(&l2);
>      local_irq_restore(flags);
> 
> The only difference is, that spin_lock_irqsave() implementations are
> allowed to reenable interrupts while spinning, but again that's an
> implementation detail which does not matter at all.

We are just discussing about it in another mail thread, so not need
reply it.


Thanks
-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Chen Gang]

On 06/19/2013 05:59 PM, Thomas Gleixner wrote:
> I'm well aware how that works. And there is no difference whether you
> do:
> 
> 	local_irq_save(flags);
> 	spin_lock(&lock);
> or
> 	spin_lock_irqsave(&lock, flags);

if CONFIG_LOCKDEP is not defined, they are not semantically the same.


Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Thomas Gleixner]

On Wed, 19 Jun 2013, Chen Gang wrote:

> On 06/19/2013 05:59 PM, Thomas Gleixner wrote:
> > I'm well aware how that works. And there is no difference whether you
> > do:
> > 
> > 	local_irq_save(flags);
> > 	spin_lock(&lock);
> > or
> > 	spin_lock_irqsave(&lock, flags);
> 
> if CONFIG_LOCKDEP is not defined, they are not semantically the same.

Care to explain _your_ spinlock semantics to me?

The factual ones are:

    spin_lock_irqsave() returns with the lock held, interrupts and
    preemption disabled. 

    spin_lock() returns with the lock held, preemption disabled. It
    does not affect interrupt disabled/enabled state

So
	local_irq_save(flags);
	spin_lock(&lock);

is semantically the same as 

	spin_lock_irqsave(&lock, flags);

And this is completely independent of LOCKDEP.

Thanks,

	tglx

    

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Chen Gang]

On 06/19/2013 06:53 PM, Thomas Gleixner wrote:
> On Wed, 19 Jun 2013, Chen Gang wrote:
> 
>> > On 06/19/2013 05:59 PM, Thomas Gleixner wrote:
>>> > > I'm well aware how that works. And there is no difference whether you
>>> > > do:
>>> > > 
>>> > > 	local_irq_save(flags);
>>> > > 	spin_lock(&lock);
>>> > > or
>>> > > 	spin_lock_irqsave(&lock, flags);
>> > 
>> > if CONFIG_LOCKDEP is not defined, they are not semantically the same.
> Care to explain _your_ spinlock semantics to me?
> 
> The factual ones are:
> 
>     spin_lock_irqsave() returns with the lock held, interrupts and
>     preemption disabled. 
> 

Yes.

>     spin_lock() returns with the lock held, preemption disabled. It
>     does not affect interrupt disabled/enabled state
> 

Yes.

> So
> 	local_irq_save(flags);
> 	spin_lock(&lock);
> 
> is semantically the same as 
> 
> 	spin_lock_irqsave(&lock, flags);
> 

Yes (but reverse is NO).

> And this is completely independent of LOCKDEP.

NO.

 	spin_lock_irqsave(&lock, flags);

 is not semantically the same as

 	local_irq_save(flags);
 	spin_lock(&lock);

It depend on the spin_lock_irqsave() implementation, if the parameters
has no relation ship with each other, semantically the same.


Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Thomas Gleixner]

On Thu, 20 Jun 2013, Chen Gang wrote:
> On 06/19/2013 06:53 PM, Thomas Gleixner wrote:
> > So
> > 	local_irq_save(flags);
> > 	spin_lock(&lock);
> > 
> > is semantically the same as 
> > 
> > 	spin_lock_irqsave(&lock, flags);
> > 
> 
> Yes (but reverse is NO).
> 
> > And this is completely independent of LOCKDEP.
> 
> NO.
> 
>  	spin_lock_irqsave(&lock, flags);
> 
>  is not semantically the same as
> 
>  	local_irq_save(flags);
>  	spin_lock(&lock);

If A is semantically the same as B, then B is semantically the same as
A. At least that's the common understanding.

You seem to have a different definition of semantics, but I prefer the
common one.
 
> It depend on the spin_lock_irqsave() implementation, if the parameters
> has no relation ship with each other, semantically the same.

Yes, it depends on the implementation, but all implementations do:

     local_irq_save(flags);
     arch_spin_lock_flags(l, flags);

And whether that maps to a reenable interrupts while spinning or not,
has nothing to do with the spinlock semantics.

If you find a single architecture specific implementation, which is
wrong, then fix it and send a patch for it.

The core implementation _IS_ correct. Period.

Thanks,

	tglx

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Chen Gang]

On 06/20/2013 05:07 PM, Thomas Gleixner wrote:
> On Thu, 20 Jun 2013, Chen Gang wrote:
>> > On 06/19/2013 06:53 PM, Thomas Gleixner wrote:
>>> > > So
>>> > > 	local_irq_save(flags);
>>> > > 	spin_lock(&lock);
>>> > > 
>>> > > is semantically the same as 
>>> > > 
>>> > > 	spin_lock_irqsave(&lock, flags);
>>> > > 
>> > 
>> > Yes (but reverse is NO).
>> > 
>>> > > And this is completely independent of LOCKDEP.
>> > 
>> > NO.
>> > 
>> >  	spin_lock_irqsave(&lock, flags);
>> > 
>> >  is not semantically the same as
>> > 
>> >  	local_irq_save(flags);
>> >  	spin_lock(&lock);
> If A is semantically the same as B, then B is semantically the same as
> A. At least that's the common understanding.
> 

  From A to B is OK.

Not means:

  From B to A is also OK.


> You seem to have a different definition of semantics, but I prefer the
> common one.
>  
>> > It depend on the spin_lock_irqsave() implementation, if the parameters
>> > has no relation ship with each other, semantically the same.
> Yes, it depends on the implementation, but all implementations do:
> 
>      local_irq_save(flags);
>      arch_spin_lock_flags(l, flags);
> 

Yes this is spin_lock_irqsave().

At least, this implemenation is not equal to.

	local_irq_save(flags);
	spin_lock(l);

So if for arch_spin_lock_flags(), 'flags' is no relation ship with 'l',
we can say semantically the same.



Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Thomas Gleixner]

Chen,

On Thu, 20 Jun 2013, Chen Gang wrote:
> On 06/20/2013 05:07 PM, Thomas Gleixner wrote:
> > If A is semantically the same as B, then B is semantically the same as
> > A. At least that's the common understanding.
> > 
> 
>   From A to B is OK.
> 
> Not means:
> 
>   From B to A is also OK.

Either you're questioning logic, math and fundamental basics of
computer science or you simply fail to grok the difference between
semantics and implementation details. See below.

> > Yes, it depends on the implementation, but all implementations do:
> > 
> >      local_irq_save(flags);
> >      arch_spin_lock_flags(l, flags);
> > 
> 
> Yes this is spin_lock_irqsave().
> 
> At least, this implemenation is not equal to.
> 
> 	local_irq_save(flags);
> 	spin_lock(l);

Again. It is semantically the same, because the semantics are:

   spin_lock_irqsave() returns with interrupts disabled, preemption
   disabled and the lock acquired.

This construct exactly follows these semantics:

 	local_irq_save(flags);
 	spin_lock(l);

After spin_lock(l) interrupts are disabled, preemption is disabled and
the lock is acquired. End of discussion.

I wasted enough time explaining you the difference between semantics
and implementation, but you seem to be simply advisory restistant.

And I already told you very impolite in the other thread, that I'm not
going to cope with such nonsense anymore. And yes, I'm tired of it.

Provide factual prove, that there is a bug in the code. And to prove
that, you actually need to understand the code and the basic concepts
behind it. 

If you keep up pursuing your contributions plan at the expense of my
and other peoples valuable time, I consider this as extremly impolite
from your side. The form of collaboration you are going to achieve
this way is an entry in my /dev/null mail filter.

Thanks,

	tglx

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Chen Gang]

On 06/20/2013 06:42 PM, Thomas Gleixner wrote:
> Chen,
> 
> On Thu, 20 Jun 2013, Chen Gang wrote:
>> > On 06/20/2013 05:07 PM, Thomas Gleixner wrote:
>>> > > If A is semantically the same as B, then B is semantically the same as
>>> > > A. At least that's the common understanding.
>>> > > 
>> > 
>> >   From A to B is OK.
>> > 
>> > Not means:
>> > 
>> >   From B to A is also OK.
> Either you're questioning logic, math and fundamental basics of
> computer science or you simply fail to grok the difference between
> semantics and implementation details. See below.
> 
>>> > > Yes, it depends on the implementation, but all implementations do:
>>> > > 
>>> > >      local_irq_save(flags);
>>> > >      arch_spin_lock_flags(l, flags);
>>> > > 
>> > 
>> > Yes this is spin_lock_irqsave().
>> > 
>> > At least, this implemenation is not equal to.
>> > 
>> > 	local_irq_save(flags);
>> > 	spin_lock(l);
> Again. It is semantically the same, because the semantics are:
> 
>    spin_lock_irqsave() returns with interrupts disabled, preemption
>    disabled and the lock acquired.
> 
> This construct exactly follows these semantics:
> 
>  	local_irq_save(flags);
>  	spin_lock(l);
> 
> After spin_lock(l) interrupts are disabled, preemption is disabled and
> the lock is acquired. End of discussion.
> 

OK, end of discussion. It is a polite.


> I wasted enough time explaining you the difference between semantics
> and implementation, but you seem to be simply advisory restistant.
> 

Yes, time resources are really very expensive for every members.


> And I already told you very impolite in the other thread, that I'm not
> going to cope with such nonsense anymore. And yes, I'm tired of it.
> 

OK, I don't care about it.

We just stop now.

:-)

> Provide factual prove, that there is a bug in the code. And to prove
> that, you actually need to understand the code and the basic concepts
> behind it. 
> 
> If you keep up pursuing your contributions plan at the expense of my
> and other peoples valuable time, I consider this as extremly impolite
> from your side. The form of collaboration you are going to achieve
> this way is an entry in my /dev/null mail filter.


Don't worry about it, and why worry about it ?


Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel/timer.c: using spin_lock_irqsave instead of spin_lock + local_irq_save, especially when CONFIG_LOCKDEP not defined

[Eric Dumazet]

On Thu, 2013-06-20 at 16:37 +0800, Chen Gang wrote:

>  	spin_lock_irqsave(&lock, flags);
> 
>  is not semantically the same as
> 
>  	local_irq_save(flags);
>  	spin_lock(&lock);
> 
> It depend on the spin_lock_irqsave() implementation, if the parameters
> has no relation ship with each other, semantically the same.

Of course all implementations must respect the blocks are
totally the same.

Arguing about this is plain silly.

If you found a buggy implementation, please fix it.