Re: [PATCH 01/84] scsi: core: Use a member variable to track the SCSI command submitter

From: Bart Van Assche <bvanassche@acm.org>
To: Benjamin Block <bblock@linux.ibm.com>
Cc: "Martin K . Petersen" <martin.petersen@oracle.com>,
	linux-scsi@vger.kernel.org, Hannes Reinecke <hare@suse.com>,
	Ming Lei <ming.lei@redhat.com>, Christoph Hellwig <hch@lst.de>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>
Subject: Re: [PATCH 01/84] scsi: core: Use a member variable to track the SCSI command submitter
Date: Fri, 24 Sep 2021 20:38:42 -0700	[thread overview]
Message-ID: <4bc6bf9c-a6bd-13c7-988b-9756bb5dd480@acm.org> (raw)
In-Reply-To: <YU2cN5H7CqVFOzTQ@t480-pf1aa2c2.linux.ibm.com>

On 9/24/21 02:36, Benjamin Block wrote:
> On Fri, Sep 17, 2021 at 05:04:44PM -0700, Bart Van Assche wrote:
>> Conditional statements are faster than indirect calls. Use a member variable
>> to track the SCSI command submitter such that later patches can call
>> scsi_done(scmd) instead of scmd->scsi_done(scmd).
>>
>> The asymmetric behavior that scsi_send_eh_cmnd() sets the submission
>> context to the SCSI error handler and that it does not restore the
>> submission context to the SCSI core is retained.
>>
>> Cc: Hannes Reinecke <hare@suse.com>
>> Cc: Ming Lei <ming.lei@redhat.com>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   drivers/scsi/scsi_error.c | 18 +++++++-----------
>>   drivers/scsi/scsi_lib.c   |  9 +++++++++
>>   drivers/scsi/scsi_priv.h  |  1 +
>>   include/scsi/scsi_cmnd.h  |  7 +++++++
>>   4 files changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 572673873ddf..ba6d748a0246 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -1577,6 +1577,15 @@ static blk_status_t scsi_prepare_cmd(struct request *req)
>>   
>>   static void scsi_mq_done(struct scsi_cmnd *cmd)
>>   {
>> +	switch (cmd->submitter) {
>> +	case BLOCK_LAYER:
>> +		break;
>> +	case SCSI_ERROR_HANDLER:
>> +		return scsi_eh_done(cmd);
>> +	case SCSI_RESET_IOCTL:
>> +		return;
>> +	}
>> +
> 
> Hmm, I'm confused, you replace one kind of branch with different one. Why
> would that increase IOPS by 5%?
> 
> Maybe its because the new `submitter` field in `struct scsi_cmnd` is now
> on a hot cache line, whereas `*scsi_done` is not?

Hi Benjamin,

To be honest, the 5% improvement is more than I had expected. This is what I
know about indirect function calls vs. branches:
- The target of an indirect branch is predicted by the indirect branch
   predictor. For direct branches the Branch Target Buffer (BTB) is used.
- The performance of indirect calls is negatively affected by security
   mitigations (CONFIG_RETPOLINE) but not the performance of direct branches
   My measurement was run with CONFIG_RETPOLINE off. I expect a larger
   difference with CONFIG_RETPOLINE enabled.

Maybe I triggered inefficient behavior of the indirect branch predictor with
the workload I ran.

Bart.