On 09/08/2016 12:37 PM, Gabriel Krisman Bertazi wrote:
> Brian King <brk...@linux.vnet.ibm.com> writes:
>
>> This converts ipr's queuecommand path to use percpu refcounts
>> rather than atomics.
>>
>> Signed-off-by: Brian King <brk...@linux.vnet.ibm.com>
>> ---
>>
>> drivers/scsi/ipr.c | 111
>> ++++++++++++++++++++++++++++++++++++++++++-----------
>> drivers/scsi/ipr.h | 53 +++----------------------
>> 2 files changed, 98 insertions(+), 66 deletions(-)
>>
>> diff -puN drivers/scsi/ipr.h~ipr_lockless_percpu drivers/scsi/ipr.h
>> --- linux-2.6.git/drivers/scsi/ipr.h~ipr_lockless_percpu 2016-09-06
>> 21:08:18.268234535 -0500
>> +++ linux-2.6.git-bjking1/drivers/scsi/ipr.h 2016-09-06 21:08:18.278234468
>> -0500
>> @@ -1472,6 +1471,12 @@ enum ipr_sdt_state {
>> DUMP_OBTAINED
>> };
>>
>> +struct ipr_ioa_ref_cnt {
>> + struct percpu_ref ref;
>> + int released;
>> + struct ipr_ioa_cfg *ioa_cfg;
>> +};
>
> Do we really need the pointer back to ioa_cfg? The released variable
> could be inferred if with keep track of the first reference. If we
> handle these two points, we don't need the ipr_ioa_ref_cnt structure and
> can encapsulate the percpu_ref counter directly in the main structure.
I think you are right. This code has been through a few different versions
and this is an artifact of a previous version. I'll simplify it.
>
>> +
>> /* Per-controller data */
>> struct ipr_ioa_cfg {
>> char eye_catcher[8];
>> diff -puN drivers/scsi/ipr.c~ipr_lockless_percpu drivers/scsi/ipr.c
>> --- linux-2.6.git/drivers/scsi/ipr.c~ipr_lockless_percpu 2016-09-06
>> 21:08:18.272234509 -0500
>> +++ linux-2.6.git-bjking1/drivers/scsi/ipr.c 2016-09-06 21:08:18.280234455
>> -0500
>> @@ -6511,7 +6511,7 @@ static int ipr_queuecommand(struct Scsi_
>>
>> hrrq = &ioa_cfg->hrrq[hrrq_id];
>>
>> - if (!ipr_hrrq_enter(hrrq)) {
>> + if (!percpu_ref_tryget_live(&ioa_cfg->ioa_usage_counter->ref)) {
>
> I'd like to see these calls encapsulated inside ipr_ioa_enter,ipr_ioa_exit or
> something like that. Just cosmetic, though.
I had thought about that as well but had resisted. I'll make the change.
>> +
>> +/**
>> + * ipr_reset_quiesce_work - Quiesce queuecommand
>> + * @work: work struct
>> + *
>> + * Description: Quiesce queuecommand.
>> + *
>> + **/
>> +static void ipr_reset_quiesce_work(struct work_struct *work)
>> +{
>> + struct ipr_cmnd *ipr_cmd = container_of(work, struct ipr_cmnd, work);
>> + struct ipr_ioa_cfg *ioa_cfg = ipr_cmd->ioa_cfg;
>> + unsigned long lock_flags = 0;
>> + int allow_cmds;
>> + ENTER;
>> +
>> + spin_lock_irqsave(ioa_cfg->host->host_lock, lock_flags);
>> + allow_cmds = ioa_cfg->allow_cmds;
>> + ioa_cfg->allow_cmds = 0;
>> + spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags);
>> +
>> + /* xxx add a timeout here? */
>
> Wouldn't a timeout indicate a bug in the code? I think we don't need an
> explicit timeout, since it should always converge to zero, unless
> there's a bug.
Agreed. Removing the comment.
>
>> + if (allow_cmds)
>> + percpu_ref_kill(&ioa_cfg->ioa_usage_counter->ref);
>> + else
>> + ipr_ioa_usage_counter_release(&ioa_cfg->ioa_usage_counter->ref);
>> + LEAVE;
>> +}
--
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
