[PATCH 3/3] scsi/ncr5380: Improve interrupt latency during PIO tranfers

[Finn Thain]

[Geert Uytterhoeven]

Hi Finn,

On Sat, Aug 27, 2016 at 4:30 AM, Finn Thain <fth...@telegraphics.com.au> wrote:
> Large PIO transfers are broken up into chunks to try to avoid disabling
> local IRQs for long periods. But IRQs are still disabled for too long
> and this causes SCC FIFO overruns during serial port transfers. This
> patch fixes the problem by halving the PIO chunk size.
>
> Testing with mac_scsi shows that the extra NCR5380_main() loop iterations
> have negligible performance impact on SCSI transfers (about 1% slower).
> On a faster system (using the dmx3191d module) transfers showed no
> measurable change.
>
> Signed-off-by: Finn Thain <fth...@telegraphics.com.au>
>
> ---
> drivers/scsi/NCR5380.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> Index: linux/drivers/scsi/NCR5380.c
> ===================================================================
> --- linux.orig/drivers/scsi/NCR5380.c 2016-08-27 12:29:57.000000000 +1000
> +++ linux/drivers/scsi/NCR5380.c 2016-08-27 12:29:58.000000000 +1000
> @@ -1847,11 +1847,11 @@ static void NCR5380_information_transfer
> /* XXX - need to source or sink data here, as appropriate */
> }
> } else {
> - /* Break up transfer into 3 ms chunks,
> - * presuming 6 accesses per handshake.
> + /* Transfer a small chunk so that the
> + * irq mode lock is not held too long.
> */
> transfersize = min((unsigned long)cmd->SCp.this_residual,
> - hostdata->accesses_per_ms / 2);
> + hostdata->accesses_per_ms >> 2);

I think it's easier to read if you use "/ 4".

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Finn Thain]

I think the factor, "1/4 byte milliseconds per access" is not very
meaningful. The PIO transfersize can be understood as,

pio_bytes_until_scc_fifo_overflow = accesses_per_ms /
(accesses_per_pio_byte / ms_until_fifo_overflow)

This loop seemed like a good place to avoid a DIV instruction (though I
didn't try to confirm that) and so I used a bit shift to indicate that
intention.

The shift amount was an empirical result that happened to work for the
hardware I tested it on, at the baud rate I was using. Admittedly, if we
want to avoid further tweaks to this then I'll have to do more testing and
find a better approximation.

--

>
> Gr{oetje,eeting}s,
>
> Geert
>
--

[Finn Thain]

[Laurence Oberman]

----- Original Message -----
> From: "Finn Thain" <fth...@telegraphics.com.au>
> To: "James E.J. Bottomley" <je...@linux.vnet.ibm.com>, "Martin K. Petersen" <martin....@oracle.com>, "Geert
> Uytterhoeven" <ge...@linux-m68k.org>, linux...@vger.kernel.org, linux...@vger.kernel.org,
> linux-...@vger.kernel.org, "Michael Schmitz" <schmi...@gmail.com>
> Sent: Wednesday, August 31, 2016 12:44:56 AM
> Subject: [PATCH v2 3/3] scsi/ncr5380: Improve interrupt latency during PIO tranfers
>
> Large PIO transfers are broken up into chunks to try to avoid disabling
> local IRQs for long periods. But IRQs are still disabled for too long
> and this causes SCC FIFO overruns during serial port transfers.
>

> This patch reduces the PIO chunk size to reduce interrupt latency to
> something on the order of milliseconds, at the expense of additional CPU
> overhead from extra iterations of the NCR5380_main() loop.
>
> That CPU overhead is a problem for slow machines (e.g. mac_scsi on 25 MHz
> 68030) but these machines generally use PDMA not PIO. This patch doesn't
> make the overhead any worse on my Mac LC III (because it only gets about
> 510 accesses per ms).
>
> This patch decreases disk performance by a fraction of one percent for
> dmx3191d on my 333 MHz PowerPC 750. Other affected hardware (such as
> g_NCR5380 on x86) was not tested but 5380 ISA cards generally use PDMA
> and not PIO.

>
> Signed-off-by: Finn Thain <fth...@telegraphics.com.au>
>
> ---

> Changed since v1:
> - PIO transfer chunk size is now hard-coded for simplicity.
>
> ---
> drivers/scsi/NCR5380.c | 8 ++++----
> drivers/scsi/NCR5380.h | 2 ++
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> Index: linux/drivers/scsi/NCR5380.c
> ===================================================================
> --- linux.orig/drivers/scsi/NCR5380.c 2016-08-31 14:44:51.000000000 +1000
> +++ linux/drivers/scsi/NCR5380.c 2016-08-31 14:44:52.000000000 +1000
> @@ -1849,11 +1849,11 @@ static void NCR5380_information_transfer

> /* XXX - need to source or sink data here, as appropriate */
> }
> } else {
> - /* Break up transfer into 3 ms chunks,
> - * presuming 6 accesses per handshake.
> + /* Transfer a small chunk so that the
> + * irq mode lock is not held too long.
> */

> - transfersize = min((unsigned long)cmd->SCp.this_residual,

> - hostdata->accesses_per_ms / 2);

> + transfersize = min(cmd->SCp.this_residual,
> + NCR5380_PIO_CHUNK_SIZE);
> len = transfersize;
> NCR5380_transfer_pio(instance, &phase, &len,
> (unsigned char **)&cmd->SCp.ptr);
> Index: linux/drivers/scsi/NCR5380.h
> ===================================================================
> --- linux.orig/drivers/scsi/NCR5380.h 2016-08-31 14:44:51.000000000 +1000
> +++ linux/drivers/scsi/NCR5380.h 2016-08-31 14:44:52.000000000 +1000
> @@ -250,6 +250,8 @@ struct NCR5380_cmd {
>
> #define NCR5380_CMD_SIZE (sizeof(struct NCR5380_cmd))
>
> +#define NCR5380_PIO_CHUNK_SIZE 256
> +
> static inline struct scsi_cmnd *NCR5380_to_scmd(struct NCR5380_cmd
> *ncmd_ptr)
> {
> return ((struct scsi_cmnd *)ncmd_ptr) - 1;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in

> the body of a message to majo...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

Patch makes sense, look good to me.

Reviwed-by: Laurence Oberman <lobe...@redhat.com>

[Martin K. Petersen]

>>>>> "Finn" == Finn Thain <fth...@telegraphics.com.au> writes:

Finn> Large PIO transfers are broken up into chunks to try to avoid
Finn> disabling local IRQs for long periods. But IRQs are still disabled
Finn> for too long and this causes SCC FIFO overruns during serial port
Finn> transfers.

Finn> This patch reduces the PIO chunk size to reduce interrupt latency
Finn> to something on the order of milliseconds, at the expense of
Finn> additional CPU overhead from extra iterations of the
Finn> NCR5380_main() loop.

Finn> That CPU overhead is a problem for slow machines (e.g. mac_scsi on
Finn> 25 MHz
Finn> 68030) but these machines generally use PDMA not PIO. This patch
Finn> doesn't
Finn> make the overhead any worse on my Mac LC III (because it only gets
Finn> about 510 accesses per ms).

Finn> This patch decreases disk performance by a fraction of one percent
Finn> for dmx3191d on my 333 MHz PowerPC 750. Other affected hardware
Finn> (such as g_NCR5380 on x86) was not tested but 5380 ISA cards
Finn> generally use PDMA and not PIO.

Geert?

--
Martin K. Petersen Oracle Linux Engineering

[Geert Uytterhoeven]

On Fri, Sep 9, 2016 at 1:28 PM, Martin K. Petersen
<martin....@oracle.com> wrote:
>>>>>> "Finn" == Finn Thain <fth...@telegraphics.com.au> writes:
>
> Finn> Large PIO transfers are broken up into chunks to try to avoid
> Finn> disabling local IRQs for long periods. But IRQs are still disabled
> Finn> for too long and this causes SCC FIFO overruns during serial port
> Finn> transfers.
>
> Finn> This patch reduces the PIO chunk size to reduce interrupt latency
> Finn> to something on the order of milliseconds, at the expense of
> Finn> additional CPU overhead from extra iterations of the
> Finn> NCR5380_main() loop.
>
> Finn> That CPU overhead is a problem for slow machines (e.g. mac_scsi on
> Finn> 25 MHz
> Finn> 68030) but these machines generally use PDMA not PIO. This patch
> Finn> doesn't
> Finn> make the overhead any worse on my Mac LC III (because it only gets
> Finn> about 510 accesses per ms).
>
> Finn> This patch decreases disk performance by a fraction of one percent
> Finn> for dmx3191d on my 333 MHz PowerPC 750. Other affected hardware
> Finn> (such as g_NCR5380 on x86) was not tested but 5380 ISA cards
> Finn> generally use PDMA and not PIO.
>
> Geert?

No comments from my side.

Gr{oetje,eeting}s,

Geert

--

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[Martin K. Petersen]

>>>>> "Finn" == Finn Thain <fth...@telegraphics.com.au> writes:

Finn> Large PIO transfers are broken up into chunks to try to avoid
Finn> disabling local IRQs for long periods. But IRQs are still disabled
Finn> for too long and this causes SCC FIFO overruns during serial port
Finn> transfers.

Finn> This patch reduces the PIO chunk size to reduce interrupt latency
Finn> to something on the order of milliseconds, at the expense of
Finn> additional CPU overhead from extra iterations of the
Finn> NCR5380_main() loop.

Applied to 4.9/scsi-queue.

--
Martin K. Petersen Oracle Linux Engineering