Re: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)

[Dan Williams]

On Thu, Jan 2, 2014 at 2:31 AM, Suresh Thiagarajan
suresh.thiagara...@pmcs.com wrote:


 On Fri, Dec 27, 2013 at 9:48 PM, Oleg Nesterov o...@redhat.com wrote:
 On 12/24, Suresh Thiagarajan wrote:

 Below is a small pseudo code on protecting/serializing the flag for global 
 access.
 struct temp
 {
   ...
   spinlock_t lock;
   unsigned long lock_flags;
 };
 void my_lock(struct temp *t)
 {
unsigned long flag; // thread-private variable as suggested
spin_lock_irqsave(t-lock, flag);
t-lock_flags = flag; //updating inside critical section now 
 to serialize the access to flag
 }

 void my_unlock(struct temp *t)
 {
unsigned long flag = t-lock_flags;
t-lock_flags = 0;  //clearing it before getting out of 
 critical section
spin_unlock_irqrestore(t-lock, flag);
 }

 Yes, this should work as a quick fix. And you do not need to clear 
 -lock_flags
 in my_unlock().

 But when I look at original patch again, I no longer understand why do
 you need pm8001_ha-lock_flags at all. Of course I do not understand this
 code, I am sure I missed something, but at first glance it seems that only
 this sequence

 spin_unlock_irq(pm8001_ha-lock);
 t-task_done(t);
 spin_lock_irq(pm8001_ha-lock);

 should be fixed?

 If yes, why you can't simply do spin_unlock() + spin_lock() around
 t-task_done() ? This won't enable irqs, but spin_unlock_irqrestore()
 doesn't necessarily enables irqs too, so -task_done() can run with
 irqs disabled?

 And note that the pattern above has a lot of users, perhaps it makes
 sense to start with something like the patch below?

 Thanks James, Oleg and all for your inputs.
 Will start with review and testing this patch and then work/investigate to 
 keep shortest and clearest critical
 section so that we can have the lock and unlock within the same routine.


Fwiw we solved this in libsas a while back with a similar pattern
proposed by Oleg:

unsigned long flags;

local_irq_save(flags);
spin_unlock(lock);
...
spin_lock_lock(lock);
local_irq_restore(flags);

See commit 312d3e56119a [SCSI] libsas: remove ata_port.lock
management duties from lldds

--
Dan
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)

[Suresh Thiagarajan]

On Fri, Dec 27, 2013 at 9:48 PM, Oleg Nesterov o...@redhat.com wrote:
 On 12/24, Suresh Thiagarajan wrote:

 Below is a small pseudo code on protecting/serializing the flag for global 
 access.
 struct temp
 {
   ...
   spinlock_t lock;
   unsigned long lock_flags;
 };
 void my_lock(struct temp *t)
 {
unsigned long flag; // thread-private variable as suggested
spin_lock_irqsave(t-lock, flag);
t-lock_flags = flag; //updating inside critical section now 
 to serialize the access to flag
 }

 void my_unlock(struct temp *t)
 {
unsigned long flag = t-lock_flags;
t-lock_flags = 0;  //clearing it before getting out of 
 critical section
spin_unlock_irqrestore(t-lock, flag);
 }

 Yes, this should work as a quick fix. And you do not need to clear 
 -lock_flags
 in my_unlock().

 But when I look at original patch again, I no longer understand why do
 you need pm8001_ha-lock_flags at all. Of course I do not understand this
 code, I am sure I missed something, but at first glance it seems that only
 this sequence

 spin_unlock_irq(pm8001_ha-lock);
 t-task_done(t);
 spin_lock_irq(pm8001_ha-lock);

 should be fixed?

 If yes, why you can't simply do spin_unlock() + spin_lock() around
 t-task_done() ? This won't enable irqs, but spin_unlock_irqrestore()
 doesn't necessarily enables irqs too, so -task_done() can run with
 irqs disabled?

 And note that the pattern above has a lot of users, perhaps it makes
 sense to start with something like the patch below?

Thanks James, Oleg and all for your inputs.
Will start with review and testing this patch and then work/investigate to keep 
shortest and clearest critical 
section so that we can have the lock and unlock within the same routine.

Regards,
Suresh

 Hmm. there is another oddity in this code, for example mpi_sata_completion()
 does

 } else if (t-uldd_task) {
 spin_unlock_irqrestore(t-task_state_lock, flags);
 pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
 mb();/* ditto */
 spin_unlock_irq(pm8001_ha-lock);
 t-task_done(t);
 spin_lock_irq(pm8001_ha-lock);
 } else if (!t-uldd_task) {
 spin_unlock_irqrestore(t-task_state_lock, flags);
 pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
 mb();/*ditto*/
 spin_unlock_irq(pm8001_ha-lock);
 t-task_done(t);
 spin_lock_irq(pm8001_ha-lock);
 }

 and both branches do the same code? mpi_sata_event(), pm8001_chip_sata_req()
 too.

 Imho, whatever you do, the changelog should tell more.

 Oleg.


 --- x/drivers/scsi/pm8001/pm8001_sas.h
 +++ x/drivers/scsi/pm8001/pm8001_sas.h
 @@ -619,6 +619,20 @@ u32 pm8001_get_ncq_tag(struct sas_task *
  void pm8001_ccb_free(struct pm8001_hba_info *pm8001_ha, u32 ccb_idx);
  void pm8001_ccb_task_free(struct pm8001_hba_info *pm8001_ha,
 struct sas_task *task, struct pm8001_ccb_info *ccb, u32 ccb_idx);
 +
 +static inline void
 +pm8001_ccb_task_free_done(struct pm8001_hba_info *pm8001_ha,
 +   struct sas_task *task, struct pm8001_ccb_info *ccb,
 +   u32 ccb_idx)
 +{
 +   pm8001_ccb_task_free(pm8001_ha, task, ccb, ccb_idx);
 +   smp_mb(); /* comment */
 +   spin_unlock(pm8001_ha-lock);
 +   task-task_done(task);
 +   spin_lock(pm8001_ha-lock);
 +}
 +
 +
  int pm8001_phy_control(struct asd_sas_phy *sas_phy, enum phy_func func,
 void *funcdata);
  void pm8001_scan_start(struct Scsi_Host *shost);
 --- x/drivers/scsi/pm8001/pm8001_hwi.c
 +++ x/drivers/scsi/pm8001/pm8001_hwi.c
 @@ -2502,11 +2502,7 @@ mpi_sata_completion(struct pm8001_hba_in
 IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
 ts-resp = SAS_TASK_UNDELIVERED;
 ts-stat = SAS_QUEUE_FULL;
 -   pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
 -   mb();/*in order to force CPU ordering*/
 -   spin_unlock_irq(pm8001_ha-lock);
 -   t-task_done(t);
 -   spin_lock_irq(pm8001_ha-lock);
 +   pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
 return;
 }
 break;
 @@ -2522,11 +2518,7 @@ mpi_sata_completion(struct pm8001_hba_in
 IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
 ts-resp = SAS_TASK_UNDELIVERED;
 ts-stat = SAS_QUEUE_FULL;
 -   pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
 -   mb();/*ditto*/
 -   spin_unlock_irq(pm8001_ha-lock);
 -   t-task_done(t);
 -   spin_lock_irq(pm8001_ha-lock);
 +   

Re: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)

[Oleg Nesterov]

On 12/24, Suresh Thiagarajan wrote:

 Below is a small pseudo code on protecting/serializing the flag for global 
 access.
 struct temp
 {
   ...
   spinlock_t lock;
   unsigned long lock_flags;
 };
 void my_lock(struct temp *t)
 {
unsigned long flag; // thread-private variable as suggested
spin_lock_irqsave(t-lock, flag);
t-lock_flags = flag; //updating inside critical section now 
 to serialize the access to flag
 }

 void my_unlock(struct temp *t)
 {
unsigned long flag = t-lock_flags;
t-lock_flags = 0;  //clearing it before getting out of 
 critical section
spin_unlock_irqrestore(t-lock, flag);
 }

Yes, this should work as a quick fix. And you do not need to clear -lock_flags
in my_unlock().

But when I look at original patch again, I no longer understand why do
you need pm8001_ha-lock_flags at all. Of course I do not understand this
code, I am sure I missed something, but at first glance it seems that only
this sequence

spin_unlock_irq(pm8001_ha-lock);
t-task_done(t);
spin_lock_irq(pm8001_ha-lock);

should be fixed?

If yes, why you can't simply do spin_unlock() + spin_lock() around
t-task_done() ? This won't enable irqs, but spin_unlock_irqrestore()
doesn't necessarily enables irqs too, so -task_done() can run with
irqs disabled?

And note that the pattern above has a lot of users, perhaps it makes
sense to start with something like the patch below?

Hmm. there is another oddity in this code, for example mpi_sata_completion()
does

} else if (t-uldd_task) {
spin_unlock_irqrestore(t-task_state_lock, flags);
pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
mb();/* ditto */
spin_unlock_irq(pm8001_ha-lock);
t-task_done(t);
spin_lock_irq(pm8001_ha-lock);
} else if (!t-uldd_task) {
spin_unlock_irqrestore(t-task_state_lock, flags);
pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
mb();/*ditto*/
spin_unlock_irq(pm8001_ha-lock);
t-task_done(t);
spin_lock_irq(pm8001_ha-lock);
}

and both branches do the same code? mpi_sata_event(), pm8001_chip_sata_req()
too.

Imho, whatever you do, the changelog should tell more.

Oleg.


--- x/drivers/scsi/pm8001/pm8001_sas.h
+++ x/drivers/scsi/pm8001/pm8001_sas.h
@@ -619,6 +619,20 @@ u32 pm8001_get_ncq_tag(struct sas_task *
 void pm8001_ccb_free(struct pm8001_hba_info *pm8001_ha, u32 ccb_idx);
 void pm8001_ccb_task_free(struct pm8001_hba_info *pm8001_ha,
struct sas_task *task, struct pm8001_ccb_info *ccb, u32 ccb_idx);
+
+static inline void
+pm8001_ccb_task_free_done(struct pm8001_hba_info *pm8001_ha,
+   struct sas_task *task, struct pm8001_ccb_info *ccb,
+   u32 ccb_idx)
+{
+   pm8001_ccb_task_free(pm8001_ha, task, ccb, ccb_idx);
+   smp_mb(); /* comment */
+   spin_unlock(pm8001_ha-lock);
+   task-task_done(task);
+   spin_lock(pm8001_ha-lock);
+}
+
+
 int pm8001_phy_control(struct asd_sas_phy *sas_phy, enum phy_func func,
void *funcdata);
 void pm8001_scan_start(struct Scsi_Host *shost);
--- x/drivers/scsi/pm8001/pm8001_hwi.c
+++ x/drivers/scsi/pm8001/pm8001_hwi.c
@@ -2502,11 +2502,7 @@ mpi_sata_completion(struct pm8001_hba_in
IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
ts-resp = SAS_TASK_UNDELIVERED;
ts-stat = SAS_QUEUE_FULL;
-   pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
-   mb();/*in order to force CPU ordering*/
-   spin_unlock_irq(pm8001_ha-lock);
-   t-task_done(t);
-   spin_lock_irq(pm8001_ha-lock);
+   pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
return;
}
break;
@@ -2522,11 +2518,7 @@ mpi_sata_completion(struct pm8001_hba_in
IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
ts-resp = SAS_TASK_UNDELIVERED;
ts-stat = SAS_QUEUE_FULL;
-   pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
-   mb();/*ditto*/
-   spin_unlock_irq(pm8001_ha-lock);
-   t-task_done(t);
-   spin_lock_irq(pm8001_ha-lock);
+   pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
return;
}
break;
@@ -2550,11 +2542,7 @@ mpi_sata_completion(struct pm8001_hba_in
IO_OPEN_CNX_ERROR_STP_RESOURCES_BUSY);
ts-resp = SAS_TASK_UNDELIVERED;
ts-stat = SAS_QUEUE_FULL;
-   pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);

Re: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)

[Ingo Molnar]

* Oleg Nesterov o...@redhat.com wrote:

 On 12/23, Ingo Molnar wrote:
 
  * Oleg Nesterov o...@redhat.com wrote:
 
   Initially I thought that this is obviously wrong, irqsave/irqrestore
   assume that flags is owned by the caller, not by the lock. And
   iirc this was certainly wrong in the past.
  
   But when I look at spinlock.c it seems that this code can actually
   work. _irqsave() writes to FLAGS after it takes the lock, and
   _irqrestore() has a copy of FLAGS before it drops this lock.
 
  I don't think that's true: if it was then the lock would not be
  irqsave, a hardware-irq could come in after the lock has been taken
  and before flags are saved+disabled.
 
 I do agree that this pattern is not safe, that is why I decided to ask.
 
 But, unless I missed something, with the current implementation
 spin_lock_irqsave(lock, global_flags) does:
 
   unsigned long local_flags;
 
   local_irq_save(local_flags);
   spin_lock(lock);
 
   global_flags = local_flags;
 
 so the access to global_flags is actually serialized by lock.

You are right, today that's true technically because IIRC due to Sparc 
quirks we happen to return 'flags' as a return value - still it's very 
ugly and it could break anytime if we decide to do more aggressive 
optimizations and actually directly save into 'flags'.

Note that even today there's a narrow exception: on UP we happen to 
build it the other way around, so that we do:

local_irq_save(global_flags);
__acquire(lock);

This does not matter for any real code because on UP there is no 
physical lock and __acquire() is empty code-wise, but any compiler 
driven locking analysis tool using __attribute__ __context__(), if 
built on UP, would see the unsafe locking pattern.

Thanks,

Ingo
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)

[Suresh Thiagarajan]

On Tue, Dec 24, 2013 at 1:59 PM, Ingo Molnar mi...@kernel.org wrote:

 * Oleg Nesterov o...@redhat.com wrote:

 On 12/23, Ingo Molnar wrote:
 
  * Oleg Nesterov o...@redhat.com wrote:
 
   Initially I thought that this is obviously wrong, irqsave/irqrestore
   assume that flags is owned by the caller, not by the lock. And
   iirc this was certainly wrong in the past.
  
   But when I look at spinlock.c it seems that this code can actually
   work. _irqsave() writes to FLAGS after it takes the lock, and
   _irqrestore() has a copy of FLAGS before it drops this lock.
 
  I don't think that's true: if it was then the lock would not be
  irqsave, a hardware-irq could come in after the lock has been taken
  and before flags are saved+disabled.

 I do agree that this pattern is not safe, that is why I decided to ask.

 But, unless I missed something, with the current implementation
 spin_lock_irqsave(lock, global_flags) does:

   unsigned long local_flags;

   local_irq_save(local_flags);
   spin_lock(lock);

   global_flags = local_flags;

 so the access to global_flags is actually serialized by lock.

Below is a small pseudo code on protecting/serializing the flag for global 
access.
struct temp
{
...
spinlock_t lock;
unsigned long lock_flags;
};
void my_lock(struct temp *t)
{
   unsigned long flag; // thread-private variable as suggested
   spin_lock_irqsave(t-lock, flag);
   t-lock_flags = flag; //updating inside critical section now to 
serialize the access to flag
}

void my_unlock(struct temp *t)
{
   unsigned long flag = t-lock_flags;
   t-lock_flags = 0;  //clearing it before getting out of critical 
section
   spin_unlock_irqrestore(t-lock, flag);
}

Here for unlocking, I could even use spin_unlock_irqrestore(t-lock, 
t-lock_flags) directly instead of my_unlock() since t-lock_flags is updated 
only in my_lock and so there is no need to explicitly clear t-lock_flags. 
Please let me know if I miss anything here in serializing the global lock flag.

Thanks,
Suresh

 You are right, today that's true technically because IIRC due to Sparc
 quirks we happen to return 'flags' as a return value - still it's very
 ugly and it could break anytime if we decide to do more aggressive
 optimizations and actually directly save into 'flags'.

 Note that even today there's a narrow exception: on UP we happen to
 build it the other way around, so that we do:

 local_irq_save(global_flags);
 __acquire(lock);

 This does not matter for any real code because on UP there is no
 physical lock and __acquire() is empty code-wise, but any compiler
 driven locking analysis tool using __attribute__ __context__(), if
 built on UP, would see the unsafe locking pattern.

 Thanks,

 Ingo
 --
 To unsubscribe from this list: send the line unsubscribe linux-scsi in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)

[James Bottomley]

On Tue, 2013-12-24 at 09:13 +, Suresh Thiagarajan wrote:
 
 On Tue, Dec 24, 2013 at 1:59 PM, Ingo Molnar mi...@kernel.org wrote:
 
  * Oleg Nesterov o...@redhat.com wrote:
 
  On 12/23, Ingo Molnar wrote:
  
   * Oleg Nesterov o...@redhat.com wrote:
  
Initially I thought that this is obviously wrong, irqsave/irqrestore
assume that flags is owned by the caller, not by the lock. And
iirc this was certainly wrong in the past.
   
But when I look at spinlock.c it seems that this code can actually
work. _irqsave() writes to FLAGS after it takes the lock, and
_irqrestore() has a copy of FLAGS before it drops this lock.
  
   I don't think that's true: if it was then the lock would not be
   irqsave, a hardware-irq could come in after the lock has been taken
   and before flags are saved+disabled.
 
  I do agree that this pattern is not safe, that is why I decided to ask.
 
  But, unless I missed something, with the current implementation
  spin_lock_irqsave(lock, global_flags) does:
 
unsigned long local_flags;
 
local_irq_save(local_flags);
spin_lock(lock);
 
global_flags = local_flags;
 
  so the access to global_flags is actually serialized by lock.
 
 Below is a small pseudo code on protecting/serializing the flag for global 
 access.
 struct temp
 {
   ...
   spinlock_t lock;
   unsigned long lock_flags;
 };
 void my_lock(struct temp *t)
 {
unsigned long flag; // thread-private variable as suggested
spin_lock_irqsave(t-lock, flag);
t-lock_flags = flag; //updating inside critical section now 
 to serialize the access to flag
 }
 
 void my_unlock(struct temp *t)
 {
unsigned long flag = t-lock_flags;
t-lock_flags = 0;  //clearing it before getting out of 
 critical section
spin_unlock_irqrestore(t-lock, flag);
 }
 
 Here for unlocking, I could even use spin_unlock_irqrestore(t-lock,
 t-lock_flags) directly instead of my_unlock() since t-lock_flags is
 updated only in my_lock and so there is no need to explicitly clear
 t-lock_flags. Please let me know if I miss anything here in
 serializing the global lock flag.

I don't think anyone's arguing that you can't do this.  The argument is
more you shouldn't: Lock contention is one of the biggest killers of
performance and getting locking right (avoiding inversions, or even
nested locks at all) is hard.  Therefore you need to keep the shortest
and clearest critical sections in Linux that you can, so the pattern for
locking is to lock and unlock in the same routine with a local flags
variable.

So, rather than develop a primitive that supports and encourages uses of
locking that violate the pattern and is far more likely to cause
performance and other problems, could you just audit the code to see if
the need to carry a lock across a routine could be eliminated (either by
shortening the critical section or by better nesting the calls).

Thanks,

James


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)

[Oleg Nesterov]

On 12/23, Oleg Nesterov wrote:

 Perhaps we should ask the maintainers upstream? Even if this works, I am
 not sure this is _supposed_ to work. I mean, in theory spin_lock_irqave()
 can be changed as, say

   #define spin_lock_irqsave(lock, flags)  \
   do {\
   local_irq_save(flags);  \
   spin_lock(lock);\
   } while (0)

 (and iirc it was defined this way a long ago). In this case flags is
 obviously not protected.

Yes, lets ask the maintainers.

In short, is this code

spinlock_t LOCK;
unsigned long FLAGS;

void my_lock(void)
{
spin_lock_irqsave(LOCK, FLAGS);
}

void my_unlock(void)
{
spin_unlock_irqrestore(LOCK, FLAGS);
}

correct or not?

Initially I thought that this is obviously wrong, irqsave/irqrestore
assume that flags is owned by the caller, not by the lock. And iirc
this was certainly wrong in the past.

But when I look at spinlock.c it seems that this code can actually work.
_irqsave() writes to FLAGS after it takes the lock, and _irqrestore()
has a copy of FLAGS before it drops this lock.

And it turns out, some users assume this should work, for example

arch/arm/mach-omap2/powerdomain.c:
pwrdm_lock() and pwrdm_unlock()

drivers/net/wireless/brcm80211/brcmfmac/fwsignal.c:
brcmf_fws_lock() and brcmf_fws_unlock()

seem to do exactly this. Plus the pending patch for drivers/scsi/pm8001/.

So is it documented somewhere that this sequence is correct, or the code
above should be changed even if it happens to work?

Oleg.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)

[Linus Torvalds]

On Mon, Dec 23, 2013 at 9:27 AM, Oleg Nesterov o...@redhat.com wrote:

 In short, is this code

 spinlock_t LOCK;
 unsigned long FLAGS;

 void my_lock(void)
 {
 spin_lock_irqsave(LOCK, FLAGS);
 }

 void my_unlock(void)
 {
 spin_unlock_irqrestore(LOCK, FLAGS);
 }

 correct or not?

Hell no. flags needs to be a thread-private variable, or at least
protected some way (ie the above could work if everything is inside a
bigger lock, to serialize access to FLAGS).

Linus
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)

[Oleg Nesterov]

On 12/23, Linus Torvalds wrote:

 On Mon, Dec 23, 2013 at 9:27 AM, Oleg Nesterov o...@redhat.com wrote:
 
  In short, is this code
 
  spinlock_t LOCK;
  unsigned long FLAGS;
 
  void my_lock(void)
  {
  spin_lock_irqsave(LOCK, FLAGS);
  }
 
  void my_unlock(void)
  {
  spin_unlock_irqrestore(LOCK, FLAGS);
  }
 
  correct or not?

 Hell no. flags needs to be a thread-private variable, or at least
 protected some way (ie the above could work if everything is inside a
 bigger lock, to serialize access to FLAGS).

This was my understanding (although, once again, it seems to me this can
suprisingly work with the current implementation).

However, the code above already has the users. Do you think it makes
sense to add something like

void spinlock_irqsave_careful(spinlock_t *lock, unsigned long *flags)
{
unsigned long _flags;
spinlock_irqsave(lock, _flags);
*flags = flags;
}

void spinlock_irqrestore_careful(spinlock_t *lock, unsigned long *flags)
{
unsigned long _flags = *flags;
spinlock_irqrestore(lock, _flags);
}

into include/linux/spinlock.h ?

Oleg.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)

[Ingo Molnar]

* Oleg Nesterov o...@redhat.com wrote:

 On 12/23, Oleg Nesterov wrote:
 
  Perhaps we should ask the maintainers upstream? Even if this works, I am
  not sure this is _supposed_ to work. I mean, in theory spin_lock_irqave()
  can be changed as, say
 
  #define spin_lock_irqsave(lock, flags)  \
  do {\
  local_irq_save(flags);  \
  spin_lock(lock);\
  } while (0)
 
  (and iirc it was defined this way a long ago). In this case flags is
  obviously not protected.
 
 Yes, lets ask the maintainers.
 
 In short, is this code
 
   spinlock_t LOCK;
   unsigned long FLAGS;
 
   void my_lock(void)
   {
   spin_lock_irqsave(LOCK, FLAGS);
   }
 
   void my_unlock(void)
   {
   spin_unlock_irqrestore(LOCK, FLAGS);
   }
 
 correct or not?
 
 Initially I thought that this is obviously wrong, irqsave/irqrestore 
 assume that flags is owned by the caller, not by the lock. And 
 iirc this was certainly wrong in the past.
 
 But when I look at spinlock.c it seems that this code can actually 
 work. _irqsave() writes to FLAGS after it takes the lock, and 
 _irqrestore() has a copy of FLAGS before it drops this lock.

I don't think that's true: if it was then the lock would not be 
irqsave, a hardware-irq could come in after the lock has been taken 
and before flags are saved+disabled.

So AFAICS this is an unsafe pattern, beyond being ugly as hell.

Thanks,

Ingo
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)

[Oleg Nesterov]

On 12/23, Ingo Molnar wrote:

 * Oleg Nesterov o...@redhat.com wrote:

  Initially I thought that this is obviously wrong, irqsave/irqrestore
  assume that flags is owned by the caller, not by the lock. And
  iirc this was certainly wrong in the past.
 
  But when I look at spinlock.c it seems that this code can actually
  work. _irqsave() writes to FLAGS after it takes the lock, and
  _irqrestore() has a copy of FLAGS before it drops this lock.

 I don't think that's true: if it was then the lock would not be
 irqsave, a hardware-irq could come in after the lock has been taken
 and before flags are saved+disabled.

I do agree that this pattern is not safe, that is why I decided to ask.

But, unless I missed something, with the current implementation
spin_lock_irqsave(lock, global_flags) does:

unsigned long local_flags;

local_irq_save(local_flags);
spin_lock(lock);

global_flags = local_flags;

so the access to global_flags is actually serialized by lock.

 So AFAICS this is an unsafe pattern, beyond being ugly as hell.

Yes, I think the same.

Oleg.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)

[Linus Torvalds]

On Mon, Dec 23, 2013 at 10:24 AM, Oleg Nesterov o...@redhat.com wrote:

 However, the code above already has the users. Do you think it makes
 sense to add something like

No. I think it makes sense to put a big warning on any users you find,
and fart in the general direction of any developer who did that broken
pattern.

Because code like that is obviously crap.

Linus
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html