On Fri, Dec 27, 2013 at 9:48 PM, Oleg Nesterov <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 2, 2014 at 2:31 AM, Suresh Thiagarajan
<[email protected]> wrote:
>
>
> On Fri, Dec 27, 2013 at 9:48 PM, Oleg Nesterov <[email protected]> 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