On 2018/08/06 13:51, Douglas Gilbert wrote:
> Make the two block layer operations most frequently used (REQ_OP_READ
> and REQ_OP_WRITE) bypass the switch statements in the submission and
> response paths. Assume these two enums are 0 and 1 which allows a
> single comparison to select both of them. Check that assumption
> at driver start-up.
What about simply moving the REQ_OP_READ and REQ_OP_WRITE cases at the top of
the switch case list ? Since that gets (usually) compiled as a jump, the effect
should be the same, no ?
>
> Signed-off-by: Douglas Gilbert <dgilbert@xxxxxxxxxxxx>
> ---
> drivers/scsi/sd.c | 82 +++++++++++++++++++++++++++++++----------------
> 1 file changed, 55 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 4b1402633c36..9f047fd3c92d 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1227,6 +1227,14 @@ static int sd_init_command(struct scsi_cmnd *cmd)
> {
> struct request *rq = cmd->request;
>
> + /*
> + * Assumption that REQ_OP_READ and REQ_OP_WRITE are 0 and 1 is
> + * checked in init_sd(). The following "if" is done to dispatch
> + * REQ_OP_READ and REQ_OP_WRITE quickly.
> + */
> + if (likely(req_op(rq) < 2))
> + return sd_setup_read_write_cmnd(cmd);
> +
> switch (req_op(rq)) {
> case REQ_OP_DISCARD:
> switch (scsi_disk(rq->rq_disk)->provisioning_mode) {
> @@ -1247,9 +1255,6 @@ static int sd_init_command(struct scsi_cmnd *cmd)
> return sd_setup_write_same_cmnd(cmd);
> case REQ_OP_FLUSH:
> return sd_setup_flush_cmnd(cmd);
> - case REQ_OP_READ:
> - case REQ_OP_WRITE:
> - return sd_setup_read_write_cmnd(cmd);
> case REQ_OP_ZONE_REPORT:
> return sd_zbc_setup_report_cmnd(cmd);
> case REQ_OP_ZONE_RESET:
> @@ -1973,30 +1978,12 @@ static int sd_done(struct scsi_cmnd *SCpnt)
> int sense_valid = 0;
> int sense_deferred = 0;
>
> - switch (req_op(req)) {
> - case REQ_OP_DISCARD:
> - case REQ_OP_WRITE_ZEROES:
> - case REQ_OP_WRITE_SAME:
> - case REQ_OP_ZONE_RESET:
> - if (!result) {
> - good_bytes = blk_rq_bytes(req);
> - scsi_set_resid(SCpnt, 0);
> - } else {
> - good_bytes = 0;
> - scsi_set_resid(SCpnt, blk_rq_bytes(req));
> - }
> - break;
> - case REQ_OP_ZONE_REPORT:
> - if (!result) {
> - good_bytes = scsi_bufflen(SCpnt)
> - - scsi_get_resid(SCpnt);
> - scsi_set_resid(SCpnt, 0);
> - } else {
> - good_bytes = 0;
> - scsi_set_resid(SCpnt, blk_rq_bytes(req));
> - }
> - break;
> - default:
> + /*
> + * Assumption that REQ_OP_READ and REQ_OP_WRITE are 0 and 1 is
> + * checked in init_sd(). The following "if" is done to expedite
> + * REQ_OP_READ and REQ_OP_WRITE.
> + */
> + if (likely(req_op(req) < 2)) {
> /*
> * In case of bogus fw or device, we could end up having
> * an unaligned partial completion. Check this here and force
> @@ -2011,6 +1998,36 @@ static int sd_done(struct scsi_cmnd *SCpnt)
> round_up(resid, sector_size));
> scsi_set_resid(SCpnt, resid);
> }
> + } else {
> + switch (req_op(req)) {
> + case REQ_OP_DISCARD:
> + case REQ_OP_WRITE_ZEROES:
> + case REQ_OP_WRITE_SAME:
> + case REQ_OP_ZONE_RESET:
> + if (!result) {
> + good_bytes = blk_rq_bytes(req);
> + scsi_set_resid(SCpnt, 0);
> + } else {
> + good_bytes = 0;
> + scsi_set_resid(SCpnt, blk_rq_bytes(req));
> + }
> + break;
> + case REQ_OP_ZONE_REPORT:
> + if (!result) {
> + good_bytes = scsi_bufflen(SCpnt)
> + - scsi_get_resid(SCpnt);
> + scsi_set_resid(SCpnt, 0);
> + } else {
> + good_bytes = 0;
> + scsi_set_resid(SCpnt, blk_rq_bytes(req));
> + }
> + break;
> + default:
> + sd_printk(KERN_NOTICE, sdkp, "unexpected REQ_OP=%u\n",
> + (unsigned int)req_op(req));
> + WARN_ON_ONCE(true);
> + break;
> + }
> }
>
> if (result) {
> @@ -3597,6 +3614,17 @@ static int __init init_sd(void)
> int majors = 0, i, err;
>
> SCSI_LOG_HLQUEUE(3, printk("init_sd: sd driver entry point\n"));
> + /*
> + * The sd_init_command() and sd_done() assume REQ_OP_READ and
> + * REQ_OP_WRITE are 0 and 1 and will fail if they are not. If they
> + * are not, would prefer a compile failure but the preprocessor can't
> + * use enum constants. Place check here because only need to check
> + * early and once.
> + */
> + if (REQ_OP_READ + REQ_OP_WRITE > 1)
> + pr_err("%s: REQ_OP_READ=%d REQ_OP_WRITE=%d %s.\n", __func__,
> + REQ_OP_READ, REQ_OP_WRITE,
> + "expected 0 and 1. Logic ERROR");
>
> for (i = 0; i < SD_MAJORS; i++) {
> if (register_blkdev(sd_major(i), "sd") != 0)
>
--
Damien Le Moal
Western Digital Research
