Skip to contentSkip to conversation

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

PATCH In review Chen Gang 3 participants 19 messages Jun 19, 2013 – Jun 20, 2013

lore
Share & tools
lore

Chen Gang

author
#0 lore raw

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

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 <[email protected]>
---
 kernel/timer.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
kernel/timer.c +2 -2
index aa8b964..2550a62 100644
@@ -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);
 		}
 	}
Signature · 1 line
1.7.7.6
#1 lore raw
Quoted 4 lines · Chen Gang

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

Quoted 8 lines

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

author
#2 lore raw
Quoted 9 lines · Thomas Gleixner
Quoted 7 lines · Chen Gang
Quoted 7 lines

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'.

Quoted 8 lines
Quoted 5 lines
Quoted 5 lines

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)

Quoted 9 lines

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);
Quoted 7 lines

and

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

Yes

Quoted 9 lines

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.

Signature · 3 lines
Chen Gang

Asianux Corporation
#3 lore raw
Quoted 55 lines · Chen Gang
Quoted 9 lines · Thomas Gleixner
Quoted 7 lines · Chen Gang
Quoted 7 lines

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'.

Quoted 8 lines
Quoted 5 lines
Quoted 5 lines

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)

Quoted 9 lines

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);
Quoted 7 lines

and

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

Yes

Quoted 9 lines

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() ->

Quoted 9 lines
    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

author
#4 lore raw
Quoted 11 lines · Thomas Gleixner

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);
Quoted 4 lines

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.

Signature · 3 lines
Chen Gang

Asianux Corporation
#5 lore raw
Quoted 12 lines · Chen Gang
Quoted 11 lines · Thomas Gleixner

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);
Quoted 6 lines
Quoted 4 lines

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.

Quoted 2 lines

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

author
#6 lore raw
Quoted 3 lines · Thomas Gleixner
Quoted 1 line
Quoted 1 line

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.

Quoted 4 lines
Quoted 2 lines
Quoted 2 lines

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:

Quoted 9 lines
#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.

Quoted 8 lines

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)

Quoted 10 lines
{
	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

Quoted 6 lines

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.

Quoted 10 lines

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.

Quoted 8 lines

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.

Signature · 3 lines
Chen Gang

Asianux Corporation
#7 lore raw
Quoted 16 lines · Chen Gang
Quoted 4 lines · Thomas Gleixner

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

do_raw_spin_lock_flags(l, flags)

Quoted 10 lines
{
	while (!arch_spin_trylock(l)) {
	      if (!irq_disabled_flags(flags)) {
	      	      arch_irq_restore(flags);
		      cpu_relax();
		      arch_irq_disable();
	      }
	}
}
Quoted 1 line

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

Sigh. You're an sparc64 and mn10300 assembler expert, right?

Quoted 2 lines

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.

Quoted 1 line

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);
}
Quoted 6 lines
Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 5 lines
Quoted 5 lines

spin_lock_irqsave() semantics are:

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

Quoted 7 lines
Quoted 3 lines

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.

Quoted 2 lines

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

author
#8 lore raw
Quoted 17 lines · Thomas Gleixner
Quoted 16 lines · Chen Gang
Quoted 16 lines · Thomas Gleixner
Quoted 14 lines
Quoted 14 lines

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();
	      }
	}
}
Quoted 3 lines
Quoted 1 line
Quoted 1 line

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" ?

Quoted 9 lines
Quoted 2 lines
Quoted 2 lines

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.

Quoted 1 line
Quoted 1 line

For x86 and parisc, it like your demo above.

For parisc, yes.

Quoted 8 lines

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)

Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines

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.

Quoted 8 lines
Quoted 2 lines
Quoted 2 lines

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.

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

spin_lock_irqsave() semantics are:

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

Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines

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.

Signature · 3 lines
Chen Gang

Asianux Corporation
#9 lore raw
Quoted 18 lines · Chen Gang
Quoted 17 lines · Thomas Gleixner
Quoted 16 lines · Chen Gang
Quoted 16 lines · Thomas Gleixner
Quoted 14 lines
Quoted 14 lines

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();
	      }
	}
}
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line

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.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines

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.

Quoted 8 lines

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

author
#10 lore raw
Quoted 19 lines · Thomas Gleixner
Quoted 18 lines · Chen Gang
Quoted 18 lines · Thomas Gleixner
Quoted 17 lines
Quoted 17 lines · Chen Gang
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines · Thomas Gleixner
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines

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();
}
}
}

Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

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'.

Quoted 10 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

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.

Quoted 19 lines
Quoted 8 lines
Quoted 8 lines

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);
Quoted 3 lines

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

Signature · 3 lines
Chen Gang

Asianux Corporation
author
#11 lore raw
In reply to Thomas Gleixner
Quoted 7 lines · Thomas Gleixner

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.

Signature · 3 lines
Chen Gang

Asianux Corporation
#12 lore raw
Quoted 10 lines · Chen Gang
Quoted 7 lines · Thomas Gleixner

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

author
#13 lore raw
Quoted 11 lines · Thomas Gleixner
Quoted 9 lines · Chen Gang
Quoted 9 lines · Thomas Gleixner
Quoted 7 lines
Quoted 7 lines

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);
Quoted 8 lines
Quoted 1 line
Quoted 1 line

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.

Quoted 3 lines

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

Yes.

Quoted 8 lines

So

	local_irq_save(flags);
	spin_lock(&lock);

is semantically the same as

	spin_lock_irqsave(&lock, flags);

Yes (but reverse is NO).

Quoted 1 line

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.

Signature · 3 lines
Chen Gang

Asianux Corporation
#14 lore raw
Quoted 16 lines · Chen Gang
Quoted 8 lines · Thomas Gleixner

So

	local_irq_save(flags);
	spin_lock(&lock);

is semantically the same as

	spin_lock_irqsave(&lock, flags);

Yes (but reverse is NO).

Quoted 1 line

And this is completely independent of LOCKDEP.

NO.

Review

Quoted 6 lines
(quoted diff) +0 -0
 	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.

Quoted 2 lines

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

author
#15 lore raw
Quoted 17 lines · Thomas Gleixner
Quoted 16 lines · Chen Gang
Quoted 16 lines · Thomas Gleixner
Quoted 8 lines
Quoted 8 lines

So

	local_irq_save(flags);
	spin_lock(&lock);

is semantically the same as

	spin_lock_irqsave(&lock, flags);

Yes (but reverse is NO).

Quoted 1 line
Quoted 1 line

And this is completely independent of LOCKDEP.

NO.

Quoted 9 lines
Quoted 6 lines

Review

Quoted 6 lines
(quoted diff) +0 -0
 	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.

Quoted 10 lines

You seem to have a different definition of semantics, but I prefer the common one.

Quoted 2 lines
Quoted 2 lines

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.

Signature · 3 lines
Chen Gang

Asianux Corporation
#16 lore raw

Chen,

Quoted 10 lines · Chen Gang
Quoted 3 lines · Thomas Gleixner

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.

Quoted 6 lines
Quoted 5 lines

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

     local_irq_save(flags);
     arch_spin_lock_flags(l, flags);
Quoted 6 lines

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

author
#17 lore raw
Quoted 39 lines · Thomas Gleixner

Chen,

Quoted 10 lines · Chen Gang
Quoted 10 lines · Thomas Gleixner
Quoted 3 lines
Quoted 3 lines

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.

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines

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

     local_irq_save(flags);
     arch_spin_lock_flags(l, flags);
Quoted 6 lines
Quoted 6 lines

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:

(quoted diff) +0 -0
 	local_irq_save(flags);
 	spin_lock(l);
Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

:-)

Quoted 8 lines

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.

Signature · 3 lines
Chen Gang

Asianux Corporation
#18 lore raw
In reply to Chen Gang
Quoted 9 lines · Chen Gang
(quoted diff) +0 -0
 	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.