On 09/08/2016 12:23 PM, Gabriel Krisman Bertazi wrote:
>> @@ -1868,6 +1872,48 @@ static inline void ipr_free_cmd(struct i
>> ipr_cmd->hrrq->active_map);
>> }
>>
>> +static inline void ipr_hrrq_activate(struct ipr_hrr_queue *hrrq)
>> +{
>> + if (!hrrq->is_active) {
>> + atomic_inc(&hrrq->active);
>> + atomic_inc(&hrrq->ioa_cfg->hrrq_active);
>> + hrrq->is_active = 1;
>> + }
>> +}
>> +
>> +static inline void ipr_hrrq_deactivate(struct ipr_hrr_queue *hrrq)
>> +{
>> + if (hrrq->is_active) {
>> + hrrq->is_active = 0;
>> +
>> + if (!atomic_dec_return(&hrrq->active))
>
> Shouldn't this set hrrq->active back to 0, instead?
Not sure I follow. When an HRRQ is available for use, the hrrq->active refcount
is
set to 1. It gets incremented by ipr_hrrq_enter and decremented by
ipr_hrrq_exit.
So its either ipr_hrrq_deactivate that decrements hrrq->active to zero, in which
case, we can then decrement ioa_cfg->hrrq_active, or we have to wait for one
or more ipr_hrrq_exit calls to decrement httq->active to zero so that we can
proceed with the reset job.
>
>> + atomic_dec(&hrrq->ioa_cfg->hrrq_active);
>> + }
>> +}
>> +
>> +static inline int ipr_hrrq_enter(struct ipr_hrr_queue *hrrq)
>> +{
>> + return atomic_inc_not_zero(&hrrq->active);
>> +}
>> +
>> +static void ipr_reset_ioa_job(struct ipr_cmnd *);
>> +
>> +static inline void ipr_hrrq_exit(struct ipr_hrr_queue *hrrq)
>> +{
>> + unsigned long flags;
>> + struct ipr_ioa_cfg *ioa_cfg;
>> +
>> + if (!atomic_dec_return(&hrrq->active)) {
>> + ioa_cfg = hrrq->ioa_cfg;
>> +
>> + if (!atomic_dec_return(&ioa_cfg->hrrq_active)) {
>> + spin_lock_irqsave(ioa_cfg->host->host_lock, flags);
>> + ipr_reset_ioa_job(ioa_cfg->reset_cmd);
>> + spin_unlock_irqrestore(ioa_cfg->host->host_lock, flags);
>> + }
>> + }
>> +}
>> +
>> static inline struct ipr_cmnd* ipr_first_active_cmd(struct ipr_hrr_queue
>> *hrrq)
>> {
>> int i = find_first_bit(hrrq->active_map, hrrq->size);
>> diff -puN drivers/scsi/ipr.c~ipr_lockless_atomic drivers/scsi/ipr.c
>> --- linux-2.6.git/drivers/scsi/ipr.c~ipr_lockless_atomic 2016-09-06
>> 21:01:16.264239271 -0500
>> +++ linux-2.6.git-bjking1/drivers/scsi/ipr.c 2016-09-06 21:07:15.311878103
>> -0500
>> @@ -6455,10 +6455,8 @@ static void ipr_scsi_done(struct ipr_cmn
>>
>> if (likely(IPR_IOASC_SENSE_KEY(ioasc) == 0)) {
>> scsi_dma_unmap(scsi_cmd);
>> - spin_lock_irqsave(ipr_cmd->hrrq->lock, lock_flags);
>> ipr_free_cmd(ipr_cmd);
>> scsi_cmd->scsi_done(scsi_cmd);
>> - spin_unlock_irqrestore(ipr_cmd->hrrq->lock, lock_flags);
>> } else {
>> spin_lock_irqsave(ioa_cfg->host->host_lock, lock_flags);
>> spin_lock(&ipr_cmd->hrrq->_lock);
>> @@ -6513,9 +6511,27 @@ static int ipr_queuecommand(struct Scsi_
>>
>> hrrq = &ioa_cfg->hrrq[hrrq_id];
>>
>> + if (!ipr_hrrq_enter(hrrq)) {
>> + dev_info(&ioa_cfg->pdev->dev, "Failed to get a ref\n");
>> +
>> + spin_lock_irqsave(hrrq->lock, hrrq_flags);
>> + if (hrrq->ioa_is_dead || hrrq->removing_ioa) {
>> + spin_unlock_irqrestore(hrrq->lock, hrrq_flags);
>> + memset(scsi_cmd->sense_buffer, 0,
>> SCSI_SENSE_BUFFERSIZE);
>> + scsi_cmd->result = (DID_NO_CONNECT << 16);
>> + scsi_cmd->scsi_done(scsi_cmd);
>> + return 0;
>> + }
>> +
>> + spin_unlock_irqrestore(hrrq->lock, hrrq_flags);
>> + return SCSI_MLQUEUE_HOST_BUSY;
>> + }
>> +
>> ipr_cmd = __ipr_get_free_ipr_cmnd(hrrq);
>> - if (ipr_cmd == NULL)
>> + if (ipr_cmd == NULL) {
>> + ipr_hrrq_exit(hrrq);
>> return SCSI_MLQUEUE_HOST_BUSY;
>> + }
>
>
> ipr_cmd is associated with hrrq. Does it make sense to get a free ipr
> cmnd without holding a reference to the hrrq? If not, I think it makes
> sense move the reference acquisition code to that function and simplify
> the logic in the queuecommand function.
Yes, it does. We do it a bunch of other places while holding a lock. I tried to
adopt the philosophy here of optimize the hot path and minimize changes outside
the hot path.
>> @@ -6560,38 +6586,20 @@ static int ipr_queuecommand(struct Scsi_
>> else
>> rc = ipr_build_ioadl(ioa_cfg, ipr_cmd);
>>
>> - spin_lock_irqsave(hrrq->lock, hrrq_flags);
>> - if (unlikely(rc || (!hrrq->allow_cmds && !hrrq->ioa_is_dead &&
>> !hrrq->removing_ioa))) {
>> - ipr_free_cmd(ipr_cmd);
>> - spin_unlock_irqrestore(hrrq->lock, hrrq_flags);
>> - if (!rc)
>> + if (rc) {
>> + if (!ipr_cmnd_complete(ipr_cmd)) {
>> + ipr_free_cmd(ipr_cmd);
>> scsi_dma_unmap(scsi_cmd);
>> + }
>> + ipr_hrrq_exit(hrrq);
>> return SCSI_MLQUEUE_HOST_BUSY;
>> }
>>
>> - if (unlikely(hrrq->ioa_is_dead || hrrq->removing_ioa)) {
>> - ipr_free_cmd(ipr_cmd);
>> - spin_unlock_irqrestore(hrrq->lock, hrrq_flags);
>> - scsi_dma_unmap(scsi_cmd);
>> - goto err_nodev;
>> - }
>> -
>> ioarcb->res_handle = res->res_handle;
>> - if (res->needs_sync_complete) {
>> - ioarcb->cmd_pkt.flags_hi |= IPR_FLAGS_HI_SYNC_COMPLETE;
>> - res->needs_sync_complete = 0;
>> - }
>> +
>> ipr_trc_hook(ipr_cmd, IPR_TRACE_START, IPR_GET_RES_PHYS_LOC(res));
>> ipr_send_command(ipr_cmd);
>> - spin_unlock_irqrestore(hrrq->lock, hrrq_flags);
>> - return 0;
>> -
>> -err_nodev:
>> - spin_lock_irqsave(hrrq->lock, hrrq_flags);
>> - memset(scsi_cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
>> - scsi_cmd->result = (DID_NO_CONNECT << 16);
>> - scsi_cmd->scsi_done(scsi_cmd);
>> - spin_unlock_irqrestore(hrrq->lock, hrrq_flags);
>> + ipr_hrrq_exit(hrrq);
>
> Shouldn't we hold the hrrq until completing the request?
No. If we did that it would cause issues for our adapter reset job which
might be resetting the adapter because I/O is wedged. The ipr_hrrq_enter/exit
code is really needed to simply ensure we don't reset the adapter while
we are:
1. Referencing any resource entries.
2. Sending new commands to the adapter. If we were to call ipr_send_command
at the same time as we were running BIST on the adapter, it would EEH.
Thanks,
Brian
--
Brian King
Power Linux I/O
IBM Linux Technology Center
------------------------------------------------------------------------------
_______________________________________________
Iprdd-devel mailing list
Iprdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iprdd-devel
