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

From: Suresh Thiagarajan
Date: Thu Jan 02 2014 - 05:31:15 EST

Next message: Matt Fleming: "Re: [PATCH v7 00/12] kexec kernel efi runtime support"
Previous message: Ding Tianhong: "Re: [PATCH RESEND net-next 1/7] bonding: use ether_addr_equal_unalignedfor bond addr compare"
Next in thread: Dan Williams: "Re: spinlock_irqsave() && flags (Was: pm80xx: Spinlock fix)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Dec 27, 2013 at 9:48 PM, Oleg Nesterov <oleg@xxxxxxxxxx> 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);
> + 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);
> - 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;
> @@ -2617,11 +2605,7 @@ mpi_sata_completion(struct pm8001_hba_in
> IO_DS_NON_OPERATIONAL);
> 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;
> @@ -2641,11 +2625,7 @@ mpi_sata_completion(struct pm8001_hba_in
> IO_DS_IN_ERROR);
> 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;
> @@ -2674,20 +2654,9 @@ mpi_sata_completion(struct pm8001_hba_in
> " resp 0x%x stat 0x%x but aborted by upper layer!\n",
> t, status, ts->resp, ts->stat));
> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
> - } 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) {
> + } else {
> 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);
> + pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> }
> }
>
> @@ -2796,11 +2765,7 @@ static void mpi_sata_event(struct pm8001
> IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
> ts->resp = SAS_TASK_COMPLETE;
> 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;
> @@ -2909,20 +2874,9 @@ static void mpi_sata_event(struct pm8001
> " resp 0x%x stat 0x%x but aborted by upper layer!\n",
> t, event, ts->resp, ts->stat));
> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
> - } 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) {
> + } else {
> 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);
> + pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> }
> }
>
> @@ -4467,23 +4421,10 @@ static int pm8001_chip_sata_req(struct p
> " stat 0x%x but aborted by upper layer "
> "\n", task, ts->resp, ts->stat));
> pm8001_ccb_task_free(pm8001_ha, task, ccb, tag);
> - } else if (task->uldd_task) {
> - spin_unlock_irqrestore(&task->task_state_lock,
> - flags);
> - pm8001_ccb_task_free(pm8001_ha, task, ccb, tag);
> - mb();/* ditto */
> - spin_unlock_irq(&pm8001_ha->lock);
> - task->task_done(task);
> - spin_lock_irq(&pm8001_ha->lock);
> - return 0;
> - } else if (!task->uldd_task) {
> + } else {
> spin_unlock_irqrestore(&task->task_state_lock,
> flags);
> - pm8001_ccb_task_free(pm8001_ha, task, ccb, tag);
> - mb();/*ditto*/
> - spin_unlock_irq(&pm8001_ha->lock);
> - task->task_done(task);
> - spin_lock_irq(&pm8001_ha->lock);
> + pm8001_ccb_task_free_done(pm8001_ha, task, ccb, tag);
> return 0;
> }
> }
> --- x/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ x/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -2175,11 +2175,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;
> @@ -2195,11 +2191,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;
> @@ -2221,11 +2213,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);
> - 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;
> @@ -2288,11 +2276,7 @@ mpi_sata_completion(struct pm8001_hba_in
> IO_DS_NON_OPERATIONAL);
> 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;
> @@ -2312,11 +2296,7 @@ mpi_sata_completion(struct pm8001_hba_in
> IO_DS_IN_ERROR);
> 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;
> @@ -2345,20 +2325,9 @@ mpi_sata_completion(struct pm8001_hba_in
> " resp 0x%x stat 0x%x but aborted by upper layer!\n",
> t, status, ts->resp, ts->stat));
> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
> - } 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) {
> + } else {
> 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);
> + pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> }
> }
>
> @@ -2470,11 +2439,7 @@ static void mpi_sata_event(struct pm8001
> IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
> ts->resp = SAS_TASK_COMPLETE;
> 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;
> @@ -2596,20 +2561,9 @@ static void mpi_sata_event(struct pm8001
> " resp 0x%x stat 0x%x but aborted by upper layer!\n",
> t, event, ts->resp, ts->stat));
> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
> - } 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) {
> + } else {
> 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);
> + pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> }
> }
>
> @@ -4304,23 +4258,10 @@ static int pm80xx_chip_sata_req(struct p
> "\n", task, ts->resp, ts->stat));
> pm8001_ccb_task_free(pm8001_ha, task, ccb, tag);
> return 0;
> - } else if (task->uldd_task) {
> - spin_unlock_irqrestore(&task->task_state_lock,
> - flags);
> - pm8001_ccb_task_free(pm8001_ha, task, ccb, tag);
> - mb();/* ditto */
> - spin_unlock_irq(&pm8001_ha->lock);
> - task->task_done(task);
> - spin_lock_irq(&pm8001_ha->lock);
> - return 0;
> - } else if (!task->uldd_task) {
> + } else {
> spin_unlock_irqrestore(&task->task_state_lock,
> flags);
> - pm8001_ccb_task_free(pm8001_ha, task, ccb, tag);
> - mb();/*ditto*/
> - spin_unlock_irq(&pm8001_ha->lock);
> - task->task_done(task);
> - spin_lock_irq(&pm8001_ha->lock);
> + pm8001_ccb_task_free_done(pm8001_ha, task, ccb, tag);
> return 0;
> }
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Matt Fleming: "Re: [PATCH v7 00/12] kexec kernel efi runtime support"
Previous message: Ding Tianhong: "Re: [PATCH RESEND net-next 1/7] bonding: use ether_addr_equal_unalignedfor bond addr compare"
Next in thread: Dan Williams: "Re: spinlock_irqsave() && flags (Was: pm80xx: Spinlock fix)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]